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

Third-Party Auth - move session verification #5262

Conversation

pevey
Copy link
Contributor

@pevey pevey commented Sep 30, 2023

This PR is in relation to previous discussion about implementing custom auth, and specifically this conversation: #5251

It moves session verification to auth service methods so that they can be overridden. The authenticate and authenticateCustomer middleware handlers no longer invoke passport directly. Instead, they invoke verifySession and verifyCustomerSession, two new methods added to the authService.

I think moving this functionality make a lot of sense. New users will be looking in the auth service to try to find out how to implement custom auth. It is less intuitive to look for the middlewares. And making them service methods makes them possible to override, making it much simpler to replace the express-session/passport auth entirely and use third-party auth instead.

This should be a non-breaking change. This will make customizing auth much less intimidating since users will not have to figure out how to make it play nice with passport if they don't even want/need to use passport. They can replace it entirely.

@pevey pevey requested a review from a team as a code owner September 30, 2023 16:20
@changeset-bot
Copy link

changeset-bot bot commented Sep 30, 2023

⚠️ No Changeset found

Latest commit: 6fbff01

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Sep 30, 2023

@pevey is attempting to deploy a commit to the medusajs Team on Vercel.

A member of the Team first needs to authorize it.

@joeldodich
Copy link

@pevey @SonnyFishback Where are we at with this? Would love to implement Auth0 or something similar to abstract away auth but seems like waiting for this makes a lot of sense.

@0xliam
Copy link

0xliam commented May 15, 2024

This would be a great feature for our use case - I am building an app with an existing authentication service, and we would like to display parts of Medusa in a unified frontend. I need a way to log users into both our existing app and Medusa, but this isn't currently possible without major changes to the way Medusa handles sessions.

This would fit our use case perfectly, as we would be able to bypass Passport and reuse our existing authentication/sessions.

@riqwan
Copy link
Contributor

riqwan commented Jul 5, 2024

Hey, thanks for the PR! Since v2 brought a lot of architectural changes, we will be closing this PR since it no longer applies to our new setup.

If you would still like to open this PR, please rebase with the v1 branch and re-request a review & we'll get back on it quickly.

@riqwan riqwan closed this Jul 5, 2024
@thomasdefilippis
Copy link

@pevey can you resubmit as this is a feature that we would like to implement with our setup as well.

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.

6 participants