8000 feat(authentication): Add setup method for auth strategies by vonagam · Pull Request #1611 · feathersjs/feathers · GitHub
[go: up one dir, main page]
Skip to content

feat(authentication): Add setup method for auth strategies#1611

Merged
daffl merged 3 commits intofeathersjs:dovefrom
vonagam:feat-auth-strategy-setup
Mar 30, 2022
Merged

feat(authentication): Add setup method for auth strategies#1611
daffl merged 3 commits intofeathersjs:dovefrom
vonagam:feat-auth-strategy-setup

Conversation

@vonagam
Copy link
Member
@vonagam vonagam commented Oct 9, 2019

Add setup method to AuthenticationStrategy by analogy with service setup. More convenient alternative to setAuthentication/setApplication/setName and removes the need for getters, since it ensures that all services are already registered by the time of a call.

For example, right now every time you get entityService in a strategy you call:

  1. AuthenticationBaseStrategy.configuration
  2. AuthenticationBase.configuration
  3. Object.assign and app.get
  4. app.service

All of those calls can be done once in setup method instead doing them every time.

Also fixed small inconsistencies in app.setup:

  1. Moved this line up since otherwise a service registered in other service setup call will not get setup call.

  2. Moved those lines down so that setup call for a dynamically registered service happens after the service is registered just like for a usual service.

@vonagam vonagam force-pushed the feat-auth-strategy-setup branch from d50263b to f818a04 Compare December 15, 2019 22:04
@jnardone
Copy link
Contributor
jnardone commented Feb 4, 2020

couldn't you do all of this in setApplication?

@vonagam
Copy link
Member Author
vonagam commented Feb 4, 2020

it ensures that all services are already registered by the time of a call

setApplication (with setName, setAuthentication and verifyConfiguration) is called in register method, before setup, and not everything is set up at that point (not all services registered).

Why have those separate methods when one will do? In Service you do not have setApplication, setName or verifyConfiguration, only setup.

@jnardone
Copy link
Contributor
jnardone commented Feb 4, 2020

Good points. This would be very useful for a project I'm currently working on.

@daffl daffl closed this Jul 11, 2020
@daffl
Copy link
Member
daffl commented Jul 11, 2020

Hm, sorry I'm not sure why this got closed. Going to reopen but will review again to add in v5 (which will also have asynchronous application setup).

@daffl daffl reopened this Jul 11, 2020
@vonagam vonagam force-pushed the feat-auth-strategy-setup branch from f818a04 to 4a5b7ff Compare October 27, 2021 12:07
@vonagam vonagam changed the base branch from master to dove October 27, 2021 12:08
@vonagam
Copy link
Member Author
vonagam commented Oct 27, 2021

Rebased.

@vonagam vonagam mentioned this pull request Dec 13, 2021
Closed
@daffl daffl changed the title feat: Add setup method for auth strategies feat(authentication): Add setup method for auth strategies Mar 30, 2022
@daffl daffl merged commit a3c3581 into feathersjs:dove Mar 30, 2022
@daffl
Copy link
Member
daffl commented Mar 30, 2022

Will go out in the next prerelease. Better late than never I guess, sorry about that 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vonagam @jnardone @daffl
0