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(Auth0): Add Auth0 Authentication #27

Merged
merged 20 commits into from
Jan 16, 2023
Merged

Conversation

juanzgc
Copy link
Contributor

@juanzgc juanzgc commented Nov 24, 2022

The following PR enables authentication via Auth0.

Required: auth0Domain in the MedusaConfig.

@juanzgc
Copy link
Contributor Author

juanzgc commented Nov 29, 2022

@adrien2p Need to do some final testing and then will set the PR ready for review.

@adrien2p
Copy link
Owner

adrien2p commented Dec 1, 2022

Wonderful, could I get you to update the readme as well 😊

@juanzgc juanzgc marked this pull request as ready for review December 2, 2022 00:55
Copy link
Owner

@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 with few comments :)

@adrien2p
Copy link
Owner

adrien2p commented Dec 8, 2022

@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 ☺️

@mpoitevineau-millin
Copy link

@juanzgc you missed to add

in the api/index.ts line 9
import Auth0Startegy from '../auth-strategies/auth0';

in the api/index.ts line 23
routers.push(...Auth0Startegy.getRouter(configModule, options));

@juanzgc
Copy link
Contributor Author

juanzgc commented Dec 12, 2022

mpoitevineau-millin

@mpoitevineau-millin Do you mind giving it a test now?

@mpoitevineau-millin
Copy link

@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.
Will try to find a timeslot to write something

@juanzgc
Copy link
Contributor Author

juanzgc commented Dec 12, 2022

@mpoitevineau-millin Are you testing authentication with the admin or with store?

Do you mind testing both if you have time 😇

@adrien2p
Copy link
Owner

@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.

Will try to find a timeslot to write something

It would be wonderful if you find a way to make the doc more easier to understand 🥰

@mpoitevineau-millin
Copy link

@mpoitevineau-millin Are you testing authentication with the admin or with store?

Do you mind testing both if you have time 😇

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

@adrien2p
Copy link
Owner

@mpoitevineau-millin Are you testing authentication with the admin or with store?
Do you mind testing both if you have time 😇

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.

@adrien2p
Copy link
Owner

adrien2p commented Dec 13, 2022

guys. I ve created a pr where the token is stored on the medusa session. I think that since I ve updated the way the strategy are built, it is now working. Can I get you to double check? @juanzgc

Copy link
Owner

@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, few comments though. I did not repeated them everywhere ahah


const user = await userService.retrieveByEmail(email).catch(() => void 0);

if (user) {
Copy link
Owner

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?

Copy link
Contributor Author

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
Copy link
Owner

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

Copy link
Contributor Author

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
Copy link
Owner

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?

Copy link
Contributor Author

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?

@mpoitevineau-millin
Copy link

Full test on my side with the PR #32 . 100% ok

@mpoitevineau-millin
Copy link

@juanzgc do you know when you will be able to finish this PR?

@adrien2p
Copy link
Owner

@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

@juanzgc
Copy link
Contributor Author

juanzgc commented Jan 6, 2023

@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 😇

Copy link
Owner

@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.

Looks great 🎉

emails: [{ value: existsEmailWithMeta }],
};

const data = await auth0StoreStrategy.validate(req, accessToken, refreshToken, extraParams, profile);
Copy link
Owner

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

Copy link
Contributor Author

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

@Arsenalist
Copy link

@adrien2p in 9ead4c9#diff-2bd386bf42efcc87b13fc16c223b031bd923cd992e5a279976ceaf80c23d4ba0 we added this block to auth-routes-builder.ts

		passport.authenticate(GOOGLE_ADMIN_STRATEGY_NAME, {
			failureRedirect,
			session: false,
		}),

This is giving an error when testing this PR because passport is authenticating with the GOOGLE_ADMIN_STRATEGY_NAME instead of the auth0 one. Am I misreading this?

@adrien2p
Copy link
Owner

@Arsenalist this looks like a mistake. Will fix it tomorrow 🙏 thanks

@Arsenalist
Copy link

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:

                passportAuthenticateMiddleware: passport.authenticate(AUTH0_STORE_STRATEGY_NAME, {
                        scope: 'openid email profile',
                        session: false,
                        audience: "custom audience parameter" 
                }),

Many use cases for Auth0 require an audience parameter, so it might be a good idea to specify options as an object inside Auth0Options where the object is merged into the arguments for passport.authenticate. This would increase the flexibility and allow all the use cases for Auth0 to be available.

@juanzgc juanzgc requested a review from adrien2p January 13, 2023 22:21
@adrien2p
Copy link
Owner

@Arsenalist can i get you to give another test please, it should be good to go 🚀 thanks for the great works @juanzgc

@Arsenalist
Copy link

@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:

Error: Admin with email myemail@gmail.com already exists

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?

@juanzgc
Copy link
Contributor Author

juanzgc commented Jan 14, 2023

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.

@adrien2p
Copy link
Owner

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.

@Arsenalist
Copy link

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 access_token and id_token and in the current state this is not available to me unless I write a custom callback which I want to avoid. Is it possible to store this in the JWT here or is the recommended way to add another middleware to the route?

@Arsenalist
Copy link

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 access_token is not decodable to the client, and that contains all the necessary information needed to determine permissions etc.

@adrien2p
Copy link
Owner

adrien2p commented Jan 14, 2023

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 access_token and id_token and in the current state this is not available to me unless I write a custom callback which I want to avoid. Is it possible to store this in the JWT here or is the recommended way to add another middleware to the route?

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.

@adrien2p adrien2p merged commit 06c0c20 into adrien2p:main Jan 16, 2023
@adrien2p
Copy link
Owner

@Arsenalist here we go, #45 if you want to try it out 👍

@Arsenalist
Copy link

Ran a quick test. Worked well!

@eparizzi-clevertech
Copy link

@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".
We are in need of migrating and old e-commerce site which is using Auth0 for both customers and vendors/admins so we will need a way to distinguish them.

@Arsenalist
Copy link

@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". We are in need of migrating and old e-commerce site which is using Auth0 for both customers and vendors/admins so we will need a way to distinguish them.

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 access_token which contains role/permission information from Auth0. For my use case I just used Auth0 actions to inject role information into the user profile and then I read the user profile in my app to make decisions.

@Arsenalist
Copy link

Can this be packaged and released?

github-actions bot pushed a commit to xponential-asia/medusa-xpo-auth-plugins that referenced this pull request Dec 25, 2024
# 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))
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