feat(Auth0): Add Auth0 Authentication #27

juanzgc · 2022-11-24T19:18:56Z

The following PR enables authentication via Auth0.

Required: auth0Domain in the MedusaConfig.

juanzgc · 2022-11-29T07:30:47Z

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

packages/medusa-plugin-auth/src/auth-strategies/auth0/store.ts

adrien2p · 2022-12-01T18:06:36Z

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

adrien2p

LGTM with few comments :)

packages/medusa-plugin-auth/README.md

packages/medusa-plugin-auth/src/auth-strategies/auth0/admin.ts

adrien2p · 2022-12-08T13:30:54Z

@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

mpoitevineau-millin · 2022-12-11T09:56:19Z

@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 · 2022-12-12T05:47:08Z

mpoitevineau-millin

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

mpoitevineau-millin · 2022-12-12T13:12:17Z

@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 · 2022-12-12T14:40:47Z

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

Do you mind testing both if you have time 😇

adrien2p · 2022-12-12T16:55:41Z

@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 · 2022-12-13T16:31:56Z

@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 · 2022-12-13T17:02:22Z

@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 · 2022-12-13T18:00:40Z

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

adrien2p

LGTM, few comments though. I did not repeated them everywhere ahah

packages/medusa-plugin-auth/README.md

adrien2p · 2022-12-14T11:09:28Z

packages/medusa-plugin-auth/src/auth-strategies/auth0/admin.ts

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


I suppose that for the invitation check we are waiting the next refactoring which extract the validate method is that right?

I think the invitation check we should do in a separate PR

adrien2p · 2022-12-14T11:12:12Z

packages/medusa-plugin-auth/src/auth-strategies/auth0/store.ts

+				);
+			}
+
+			const customer = await customerService


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

I see you have refactored to only do retrieveRegisteredByEmail, should we still add the fallback?

adrien2p · 2022-12-14T11:13:40Z

packages/medusa-plugin-auth/src/auth-strategies/facebook/store.ts

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


We can maybe add a todo to remove it in the next major?

I thought we wanted to keep this moving forward to allow for backward compatibility with those currently using the authentication service?

mpoitevineau-millin · 2022-12-14T15:08:26Z

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

mpoitevineau-millin · 2022-12-21T16:25:02Z

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

adrien2p · 2022-12-22T19:12:50Z

@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 · 2023-01-06T04:48:08Z

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

adrien2p

Looks great 🎉

adrien2p · 2023-01-10T09:09:51Z

packages/medusa-plugin-auth/src/auth-strategies/auth0/__tests__/store/verify-callback.spec.ts

+			emails: [{ value: existsEmailWithMeta }],
+		};
+
+    const data = await auth0StoreStrategy.validate(req, accessToken, refreshToken, extraParams, profile);


todo: We should check that the update have been called here. also with the right input data

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

packages/medusa-plugin-auth/src/core/validate-callback.ts

Arsenalist · 2023-01-12T18:16:12Z

@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 · 2023-01-12T20:17:42Z

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

Arsenalist · 2023-01-13T04:00:47Z

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.

adrien2p · 2023-01-13T22:31:40Z

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

Arsenalist · 2023-01-14T02:30:26Z

@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 · 2023-01-14T02:33:47Z

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 · 2023-01-14T06:58:31Z

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 · 2023-01-14T19:45:18Z

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 · 2023-01-14T20:10:10Z

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 · 2023-01-14T20:22:22Z

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 · 2023-01-16T21:42:30Z

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

Arsenalist · 2023-01-16T22:18:17Z

Ran a quick test. Worked well!

eparizzi-clevertech · 2023-01-17T13:03:07Z

@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 · 2023-01-17T16:12:51Z

@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 · 2023-01-19T03:52:25Z

Can this be packaged and released?

juanzgc force-pushed the feat/auth0 branch from 72f81a5 to fb78b04 Compare November 29, 2022 05:49

juanzgc commented Nov 29, 2022

View reviewed changes

packages/medusa-plugin-auth/src/auth-strategies/auth0/store.ts Outdated Show resolved Hide resolved

juanzgc force-pushed the feat/auth0 branch from 73aea6d to ae22a9a Compare December 2, 2022 00:52

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

adrien2p requested changes Dec 2, 2022

View reviewed changes

packages/medusa-plugin-auth/README.md Show resolved Hide resolved

packages/medusa-plugin-auth/src/auth-strategies/auth0/admin.ts Show resolved Hide resolved

packages/medusa-plugin-auth/src/auth-strategies/auth0/admin.ts Outdated Show resolved Hide resolved

juanzgc force-pushed the feat/auth0 branch from 49d20f1 to 2b4088a Compare December 7, 2022 23:39

juanzgc commented Dec 11, 2022

View reviewed changes

...ages/medusa-plugin-auth/src/auth-strategies/facebook/__tests__/store/verify-callback.spec.ts Outdated Show resolved Hide resolved

adrien2p requested changes Dec 14, 2022

View reviewed changes

mpoitevineau-millin mentioned this pull request Dec 14, 2022

feat: Allow to access session from the plugin and remove cookie usage #32

Merged

juanzgc force-pushed the feat/auth0 branch from eb0ab48 to aa98474 Compare January 6, 2023 04:32

adrien2p requested changes Jan 10, 2023

View reviewed changes

adrien2p mentioned this pull request Jan 12, 2023

fix auth-routes-builder.ts using google specifics #43

Closed

juanzgc added 10 commits January 13, 2023 16:20

.

c3a32d3

Update Authentication Plugin tests

afedf0b

Update README

1456a28

Updated Tests and re-added Legacy authentication default

a2665af

.

eda4b3e

Add API Route and Further testing

d86c3c4

Update ReadMe

d7c1fd3

.

ff7e204

Update tests

1f0d530

.

400e952

juanzgc force-pushed the feat/auth0 branch from 1863274 to 400e952 Compare January 13, 2023 21:25

juanzgc mentioned this pull request Jan 13, 2023

fix: Auth route builder #44

Merged

juanzgc added 3 commits January 13, 2023 16:59

.

f2090e3

.

10a1902

.

fca1e20

juanzgc requested a review from adrien2p January 13, 2023 22:21

.

cc46004

adrien2p merged commit 06c0c20 into adrien2p:main Jan 16, 2023


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

		if (user) {

feat(Auth0): Add Auth0 Authentication #27

feat(Auth0): Add Auth0 Authentication #27

Conversation

juanzgc commented Nov 24, 2022 • edited

juanzgc commented Nov 29, 2022

adrien2p commented Dec 1, 2022

adrien2p left a comment

Choose a reason for hiding this comment

adrien2p commented Dec 8, 2022

mpoitevineau-millin commented Dec 11, 2022

juanzgc commented Dec 12, 2022

mpoitevineau-millin commented Dec 12, 2022

juanzgc commented Dec 12, 2022

adrien2p commented Dec 12, 2022

mpoitevineau-millin commented Dec 13, 2022

adrien2p commented Dec 13, 2022

adrien2p commented Dec 13, 2022 • edited

adrien2p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpoitevineau-millin commented Dec 14, 2022

mpoitevineau-millin commented Dec 21, 2022

adrien2p commented Dec 22, 2022

juanzgc commented Jan 6, 2023

adrien2p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arsenalist commented Jan 12, 2023

adrien2p commented Jan 12, 2023

Arsenalist commented Jan 13, 2023

adrien2p commented Jan 13, 2023

Arsenalist commented Jan 14, 2023

juanzgc commented Jan 14, 2023

adrien2p commented Jan 14, 2023

Arsenalist commented Jan 14, 2023

Arsenalist commented Jan 14, 2023

adrien2p commented Jan 14, 2023 • edited

adrien2p commented Jan 16, 2023

Arsenalist commented Jan 16, 2023

eparizzi-clevertech commented Jan 17, 2023

Arsenalist commented Jan 17, 2023

Arsenalist commented Jan 19, 2023

juanzgc commented Nov 24, 2022 •

edited

adrien2p commented Dec 13, 2022 •

edited

adrien2p commented Jan 14, 2023 •

edited