feat(medusa-plugin-auth): added oauth2 as login strategy

[stephane-segning]

No description provided.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
medusa-plugins	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 24, 2023 8:29pm

[adrien2p]

I am thinking of something, since the purpose of the OAuth2 strategy would be to allow the users to use it for multiple purposes/plateform I would suggest that instead of directly creating a class that extends the PassportStrategy, we should probably create a function that return a class that extends passport strategy with a custom named strategy. That way we can have multiple instanced of it. In that case, we also need to allow an array of configuration for this strategy in tthe plugin config. wdyt?

it would look like this:

export function getOAuth2AdminStrategy(name: string) {
  return class OAuth2AdminStrategy extends PassportStrategy(OAuth2Strategy, `${OAUTH2_ADMIN_STRATEGY_NAME}_${name}`) {
    // ...
  }
}

// packages/medusa-plugin-auth/src/auth-strategies/oauth2/index.ts
export default {
  load: (container: MedusaContainer, configModule: ConfigModule, options: AuthOptions): void => {
    if (options.oauth2?.admin) {
      const strategyOptions = Array.isArray(options.oauth2.admin) ? options.oauth2.admin : [options.oauth2.admin];
      for (const strategyOption of strategyOptions) {
        const OAuth2AdminStrategy = getOAuth2AdminStrategy(strategyOption.name);
        new OAuth2AdminStrategy(container, configModule, strategyOption, options.strict);
      }
    }
  }
}

and so one. Let me know if thats no clear :)

[stephane-segning]

Sure! I totally agree. To be honest, I wanted to do it that way, but I changed my mind, telling myself that wasn't the point of the ticket. But as you point out, I agree.
I suggest we finish this ticket first, then adapt the project with a dynamic list of providers. You know, so that we don't get mixed up...

[adrien2p]

In that case ill do a final review and merge but not released to not have a breaking changes api in btween

[stephane-segning]

Yep yep. I'll stat thinking about how to refactor the code for that and check for the steam integration too

[adrien2p]

LGTM

[adrien2p]

I will merge this one today, in the followup pr would you be up to add a doc page for oauth2 similar to the other providers :)

[stephane-segning]

Sure @adrien2p! But let's add documentation after the change of the structure. How do you think this?

[stephane-segning]

I wrote the doc in the #121

[adrien2p]

I think that in terms of changes, most of it is part of my comment above with the code snippet. For the configuration, I think it can just be an array of OAuth2 config + a name for the OAuth strategy config key. Also, for the end point, I believe the url needs to be changed to include the strategy name inside so that there is no conflict. Does it make sense? @stephane-segning

[stephane-segning]

For the configuration, an arraylike makes sense. Now, to allow customization, we can define each item either in the form of a function, maybe promise-like for people thinking about multi-tenancy, so that they resolve to the configuration or the configuration.

And as you said, each array item's subconfig can simply be

• a name of the (auth) provider
• an OAuth2 Config related to the provider
• an ID of the login type (default to the name of the provider)

For the URL, we can check just after plugin init if everything is ok and throw an error if there are conflicts. We don't need to change URLs for that. But we can build default URLs using the id of config.

How do you see this one @adrien2p ?

[adrien2p]

That sounds like a solid plan. I like it 💪