-
-
Notifications
You must be signed in to change notification settings - Fork 48
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(Auth0): Add Auth0 Authentication #27
Conversation
@adrien2p Need to do some final testing and then will set the PR ready for review. |
Wonderful, could I get you to update the readme as well 😊 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with few comments :)
@juanzgc perfect, if you can just add the authPath and authCallbackPath under the optional props in the readme just like you have done with auth0 it would be amazing. So that people knows the default values |
...ages/medusa-plugin-auth/src/auth-strategies/facebook/__tests__/store/verify-callback.spec.ts
Outdated
Show resolved
Hide resolved
@juanzgc you missed to add in the api/index.ts line 9 in the api/index.ts line 23 |
@mpoitevineau-millin Do you mind giving it a test now? |
@juanzgc it's ok now on my side Maybe we can improve the documentation, I spent a day this weekend to understand how to connect auth0 and the plugin. |
@mpoitevineau-millin Are you testing authentication with the admin or with store? Do you mind testing both if you have time 😇 |
It would be wonderful if you find a way to make the doc more easier to understand 🥰 |
I have an issue with my local build of this PR regarding the session (but this is not link to the auth0 part). So everything is ok regarding auth0 from my tests |
Glad to here @mpoitevineau-millin , I ve shared you a way to test locally. PeerDependencies are always a bit hard to manage locally but with what I ve provide you, you should be good to go. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, few comments though. I did not repeated them everywhere ahah
|
||
const user = await userService.retrieveByEmail(email).catch(() => void 0); | ||
|
||
if (user) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose that for the invitation check we are waiting the next refactoring which extract the validate method is that right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the invitation check we should do in a separate PR
); | ||
} | ||
|
||
const customer = await customerService |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the newest version of medusa, the retrieval of the customer has changed a bit. We can have now multiple customer with the same email. One as a guest and one with an account. I suggest that we check if retrieveRegisteredByEmail
exists on the customer service and fallback to retrieveByEmail
otherwise. wdyt? so that we can support older and newer versions of medusa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you have refactored to only do retrieveRegisteredByEmail
, should we still add the fallback?
@@ -65,7 +69,23 @@ export class FacebookStoreStrategy extends PassportStrategy(FacebookStrategy, FA | |||
.catch(() => void 0); | |||
|
|||
if (customer) { | |||
if (!customer.metadata || !customer.metadata[CUSTOMER_METADATA_KEY]) { | |||
// To prevent Legacy applications from not authenticating because only CUSTOMER_METADATA_KEY was set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can maybe add a todo to remove it in the next major?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we wanted to keep this moving forward to allow for backward compatibility with those currently using the authentication service?
Full test on my side with the PR #32 . 100% ok |
@juanzgc do you know when you will be able to finish this PR? |
@juanzgc in order to not block the other refactoring with that pr. I ve done them and i ll let you update the pr accordingly. Do not forget to update the refacto according to the new metadata keys etc |
@mpoitevineau-millin The PR should now be finished, do you mind giving it one more test to confirm that all is working as expected 😇 @adrien2p Should be good to take a final look at 😇 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great 🎉
emails: [{ value: existsEmailWithMeta }], | ||
}; | ||
|
||
const data = await auth0StoreStrategy.validate(req, accessToken, refreshToken, extraParams, profile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: We should check that the update have been called here. also with the right input data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: Possibly update the tests for the input data. As of now it is a bit more general in all providers
packages/medusa-plugin-auth/src/auth-strategies/auth0/__tests__/store/verify-callback.spec.ts
Show resolved
Hide resolved
@adrien2p in 9ead4c9#diff-2bd386bf42efcc87b13fc16c223b031bd923cd992e5a279976ceaf80c23d4ba0 we added this block to
This is giving an error when testing this PR because passport is authenticating with the |
@Arsenalist this looks like a mistake. Will fix it tomorrow 🙏 thanks |
Small suggestion, and this applies to store and admin routers. When authenticating using passport, can we also have an option to pass in other auth0 specific options to passport? For example:
Many use cases for Auth0 require an audience parameter, so it might be a good idea to specify |
@Arsenalist can i get you to give another test please, it should be good to go 🚀 thanks for the great works @juanzgc |
Store works as expected. When I login to admin with an email that already exists (after signing into Auth0 using the Google social login), I get this message:
Is that expected behaviour? I would imagine an admin logging in with an email that already exists would just log them in and not throw an error? |
Correct, this is expected behavior. Because, you authenticated with a different provider, and could potentially pose a security vulnerability if someone creates an auth0 account with your email. |
As @juanzgc mentioned it is expected for security purpose in order to prevent a user to log into your account on your behalf from another provider. One thing you can do if you want to still allow a user to user multiple provider is to add to your ui the ability to activate/deactivate providers for the user. It is a key manipulation in the user metadata. |
Yes, I understand that now. I also noticed that auth0 returns |
One last thing, here's the rational as per auth0 for having the option to pass in an audience parameter. https://community.auth0.com/t/decode-access-token/62014 Otherwise, the |
We can definitely update the handler to accept an optional extra data object to be added to the token data, so that you can access them in any middleware. I think it make sense and this is something i proposed to the person building the firebase auth. Wdyt @juanzgc Maybe that we only need the validate method to return all the data along the id so that the builder can store the user/customer id plus the rest in the token data 🤔 in that case the builder does not have to take extra argument and instead only use req.user as usual We can also merge as it is, and i create a new pr for the small changes. |
@Arsenalist here we go, #45 if you want to try it out 👍 |
Ran a quick test. Worked well! |
@adrien2p We're ready to test this out if you push it to npm. I do have a question though. Is this for both users and customers? If so, how are they being distinguished? Do we need a special setup on Auth0 to say "this is an user with role=member", "this is a customer", "this is an user with role=admin". |
This is for both user and customers (you can pick one or both), as you can see by the configuration required. You do not need special configuration in auth0 as you can differentiate who is who based on the callback to your app (store and admin have different callbacks). You also have access to the |
Can this be packaged and released? |
# 1.0.0 (2024-12-25) ### Bug Fixes * add support for logout ([7bbefe7](7bbefe7)) * admin.scope and store.scope parameters from medusa-config.js not passed through to passport ([adrien2p#170](https://github.com/xponential-asia/medusa-xpo-auth-plugins/issues/170)) ([b4c3359](b4c3359)) * apply expiresIn from the auth provider config ([76d37d4](76d37d4)) * Auth route builder ([adrien2p#44](https://github.com/xponential-asia/medusa-xpo-auth-plugins/issues/44)) ([b5f6d7a](b5f6d7a)) * auth store strategies ([adrien2p#24](https://github.com/xponential-asia/medusa-xpo-auth-plugins/issues/24)) ([60a03e6](60a03e6)) * authentication strategy cookies and split cookies per domain ([80363de](80363de)) * **auth:** loadRouters ([356fff4](356fff4)) * **auth:** Remove promise options resolution as it is not supported by medusa for now ([8852c3e](8852c3e)) * build callback handler token gegeneration ([adrien2p#31](https://github.com/xponential-asia/medusa-xpo-auth-plugins/issues/31)) ([69313db](69313db)) * check that api token have been provided before attaching the admin routes ([cb392b2](cb392b2)) * CICD node version in semantic release ([59586db](59586db)) * Cors issue ([d14d8f2](d14d8f2)) * Error request always return 200 after being sent to sentry ([c37272a](c37272a)) * failure redirect not working properly ([adrien2p#103](https://github.com/xponential-asia/medusa-xpo-auth-plugins/issues/103)) ([5a10308](5a10308)) * Few fixes on sentry table management and data display ([adrien2p#15](https://github.com/xponential-asia/medusa-xpo-auth-plugins/issues/15)) ([44d7342](44d7342)) * Force medusa deps version to be in range ([7cd666e](7cd666e)) * generate random password the first time ([adrien2p#115](https://github.com/xponential-asia/medusa-xpo-auth-plugins/issues/115)) ([5f5f269](5f5f269)) * google auth route builder ([adrien2p#60](https://github.com/xponential-asia/medusa-xpo-auth-plugins/issues/60)) ([f0d678a](f0d678a)) * Handle user type guest in azure ad ([ed60fed](ed60fed)) * init project ([10fe1d2](10fe1d2)) * int package ([5058353](5058353)) * jwt store token property ([775df46](775df46)) * logout ([c23027d](c23027d)) * logout ([a6c6a08](a6c6a08)) * max value of per page ([aa699fa](aa699fa)) * **medusa/payment-paytr:** Wrong gitignore configuration ([8e07991](8e07991)) * missing update of jwt strategy ([ec8aecf](ec8aecf)) * Node version in cicd ([391c3d6](391c3d6)) * npm files ([ab44413](ab44413)) * On query filter, the cursor should be rested ([ec03564](ec03564)) * revert path changes ([5232bbe](5232bbe)) * sentry typo ([1bb7920](1bb7920)) * update logout handler accordingly to the previous changes ([4f01f31](4f01f31)) * Wrong user property used in store auth strategies ([73812dc](73812dc)) ### Features * Add environment support ([8376066](8376066)) * add Linkedin Oauth 2 support ([acc036b](acc036b)) * Add support for dynamic success redirect url through a query param ([1d16fc1](1d16fc1)) * add support for strict options ([adrien2p#84](https://github.com/xponential-asia/medusa-xpo-auth-plugins/issues/84)) ([3e18cb3](3e18cb3)) * Add twitter OAuth 2 pre-support ([1395689](1395689)) * Allow to access session from the plugin and remove cookie usage ([adrien2p#32](https://github.com/xponential-asia/medusa-xpo-auth-plugins/issues/32)) ([d97a96f](d97a96f)) * Allow to pass a custom verify callback for google strategy ([adrien2p#18](https://github.com/xponential-asia/medusa-xpo-auth-plugins/issues/18)) ([7b0f824](7b0f824)) * **api:** Add support for equation in 'query' query param for sentry end points ([19c5e1e](19c5e1e)) * **Auth0:** Add Auth0 Authentication ([adrien2p#27](https://github.com/xponential-asia/medusa-xpo-auth-plugins/issues/27)) ([06c0c20](06c0c20)) * cleanup old code and start sentry plugin ([dd4a518](dd4a518)) * Implement prometheus plugin ([adrien2p#13](https://github.com/xponential-asia/medusa-xpo-auth-plugins/issues/13)) ([e8c2e47](e8c2e47)) * Include more data from the fetched sentry transactions and improve pagination ([e476e50](e476e50)) * Init project ([040b21c](040b21c)) * **medusa-plugin-auth:** added oauth2 as login strategy ([adrien2p#119](https://github.com/xponential-asia/medusa-xpo-auth-plugins/issues/119)) ([4dde834](4dde834)) * **medusa-plugin-sentry:** add repo to package.json ([2a407f0](2a407f0)) * **medusa-plugin-sentry:** Add service unit tests and update readme ([05b3e94](05b3e94)) * **medusa-plugin-sentry:** Finalise sentry plugin + add support for web hooks + Add API end points to fetch transactrions and transaction events paginated" ([204cc69](204cc69)) * **medusa-plugin-sentry:** Fix package.json ([09664c7](09664c7)) * **medusa-plugin-sentry:** Fix package.json ([8b94959](8b94959)) * **medusa-plugin-sentry:** Update readme ([1d424cf](1d424cf)) * **medusa-plugin-sentry:** Update readme ([385dd36](385dd36)) * **medusa-plugins:** fix package.json ([36424b4](36424b4)) * **medusa/payement-paytr:** Improve tests and logic ([59cea45](59cea45)) * **medusa/payment-paytr:** Finalize paytr payment ([e6c9af9](e6c9af9)) * **medusa/payment-paytr:** Update web hook ([4bafed9](4bafed9)) * **payment-paytr:** Implement payment provider ([#1](#1)) ([cb419ca](cb419ca)) * Re organise packages ([0eecc66](0eecc66)) * Support for medusa latest storefron ([3a44446](3a44446)) * supposrt facebook authentication strategy ([adrien2p#19](https://github.com/xponential-asia/medusa-xpo-auth-plugins/issues/19)) ([1dc7582](1dc7582)) * top level domain set in the cookie ([adrien2p#166](https://github.com/xponential-asia/medusa-xpo-auth-plugins/issues/166)) ([c226927](c226927)) * Ui component and routing for medusa admin sentry ([adrien2p#14](https://github.com/xponential-asia/medusa-xpo-auth-plugins/issues/14)) ([e04071d](e04071d)) * update strategies according to the latest auth changes from medusa core ([adrien2p#26](https://github.com/xponential-asia/medusa-xpo-auth-plugins/issues/26)) ([9afc2fd](9afc2fd))
The following PR enables authentication via Auth0.
Required:
auth0Domain
in the MedusaConfig.