sergiodxa / remix-auth-oauth2

A OAuth2Strategy for Remix Auth
https://sergiodxa.github.io/remix-auth-oauth2/
MIT License
150 stars 56 forks source link

Support PKCE flow #52

Closed TheFacto closed 3 months ago

TheFacto commented 1 year ago

👋 First time contributor.

I'm putting this out here as Draft as it's currently an idea, and I want to make sure it's something you'd like before I take the idea further and flesh it out with tests, etc.

A little background: https://developer.okta.com/blog/2019/08/22/okta-authjs-pkce#use-pkce-to-make-your-apps-more-secure

svengau commented 1 year ago

Also, could you update the README file ? thx

maurerbot commented 1 year ago

why not make crypto a dependency injection?

sergiodxa commented 1 year ago

why not make crypto a dependency injection?

While it will be easier for the package to do that, it means the dev using the strategy could provide functions that don't encrypt anything risking the security of their apps.

It's better if the strategy owns that so it can ensure it's safe, also it will be a breaking change for every user of the strategy and any of the substrategies.

maurerbot commented 1 year ago

I can get behind that reasoning. Just thinking of ways to move this faster to done.

piotrkulpinski commented 1 year ago

Hi guys, any progress on this? Is there any way to install the package from this specific branch?

sergiodxa commented 1 year ago

Hi guys, any progress on this? Is there any way to install the package from this specific branch?

@piotrkulpinski I've been working on a package to use OIDC with Web APIs, and I used it to create a OIDCStrategy for Remix Auth, while it doesn't support everything from OpenID Connect yet it does support PKCE.

Note that the package is still on v0 as I'm testing it myself to find any bug and check the DX of using it.

piotrkulpinski commented 1 year ago

@sergiodxa That's great, thanks for doing that!

Unfortunately, in my case, this won't help as I'm trying to connect to Airtable which I believe doesn't support OIDC yet.

sergiodxa commented 1 year ago

The current strategy supports OAuth2 with PKCE too, OIDC works like OAuth2 with a few extra things, in your case instead of using the discovery feature of web-OIDC you can pass the issuer endpoints directly to the strategy.

piotrkulpinski commented 1 year ago

@sergiodxa Is there any example of using the package this way? I'm very new to the OIDC standard and couldn't find anything in the docs.

sergiodxa commented 1 year ago

@piotrkulpinski you can do this:

import { OIDCStrategy } from "web-oidc/remix";

authenticator.use(
  new OIDCStrategy(
    {
      issuer: {
        issuer: "https://auth.company.tld",
        authorization_endpoint: "https://auth.company.tld/authorize",
        token_endpoint: "https://auth.company.tld/oauth/token",
        userinfo_endpoint: "https://auth.company.tld/userinfo",
      },
      client_id: "CLIENT_ID",
      client_secret: "CLIENT_SECRET",
      redirect_uri: "https://www.company.tld/auth/callback",
      response_type: "code id_token",
    },
    async ({ profile, tokens, issuer, client }) => {
      return { profile, tokens };
    }
  )
);

You will need to replace the issuer, authorization_endpoint, token_endpoint and userinfo_endpoint with the values from Airtable. This is the minimum you need right now to use the strategy.

TheFacto commented 1 year ago

@sergiodxa given your comments about using OIDC, do you feel we should pursue PKCE in remix-auth-oauth2? I'm still happy to pick this back up and utilize crypto-js to avoid the cloudfare conundrum you mentioned previously.

And apologies for being unresponsive for a bit, been really busy.

sergiodxa commented 1 year ago

@TheFacto if you want to work on it I will definitely merge it, PKCE doesn't work on all OAuth2 providers since it's a OIDC thing but it's ignored by the ones that doesn't support it so it doesn't break anything.

Alexandre-Herve commented 9 months ago

Hey there any update on this? I'm implementing a remix-auth-lichess strategy which requires PKCE. Maybe I can help with something?

TheFacto commented 8 months ago

@Alexandre-Herve I'm not sure the status here. I had pushed a few commits/tests and flagged as ready for review. I didn't want to pressure any maintainers to respect their time. Since then, I've picked up a few merge conflicts - I'm happy to get those resolved if there's still any interest to actually get this included.

sergiodxa commented 3 months ago

Support for this was implemented in #89