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

Remove Passport #2809

Merged
merged 29 commits into from
Feb 17, 2025
Merged

Remove Passport #2809

merged 29 commits into from
Feb 17, 2025

Conversation

fraxachun
Copy link
Contributor

@fraxachun fraxachun commented Nov 22, 2024

Description

We decided to remove passport and use built in mechanism instead.

  • Change naming scheme from Strategy to AuthService
  • Simplify naming
    • AuthProxyJwtStrategy => JwtAuthService
    • StaticAuthedUserStrategy => StaticUserStrategy
    • StaticCredentialsBasicStrategy => BasicAuthService
  • Use helper function createAuthGuardProviders to create providers (instead intransparent strategy-names)
  • JwtAuthService uses @nestjs/jwt and is more flexible to cover different use cases
  • Move creating the User object to AuthService (this is the correct place, not UserPermissionService or UserService)
  • Use CometAuthGuard working without the need of a factory (makes it easier expandable)
  • Let CometAuthGuard creates the CurrentUser-Object (instead AuthService, they're only for User)
  • Export JwtModule from cms-api (has to be added to imports in auth.module.ts when JwtAuthService is used)

Open TODOs/questions

  • Changeset
  • Migration-Guide (or Upgrade-Script?)
  • Caching of userService.getUser() in CometAuthGuard - separate PR
  • Logging (especially for debug purposes) - separate PR
  • Unit-Testing

Further information

https://vivid-planet.atlassian.net/browse/COM-1201

@fraxachun fraxachun mentioned this pull request Nov 26, 2024
5 tasks
@fraxachun fraxachun requested a review from johnnyomair December 2, 2024 12:31
@johnnyomair
Copy link
Collaborator

Migration-Guide (or Upgrade-Script?)

A section in the migration guide is a must have IMO. If you want, you could also add an upgrade script. Though I believe it would be difficult to cover all usages across our projects.

@thomasdax98 thomasdax98 added this to the v8.0.0 milestone Dec 16, 2024
Copy link
Member

@thomasdax98 thomasdax98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In some projects we use custom passport strategies, sometimes third-party, (e.g. https://www.npmjs.com/package/passport-headerapikey).
As far as I understand, this is still possible without passport in the library. Is this correct or could this cause problems?

@fraxachun
Copy link
Contributor Author

A section in the migration guide is a must have IMO. If you want, you could also add an upgrade script. Though I believe it would be difficult to cover all usages across our projects.

84316fd

@fraxachun
Copy link
Contributor Author

Passport is not supported anymore. But it should be easy adding a custom auth service.

Should we add a note regarding this to the migration guide?

7b66355

thomasdax98
thomasdax98 previously approved these changes Dec 31, 2024
johnnyomair
johnnyomair previously approved these changes Jan 9, 2025
dkarnutsch
dkarnutsch previously approved these changes Jan 21, 2025
nsams
nsams previously approved these changes Jan 30, 2025
@fraxachun
Copy link
Contributor Author

@johnnyomair Do we want this in v8? I would resolve the conflicts then so that you can merge it.

@johnnyomair
Copy link
Collaborator

@fraxachun I'd say yes. @thomasdax98 what do you think?

@thomasdax98
Copy link
Member

@fraxachun I'd say yes. @thomasdax98 what do you think?

Yes, definitely

@fraxachun
Copy link
Contributor Author

@johnnyomair Conflicts resolved

@johnnyomair johnnyomair changed the title Remove passport Remove Passport Feb 17, 2025
@johnnyomair johnnyomair merged commit 0d210fe into next Feb 17, 2025
12 checks passed
@johnnyomair johnnyomair deleted the remove-passport branch February 17, 2025 12:39
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.

5 participants