Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Mar 30, 2022

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.

@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 changed the base branch from master to dove October 27, 2021 12:08
@vonagam
Copy link
Member Author

vonagam commented Oct 27, 2021

Rebased.

@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants