Use Arctic for OAuth providers #1851

infomiho · 2024-03-04T17:13:31Z

Status

Needs to be implemented

Improvements

The client page now receives a one-time-code and exchanges it for a session
- Ideally, we don't redirect to "on login failed" path, but show a message Auth UI and OAuth: show auth errors on screen #1817
Move some of the paths (login path, oauth callback paths) in Haskell
createToken and verifyToken functions need to be refactored (tech debt from old auth)
Replaces jsonwebtoken with oslo/jwt #1852
DRY up the code for google.ts and github.ts (to reduce the effort of adding next providers)

Changes for user

They need to change the redirect URL from client to server since we now do the OAuth redirect from backend to Google to backend (⚠️ breaking change)

Open questions

The property in the user config (when users use configFn property in Wasp file) to set scopes changed from scope to scopes (⚠️ breaking change) -> we can maybe make it backwards compatible?
Passport strategies that we used mapped the Google data into a specific shape -> do we want to do it as well to be backwards compatible? (Same things goes for Github)

Closes #1817

infomiho · 2024-03-05T10:22:31Z

waspc/data/Generator/templates/server/src/auth/providers/config/github.ts

+            try {
+                const { accessToken } = await github.validateAuthorizationCode(code);
+
+                // TODO: maybe an additional request to get the user's email?


The passport strategy did it by default if the scopes included user or user:email https://github.com/cfsghost/passport-github/blob/master/lib/strategy.js#L119

What are the options? Either we get it for them, or they get it on their own.

Maybe let's do the same as Passport strategy did, it sounds useful in practice, and will be a less of a breaking change.

But if you feel that is too much, we can also skip it.

Okay, this comment got outdated with latest changes. I replicated the logic that Passport had to keep the same information available. I think that makes the most sense!

infomiho · 2024-03-07T18:09:24Z

waspc/data/Generator/templates/react-app/src/auth/pages/OAuthCallback.tsx

+    return <Redirect to="{= onAuthSucceededRedirectTo =}" />;
+  }
+
+  /* TODO: Decide if we want to redirect to a page that shows the error message */


I vote that we show the user an error instead of redirecting to "onAuthFailedRedirectTo" or if we were to redirect to "onAuthFailedRedirectTo" we attach an error message to the redirect.

yeah that sounds about right! I guess "onAuthFailedRedirectTo" serves multiple things -> where to send user if they try to open a page that needs auth but they are not authenticated, but then also where to send user on failed login. First scenario is ok, but we don't want to use it upon failed login / signup -> better to just show an error message.

better to just show an error message.

I agree. Let's do that.

I don't think failed logins and signups do any redirecting as it currently stands. Or do they?

The OAuth login and signup, do in fact redirect. We are taking about this line: https://github.com/wasp-lang/wasp/blob/main/waspc/data/Generator/templates/react-app/src/auth/pages/OAuthCodeExchange.jsx#L58

waspc/data/Generator/templates/react-app/src/router.tsx

infomiho · 2024-03-07T18:09:47Z

waspc/data/Generator/templates/sdk/wasp/auth/providers/types.ts

@@ -16,19 +16,11 @@ export type ProviderConfig = {
    // Unique provider identifier, used as part of URL paths
    id: ProviderName;
    displayName: string;
-    // Each provider config can have an init method which is ran on setup time
-    // e.g. for oAuth providers this is the time when the Passport strategy is registered.
-    init?(provider: ProviderConfig): Promise<InitData>;


Not needed anymore

infomiho · 2024-03-07T18:12:18Z

waspc/data/Generator/templates/server/src/auth/providers/config/github.ts

+            env.GITHUB_CLIENT_SECRET,
+        );
+
+        router.get('/login', async (_req, res) => {


Hard coded path, ideally we would move it to Haskell

infomiho · 2024-03-07T19:01:20Z

waspc/data/Generator/templates/server/src/auth/providers/oauth/user.ts

+
+// We need a user id to create the auth token, so we either find an existing user
+// or create a new one if none exists for this provider.
+async function getAuthIdFromProviderDetails(


Existing function

infomiho · 2024-03-07T19:02:17Z

waspc/examples/todoApp/src/auth/github.js

@@ -1,9 +1,7 @@
 export function config() {
  console.log('Inside user-supplied GitHub config')
  return {
-    clientID: process.env['GITHUB_CLIENT_ID'],


No need for client ID or client secret in the user config any more. Although, I believe that wasn't needed for some time since we merged our default options and their options.

infomiho · 2024-03-07T19:07:14Z

waspc/src/Wasp/Generator/ServerGenerator/Auth/OAuthAuthG.hs

@@ -119,28 +118,8 @@ genOAuthConfig provider maybeUserConfig pathToConfigDst = return $ C.mkTmplFdWit
    relPathFromAuthConfigToServerSrcDir :: Path Posix (Rel importLocation) (Dir C.ServerSrcDir)
    relPathFromAuthConfigToServerSrcDir = [reldirP|../../../|]

-getJsonForOAuthConfigProps :: OAuthAuthProvider -> [Aeson.Value]


Moved the env variables knowledge in the template

How come you decided to do this?

I believe we prematurely abstracted stuff here. This code assumes that there are only client ID and client secret env vars, but in reality, there can be 2, 3 or 4 even env vars needed to set up a provider. We don't use the knowledge about the env vars in Haskell so it wasn't really useful.

Also, I wanted to keep things as simple as possible to make the rewrite easier :)

infomiho · 2024-03-07T19:08:07Z

waspc/src/Wasp/Generator/ServerGenerator/Auth/OAuthAuthG.hs

  Generator FileDraft
-genOAuthConfig provider maybeUserConfig pathToConfigDst = return $ C.mkTmplFdWithDstAndData tmplFile dstFile (Just tmplData)
+genOAuthConfig provider maybeUserConfig pathToConfigTmpl = return $ C.mkTmplFdWithData tmplFile (Just tmplData)


We used _oauth.ts as a common template before, but now each provider has their own template file because their code now differs (the way we fetch the user info, some providers require extra steps, some not)

Understood!

Still, the new provider files have a lot in common. There are differences, sure, but there's definitely repetition. Could we extract the similar parts into functions and maximize the signal-to-noise ratio in the createRouter functions.

This will probably make adding new providers simpler. Still, I haven't studied the similarities and differences between the provider files in extreme detail, you'll be a better judge of whether such an effort makes sense or not.

I've given it a shot to minimise the repeated code. We could probably go further, but I didn't to be too smart about inventing new abstractions, this now feels okay, check it out and tell me what do you think. Should we go further?

I think that's enough abstraction for now. We'll come back to it when we add more providers and are smarter.

Martinsos

Nice job @infomiho , I like the analytical and detailed approach!

I answered some top level stuff, and will leave the details to @sodic . Let me know if I missed something that you would like to get my opinion on.

scope -> scopes: why did that change? Because underlying thing names it that way? What is better, sounds like there is some semantic involved with those names? Is it actually scope or scopes, what makes more sense? I wouldn't try to avoid breaking change, I would instead go for the name that we think works the best, be it a breaking change or not, doesn't matter.

Passport strategies mapped the provider data into a specific shape -> how breaking is it? Will they need to migrate the database, because we stored that stuff in the DB? If so, then maybe it might make sense to do that mapping. If we are doing the mapping, is it complex, does it feel silly, or does it make sense, do we lose some info, ... -> how do we feel about it?
If there is no DB migration involved, then I would certainly not bother with any mapping (unless you think they way data is returned by Artcic is ugly and could benefit from mapping).

sodic · 2024-03-11T15:57:39Z

The property in the user config (when users use configFn property in Wasp file) to set scopes changed from scope to scopes (⚠️ breaking change) -> we can maybe make it backwards compatible?

I don't think we should focus on the breaking change part - we're not stable enough for such sacrifices in consistency (we shouldn't support both names).. Pick the more appropriate name, regardless of whether it causes a breaking change or not. What we can do is throw a nice error. Instead of "Unknown property: scope". We can say "Please change 'scope' to 'scopes' in this file".

Passport strategies that we used mapped the Google data into a specific shape -> do we want to do it as well to be backwards compatible? (Same things goes for Github)

I think the same principle applies here. Wasp is still immature enough to have the luxury of breaking stuff on account of creating something of high quality. Of course, change purely for the sake of change is also something we should avoid (especially when it impacts users). In short:

If the current format is a random thing people at passport) came up with and you see something obvious you want to change, I say "go for it".
If the current format is a well-documented or standard way of storing this particular provider's data (which could be the case if everyone uses passport), I say we should leave it as is.

By the way, can we create migration scripts for these things?

sodic

A couple of old comments

waspc/examples/todoApp/src/auth/github.js

waspc/examples/todoApp/src/user.ts

waspc/data/Generator/templates/react-app/src/router.tsx

waspc/data/Generator/templates/react-app/src/auth/pages/OAuthCallback.tsx

sodic · 2024-03-08T16:43:39Z

waspc/data/Generator/templates/react-app/src/auth/pages/OAuthCallback.tsx

+    return <Redirect to="{= onAuthSucceededRedirectTo =}" />;
+  }
+
+  /* TODO: Decide if we want to redirect to a page that shows the error message */


I don't think failed logins and signups do any redirecting as it currently stands. Or do they?

Martinsos · 2024-03-12T10:18:14Z

Nice job @infomiho , I like the analytical and detailed approach!

I answered some top level stuff, and will leave the details to @sodic . Let me know if I missed something that you would like to get my opinion on.

scope -> scopes: why did that change? Because underlying thing names it that way? What is better, sounds like there is some semantic involved with those names? Is it actually scope or scopes, what makes more sense? I wouldn't try to avoid breaking change, I would instead go for the name that we think works the best, be it a breaking change or not, doesn't matter.

Passport strategies mapped the provider data into a specific shape -> how breaking is it? Will they need to migrate the database, because we stored that stuff in the DB? If so, then maybe it might make sense to do that mapping. If we are doing the mapping, is it complex, does it feel silly, or does it make sense, do we lose some info, ... -> how do we feel about it? If there is no DB migration involved, then I would certainly not bother with any mapping (unless you think they way data is returned by Artcic is ugly and could benefit from mapping).

@infomiho just a reminder not to miss this comment.

infomiho · 2024-03-12T10:20:40Z

Of course! I've been waiting to get input from both Filip and you and I'll give some sort of a conclusion after we agree on what is the best here 👍

infomiho · 2024-03-13T20:58:43Z

@Martinsos

scope -> scopes: why did that change? Because underlying thing names it that way? What is better, sounds like there is some semantic involved with those names? Is it actually scope or scopes, what makes more sense? I wouldn't try to avoid breaking change, I would instead go for the name that we think works the best, be it a breaking change or not, doesn't matter.

We just passed through the options to Passport before. Passport used scope, and that we pass through the options to Arctic, Arctic uses scopes. If we don't care about the backwards compatibility, I think it's okay to change the naming.

IMHO We'll need good migration docs from 0.12 to 0.13 (Maybe even a script is possible?)

Passport strategies mapped the provider data into a specific shape -> how breaking is it? Will they need to migrate the database, because we stored that stuff in the DB? If so, then maybe it might make sense to do that mapping. If we are doing the mapping, is it complex, does it feel silly, or does it make sense, do we lose some info, ... -> how do we feel about it?
If there is no DB migration involved, then I would certainly not bother with any mapping (unless you think they way data is returned by Artcic is ugly and could benefit from mapping).

No database migrations will be necessary. What changes: the shape of the data they receive in userSignupFields handlers. So, if they used data.profile.emails[0] they'll need to use ... data.profile.email or smth along those lines to get the email.
Passport has their own format they tried to keep across their strategies: https://www.passportjs.org/reference/normalized-profile/ We could try to replicate their approach. Alternatively, we can just return what we receive from the provider. This will make our maintenance easier since if something changes, we are just passing it along anyways and no changes from our side our needed.

@sodic

I don't think we should focus on the breaking change part - we're not stable enough for such sacrifices in consistency (we shouldn't support both names).. Pick the more appropriate name, regardless of whether it causes a breaking change or not.

Makes sense 👍

What we can do is throw a nice error. Instead of "Unknown property: scope". We can say "Please change 'scope' to 'scopes' in this file".

This could be a nice little touch. I've done stuff like this e.g. "Hey this error might mean you didn't run migrations" and it seemed to help the user.

If the current format is a random thing people at passport) came up with and you see something obvious you want to change, I say "go for it".

As I said above, the format is something Passport invented and tried to stick to. But they are not the "one true solution" for OAuth, so I'd say, maybe stick to just passing through the info we get from providers. At least that way, it matches what the providers' might have in their docs?

By the way, can we create migration scripts for these things?

+1 from me. I'd like to have at least good 0.12 to 0.13 migration docs.

Martinsos · 2024-03-14T17:16:15Z

@Martinsos

scope -> scopes: why did that change? Because underlying thing names it that way? What is better, sounds like there is some semantic involved with those names? Is it actually scope or scopes, what makes more sense? I wouldn't try to avoid breaking change, I would instead go for the name that we think works the best, be it a breaking change or not, doesn't matter.

We just passed through the options to Passport before. Passport used scope, and that we pass through the options to Arctic, Arctic uses scopes. If we don't care about the backwards compatibility, I think it's okay to change the naming.

IMHO We'll need good migration docs from 0.12 to 0.13 (Maybe even a script is possible?)

Passport strategies mapped the provider data into a specific shape -> how breaking is it? Will they need to migrate the database, because we stored that stuff in the DB? If so, then maybe it might make sense to do that mapping. If we are doing the mapping, is it complex, does it feel silly, or does it make sense, do we lose some info, ... -> how do we feel about it?
If there is no DB migration involved, then I would certainly not bother with any mapping (unless you think they way data is returned by Artcic is ugly and could benefit from mapping).

No database migrations will be necessary. What changes: the shape of the data they receive in userSignupFields handlers. So, if they used data.profile.emails[0] they'll need to use ... data.profile.email or smth along those lines to get the email.

Passport has their own format they tried to keep across their strategies: https://www.passportjs.org/reference/normalized-profile/ We could try to replicate their approach. Alternatively, we can just return what we receive from the provider. This will make our maintenance easier since if something changes, we are just passing it along anyways and no changes from our side our needed.

@sodic

I don't think we should focus on the breaking change part - we're not stable enough for such sacrifices in consistency (we shouldn't support both names).. Pick the more appropriate name, regardless of whether it causes a breaking change or not.

Makes sense 👍

What we can do is throw a nice error. Instead of "Unknown property: scope". We can say "Please change 'scope' to 'scopes' in this file".

This could be a nice little touch. I've done stuff like this e.g. "Hey this error might mean you didn't run migrations" and it seemed to help the user.

If the current format is a random thing people at passport) came up with and you see something obvious you want to change, I say "go for it".

As I said above, the format is something Passport invented and tried to stick to. But they are not the "one true solution" for OAuth, so I'd say, maybe stick to just passing through the info we get from providers. At least that way, it matches what the providers' might have in their docs?

By the way, can we create migration scripts for these things?

+1 from me. I'd like to have at least good 0.12 to 0.13 migration docs.

I would then say let's do what is easy for us and also gives us nice DX. Wasp is in Beta, and it is ok we are changing fast.
I wouldn't do a migration script, because we can have instructions and they are not complex, they can be done manually. This will not be such a big release as 0.12.
Also, I wouldn't throw an error if they passed scope -> that is something we have to maintain then, we are not yet 1.0 and no need for this. Let's just mention it in the Changelog under migration instructions and that is ok.

sodic

Left improvement suggestions, but no major problems.

Here's the only major thing I'd like us to discuss: #1851 (comment)

Nice work!

waspc/data/Generator/templates/react-app/src/auth/pages/OAuthCallback.tsx

waspc/data/Generator/templates/server/src/auth/providers/oauth/env.ts

waspc/data/Generator/templates/server/src/auth/providers/oauth/oneTimeCode.ts

waspc/data/Generator/templates/server/src/auth/providers/oauth/redirect.ts

waspc/data/Generator/templates/server/src/auth/providers/oauth/user.ts

sodic · 2024-03-18T07:56:09Z

waspc/data/Generator/templates/react-app/src/auth/pages/OAuthCallback.tsx

+export function OAuthCallbackPage() {
+  const { isLoading, error, user } = useOAuthCallbackHandler();
+
+  if (user !== undefined && user !== null) {


How come it can be both of these? I'm guessing because undefined happens on the first render, while null happens when the user isn't logged in?

If so, there's a trick that many linters recommend (although I'm not sure I like it). It sits halfway between being fully explicit and using coercion:

if (user != null) { // ... }

This covers both undefined and null.

I'd like to skip the non strict check if possible. I know this looks ugly e.g. checking both undefined and null but it's at least not relying on implicit JS behaviour.

waspc/data/Generator/templates/server/src/auth/providers/oauth/user.ts

sodic · 2024-03-18T08:07:03Z

waspc/data/Generator/templates/server/src/auth/providers/oauth/oneTimeCode.ts

+      const { id: authId } = await tokenStore.verifyToken(code);
+      const auth = await findAuthWithUserBy({ id: authId })
+
+      if (!auth) {


Explicit check (if you want)

sodic · 2024-03-18T08:18:07Z

waspc/data/Generator/templates/server/src/auth/providers/oauth/oneTimeCode.ts

+  function verifyToken(token: string) {
+    return validateJWT<{ id: string }>(token);
+  }


Why not:

function getVerifiedToken(token: string) Promise<id: string> { return validateJWT(token); }

It's a part of the public API through the returned object, so it makes sense to type it explicitly. Also, I'd say it's always better to type function signatures and leave the type inference for the body than the other way around.

sodic · 2024-03-18T08:22:17Z

waspc/data/Generator/templates/server/src/auth/providers/oauth/state.ts

+  const result = {} as { [name in StateType]: string }
+
+  if (stateTypes.includes('state' as ST)) {
+    const state = generateState();
+    setOAuthCookieValue(provider, res, 'state', state);
+    result.state = state;
+  }
+
+  if (stateTypes.includes('codeVerifier' as ST)) {
+    const codeVerifier = generateCodeVerifier();
+    setOAuthCookieValue(provider, res, 'codeVerifier', codeVerifier);
+    result.codeVerifier = codeVerifier;
+  }
+
+  return result as { [name in ST]: string };


A less error prone way to implement this is by generating the resulting state first, and then using that to set the value. In pseudocode:

// construct the result setOAuthCookieValue(provider, res, result); // this function would iterate the object and set all cookies return result

I probably wouldn't do this now though and would just merge this. Not a big deal.

sodic · 2024-03-18T08:23:19Z

waspc/data/Generator/templates/server/src/auth/providers/oauth/state.ts

+  }
+  result.code = code;
+
+  return result as { [name in ST]: string } & { code: string };


I don't think you need this cast as the function is currently written.

sodic · 2024-03-18T08:25:14Z

waspc/data/Generator/templates/server/src/auth/providers/oauth/user.ts

+
+  const authId = await getAuthIdFromProviderDetails(providerId, providerProfile, userSignupFields);
+
+  if (!authId) {


Explicit check if you want

This function always returns a value or it throws an error, so I've removed this check.

sodic · 2024-03-18T08:25:47Z

waspc/src/Wasp/Generator/ServerGenerator/Auth/OAuthAuthG.hs

  Generator FileDraft
-genOAuthConfig provider maybeUserConfig pathToConfigDst = return $ C.mkTmplFdWithDstAndData tmplFile dstFile (Just tmplData)
+genOAuthConfig provider maybeUserConfig pathToConfigTmpl = return $ C.mkTmplFdWithData tmplFile (Just tmplData)


I think that's enough abstraction for now. We'll come back to it when we add more providers and are smarter.

Signed-off-by: Mihovil Ilakovac <mihovil@ilakovac.com>

infomiho commented Mar 5, 2024

View reviewed changes

infomiho changed the base branch from main to arctic-feature March 5, 2024 17:55

infomiho force-pushed the miho-arctic-integration branch from f714353 to 45614f2 Compare March 5, 2024 18:19

sodic mentioned this pull request Mar 5, 2024

Auth UI and OAuth: show auth errors on screen #1817

Closed

infomiho commented Mar 7, 2024

View reviewed changes

waspc/data/Generator/templates/react-app/src/router.tsx Outdated Show resolved Hide resolved

infomiho commented Mar 7, 2024

View reviewed changes

infomiho changed the title ~~Arctic integration (WIP)~~ Use Arctic for OAuth providers Mar 7, 2024

infomiho mentioned this pull request Mar 7, 2024

User defined imports name mangling #1865

Open

infomiho marked this pull request as ready for review March 7, 2024 19:33

Martinsos reviewed Mar 8, 2024

View reviewed changes

infomiho force-pushed the miho-arctic-integration branch from 1965d1b to da2cd6e Compare March 8, 2024 19:00

infomiho mentioned this pull request Mar 10, 2024

Use Arctic for OAuth providers #1869

Closed

sodic reviewed Mar 11, 2024

View reviewed changes

infomiho force-pushed the miho-arctic-integration branch from 1cbb6dd to fdf33df Compare March 13, 2024 08:26

infomiho added the auth label Mar 14, 2024

sodic reviewed Mar 15, 2024

View reviewed changes

sodic mentioned this pull request Mar 15, 2024

Adds Keycloak OAuth provider #1876

Merged

2 tasks

sodic reviewed Mar 18, 2024

View reviewed changes

sodic approved these changes Mar 18, 2024

View reviewed changes

Replaces Passport with Arctic for OAuth

13b0c5c

Signed-off-by: Mihovil Ilakovac <mihovil@ilakovac.com>

infomiho force-pushed the miho-arctic-integration branch from 5813990 to 13b0c5c Compare March 18, 2024 11:32

infomiho merged commit a45b7a9 into arctic-feature Mar 18, 2024
6 checks passed

infomiho deleted the miho-arctic-integration branch March 18, 2024 11:33

infomiho added a commit that referenced this pull request Mar 18, 2024

Replaces Passport with Arctic for OAuth (#1851)

3d5ed1a

sodic mentioned this pull request Aug 23, 2024

Refresh token impl #2212

Merged

2 tasks


		const authId = await getAuthIdFromProviderDetails(providerId, providerProfile, userSignupFields);

		if (!authId) {

Use Arctic for OAuth providers #1851

Use Arctic for OAuth providers #1851

Conversation

infomiho commented Mar 4, 2024 • edited Loading

Status

Choose a reason for hiding this comment

Martinsos Mar 8, 2024 • edited Loading

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

infomiho Mar 8, 2024 • edited Loading

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

infomiho Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infomiho Mar 7, 2024 • edited Loading

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

Choose a reason for hiding this comment

Martinsos left a comment

Choose a reason for hiding this comment

sodic commented Mar 11, 2024 • edited Loading

sodic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martinsos commented Mar 12, 2024

infomiho commented Mar 12, 2024

infomiho commented Mar 13, 2024

Martinsos commented Mar 14, 2024

sodic left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infomiho commented Mar 4, 2024 •

edited

Loading

Martinsos Mar 8, 2024 •

edited

Loading

infomiho Mar 8, 2024 •

edited

Loading

infomiho Mar 7, 2024 •

edited

Loading

infomiho Mar 7, 2024 •

edited

Loading

sodic commented Mar 11, 2024 •

edited

Loading

sodic left a comment •

edited

Loading