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

fix(medusa): Separate JWT auth strategies per domain #2646

Merged
merged 19 commits into from
Nov 22, 2022

Conversation

pKorsholm
Copy link
Contributor

@pKorsholm pKorsholm commented Nov 21, 2022

What
Separate JWT auth strategies per domain

@pKorsholm pKorsholm requested a review from a team as a code owner November 21, 2022 19:35
@changeset-bot
Copy link

changeset-bot bot commented Nov 21, 2022

🦋 Changeset detected

Latest commit: f9525ba

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@medusajs/medusa Patch
medusa-core-utils Patch

Not sure what this means? Click here to learn what changesets are.

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

@olivermrbl
Copy link
Contributor

/snapshot-this

@olivermrbl
Copy link
Contributor

/snapshot-this

@github-actions
Copy link
Contributor

🚀 A snapshot release has been made for this PR

Test the snapshots by updating your package.json with the newly published versions:

yarn add @medusajs/medusa@0.0.0-snapshot-20221121194915
yarn add medusa-core-utils@0.0.0-snapshot-20221121194915

Latest commit: 6a1df8b

@olivermrbl olivermrbl changed the title hotfix fix(medusa): Separate JWT auth strategies per domain Nov 21, 2022
@patrick-medusajs patrick-medusajs self-requested a review November 21, 2022 20:16
@olivermrbl olivermrbl changed the title fix(medusa): Separate JWT auth strategies per domain fix(medusa): TBA Nov 21, 2022
@olivermrbl
Copy link
Contributor

/snapshot-this

@github-actions
Copy link
Contributor

🚀 A snapshot release has been made for this PR

Test the snapshots by updating your package.json with the newly published versions:

yarn add @medusajs/medusa@0.0.0-snapshot-20221121211122
yarn add medusa-core-utils@0.0.0-snapshot-20221121211122

Latest commit: 6a1df8b

@olivermrbl
Copy link
Contributor

/snapshot-this

@github-actions
Copy link
Contributor

🚀 A snapshot release has been made for this PR

Test the snapshots by updating your package.json with the newly published versions:

yarn add @medusajs/medusa@0.0.0-snapshot-20221121213619
yarn add medusa-core-utils@0.0.0-snapshot-20221121213619

Latest commit: 6a1df8b

@olivermrbl
Copy link
Contributor

/snapshot-this

@github-actions
Copy link
Contributor

🚀 A snapshot release has been made for this PR

Test the snapshots by updating your package.json with the newly published versions:

yarn add @medusajs/medusa@0.0.0-snapshot-20221121224430
yarn add medusa-core-utils@0.0.0-snapshot-20221121224430

Latest commit: 6a1df8b

@olivermrbl
Copy link
Contributor

/snapshot-this

@github-actions
Copy link
Contributor

🚀 A snapshot release has been made for this PR

Test the snapshots by updating your package.json with the newly published versions:

yarn add @medusajs/medusa@0.0.0-snapshot-20221121230632
yarn add medusa-core-utils@0.0.0-snapshot-20221121230632

Latest commit: 6a1df8b

@olivermrbl
Copy link
Contributor

/snapshot-this

@olivermrbl
Copy link
Contributor

/snapshot-this

@github-actions
Copy link
Contributor

🚀 A snapshot release has been made for this PR

Test the snapshots by updating your package.json with the newly published versions:

yarn add @medusajs/medusa@0.0.0-snapshot-20221122082111
yarn add medusa-core-utils@0.0.0-snapshot-20221122082111

Latest commit: 6a1df8b


export default {
authenticate,
authenticateCustomer,
authenticateCustomerOrThrow,
Copy link
Member

@adrien2p adrien2p Nov 22, 2022

Choose a reason for hiding this comment

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

suggestion: requireAuthenticateCustomer just to avoid confusion for the consumer

Copy link
Member

@adrien2p adrien2p left a comment

Choose a reason for hiding this comment

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

LGTM 🎉 just a little suggestion non blocking

Copy link
Member

@adrien2p adrien2p left a comment

Choose a reason for hiding this comment

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

The tests need to be updated the reflect the auth changes 🚀

@pKorsholm
Copy link
Contributor Author

@adrien2p done 😎

@adrien2p
Copy link
Member

@adrien2p done 😎

perfect 🚀 it looks like we've done it 😍

Copy link
Contributor

@kasperkristensen kasperkristensen left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@olivermrbl olivermrbl changed the title fix(medusa): TBA fix(medusa): Separate JWT auth strategies per domain Nov 22, 2022
Copy link
Contributor

@olivermrbl olivermrbl left a comment

Choose a reason for hiding this comment

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

🚀

@kodiakhq kodiakhq bot merged commit e7c4cc3 into master Nov 22, 2022
@kodiakhq kodiakhq bot deleted the fix/jwt-strategies branch November 22, 2022 12:30
Copy link
Collaborator

@srindom srindom left a comment

Choose a reason for hiding this comment

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

LGTM!

import passport from "passport"

export default (): RequestHandler => {
return (req: Request, res: Response, next: NextFunction): void => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

thought(non-blocking): when this is called the authenticateCustomer middleware will already be applied. This means the JWT is parsed. We could just verify with a simple if that the customer has been parsed. Not an important change but just wanted to highlight that this might be duplicate work.

Copy link
Member

Choose a reason for hiding this comment

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

You are right, I would just say that a user could use them independently and therefore I am not sure we should go with the idea that we know that internally it has already been parsed. Wdyt?

adrien2p added a commit that referenced this pull request Nov 25, 2022
**What**
Separate JWT auth strategies per domain

Co-authored-by: Oliver Windall Juhl <59018053+olivermrbl@users.noreply.github.com>
Co-authored-by: Adrien de Peretti <25098370+adrien2p@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants