remix-auth-oauth2 icon indicating copy to clipboard operation
remix-auth-oauth2 copied to clipboard

Support PKCE flow

Open TheFacto opened this issue 2 years ago • 14 comments
trafficstars

👋 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

TheFacto avatar Apr 28 '23 16:04 TheFacto

Also, could you update the README file ? thx

svengau avatar May 24 '23 06:05 svengau

why not make crypto a dependency injection?

4x4notfound avatar Jun 05 '23 19:06 4x4notfound

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.

sergiodxa avatar Jun 05 '23 21:06 sergiodxa

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

4x4notfound avatar Jun 06 '23 13:06 4x4notfound

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

piotrkulpinski avatar Aug 07 '23 21:08 piotrkulpinski

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.

sergiodxa avatar Aug 07 '23 23:08 sergiodxa

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

piotrkulpinski avatar Aug 08 '23 06:08 piotrkulpinski

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.

sergiodxa avatar Aug 08 '23 06:08 sergiodxa

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

piotrkulpinski avatar Aug 08 '23 07:08 piotrkulpinski

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

sergiodxa avatar Aug 08 '23 20:08 sergiodxa

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

TheFacto avatar Aug 11 '23 15:08 TheFacto

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

sergiodxa avatar Aug 11 '23 20:08 sergiodxa

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

Alexandre-Herve avatar Dec 08 '23 09:12 Alexandre-Herve

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

TheFacto avatar Jan 02 '24 16:01 TheFacto

Support for this was implemented in #89

sergiodxa avatar May 17 '24 23:05 sergiodxa