Refresh token impl #2212

infomiho · 2024-07-30T07:02:15Z

Closes #2124

What was done

Moved init of OAuth clients (google, github, discord and keycloak) to the SDK so users can also import them and use them
- These clients are needed to refresh auth tokens
Move some of the OAuth helpers to SDK - because they are only used in the SDK or could be useful for users
- The env helpers are only used in the SDK
- The redirect and one-time token helpers can be useful if users implement some OAuth client themselves
Updated the OAuth tokens that we provide in auth hooks: we now provide all the data we get and not just the access token
- We forward the data we receive so users will get the refresh token if we receive it

Left to do

Update docs
Fix provider name types

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

sodic

Nice work, just took a look at the docs for now to familiarize myself with the feature.

I'm now going for the code.

web/docs/auth/auth-hooks.md

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

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

sodic

Nice work!

Left some suggestions, don't think I found anything major.

Please remember to change the public API table if you're changing the new API 😬

waspc/data/Generator/templates/sdk/wasp/package.json

waspc/data/Generator/templates/sdk/wasp/server/auth/hooks.ts

sodic · 2024-08-21T09:55:17Z

waspc/data/Generator/templates/sdk/wasp/server/auth/hooks.ts

  req: ExpressRequest
 } & InternalAuthHookParams
+
+// PRIVATE API (server)


Should be public though, right? According to the "public types" rule from Effective TypeScript.

Didn't we want to hide parts in the top level exports so the users don't get autocomplete suggestions that are not really used by them? Based on that I put "private API" since ideally, users wouldn't need to see this when using the SDK.

Oh, you're refering to what we did with OnBeforeSignupHookParams and friends?

Ok, if you think this is one of those cases, we can keep it private, just add a note like we did there.

FWIW, I'm not sure this is one of thouse cases though.

The fact that we export many symbols isn't a problem by iteself - users will type up what they need and choose a quick action to import it. It only becomes a problem when the IDE suggests a niche export before a common one.

In the hooks params case, the parameter types had the same prefix as the function types. Since people are likely to use function types and unlikely to use params types, we decided to keep the params private to avoid these frustrating autocomplete/autoimport errors. But I don't think that's the case here.

This should be a public API since the users can access it anyways through other types, yes? I'll make it public then 👍 It doesn't make sense to hide it and your other arguments make sense as well.

sodic · 2024-08-21T10:37:40Z

waspc/data/Generator/templates/sdk/wasp/server/oauth/oneTimeCode.ts

@@ -0,0 +1,50 @@
+import { createJWT, validateJWT, TimeSpan } from '../../auth/jwt.js'
+
+export const tokenStore = createTokenStore();


Any chance of avoiding a singleton module here?

We need the one time tokens to allow the client to exchange one time codes for session IDs. If the server and client were on the same domain, it would be possible. This is not the case right now, so we need this kind of a singleton that keeps track of the tokens. This is not introduced in this PR btw. so I wouldn't change it in this PR irregardless.

At some point in the future, we'll need to figure out the cookies story so we can avoid this.

We need the one time tokens to allow the client to exchange one time codes for session IDs. If the server and client were on the same domain, it would be possible.

Oh, I understand we need the single instance that keeps the tokens. What I meant was "Can we not use a singleton pattern for it?"

This is not introduced in this PR btw. so I wouldn't change it in this PR irregardless.

It's perfectly fine not to do it now. FYI, here are two good posts I have bookmarked that will help us avoid singletons in the future (although I admit I sometimes break this rule myself):

https://softwareengineering.stackexchange.com/questions/40373/so-singletons-are-bad-then-what

https://stackoverflow.com/questions/1300655/whats-alternative-to-singleton

Good thing we're not testing anything so the singletons aren't causing any problems 🥲

Feel free to resolve.

It's not a singleton in a sense that there can only exist only one instance of the object. It's true that we create a single instance that we have to use in both places where it's needed. In the case of unit tests, we could make the token store a param of the which ever fn we wanted to unit test.

waspc/src/Wasp/Generator/SdkGenerator/Server/OAuthG.hs

web/docs/auth/auth-hooks.md

sodic

Ok, resolved most of the stuff.

I left several more comments with suggestions I'd like, but it can definitely go out as is!

sodic · 2024-08-23T14:27:29Z

waspc/data/Generator/templates/sdk/wasp/server/auth/hooks.ts

  req: ExpressRequest
 } & InternalAuthHookParams
+
+// PRIVATE API (server)


Oh, you're refering to what we did with OnBeforeSignupHookParams and friends?

Ok, if you think this is one of those cases, we can keep it private, just add a note like we did there.

FWIW, I'm not sure this is one of thouse cases though.

The fact that we export many symbols isn't a problem by iteself - users will type up what they need and choose a quick action to import it. It only becomes a problem when the IDE suggests a niche export before a common one.

In the hooks params case, the parameter types had the same prefix as the function types. Since people are likely to use function types and unlikely to use params types, we decided to keep the params private to avoid these frustrating autocomplete/autoimport errors. But I don't think that's the case here.

waspc/data/Generator/templates/sdk/wasp/server/oauth/providers/github.ts

sodic · 2024-08-23T14:47:44Z

waspc/data/Generator/templates/sdk/wasp/server/auth/oauth/provider.ts

+
+export function defineProvider<
+  OAuthClient extends OAuth2Provider | OAuth2ProviderWithPKCE,
+  Env extends Record<string, string>


#1851 (comment)

sodic · 2024-08-23T14:58:06Z

waspc/data/Generator/templates/sdk/wasp/server/oauth/oneTimeCode.ts

@@ -0,0 +1,50 @@
+import { createJWT, validateJWT, TimeSpan } from '../../auth/jwt.js'
+
+export const tokenStore = createTokenStore();


We need the one time tokens to allow the client to exchange one time codes for session IDs. If the server and client were on the same domain, it would be possible.

Oh, I understand we need the single instance that keeps the tokens. What I meant was "Can we not use a singleton pattern for it?"

This is not introduced in this PR btw. so I wouldn't change it in this PR irregardless.

It's perfectly fine not to do it now. FYI, here are two good posts I have bookmarked that will help us avoid singletons in the future (although I admit I sometimes break this rule myself):

https://softwareengineering.stackexchange.com/questions/40373/so-singletons-are-bad-then-what

https://stackoverflow.com/questions/1300655/whats-alternative-to-singleton

Good thing we're not testing anything so the singletons aren't causing any problems 🥲

Feel free to resolve.

waspc/src/Wasp/Generator/SdkGenerator/Server/OAuthG.hs

sodic · 2024-08-23T15:06:59Z

waspc/data/Generator/templates/sdk/wasp/server/auth/oauth/providers/discord.ts

+  displayName,
+  env,
+  oAuthClient,
+})


Missing empty lines in some of these files.

Working without the linter on these templates really is a pain. It's all trivial stuff, but very annoying (for example, once you notice how inconsistent we are with the semicolons, you can't unsee it).

sodic · 2024-08-23T15:08:28Z

waspc/data/Generator/templates/sdk/wasp/server/auth/oauth/index.ts

+{=# enabledProviders.isGitHubAuthEnabled =}
+// PUBLIC API
+export { github } from './providers/github.js';
+{=/ enabledProviders.isGitHubAuthEnabled =}


Another great change with scoping the providers flags under enabledProviders

infomiho added 13 commits July 29, 2024 13:02

Implement login hooks

8151dd7

Refresh token impl

3d3dc18

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

Update login hook params

69de38b

Cleanup

a12863b

Update e2e tests

5a8ec33

Merge branch 'miho-login-hooks' into miho-refresh-token

ac77fd2

Adds headless test

e43062b

Cleanup

a37e843

Fix typo

c007564

Merge branch 'miho-login-hooks' into miho-refresh-token

4a55109

Move extra helpers. Cleanup

89d930b

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

e2e tests

57966af

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

Use dynamic imports for types

4fa259b

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

infomiho marked this pull request as ready for review July 31, 2024 12:26

Cleanup and update comment

f0c2f2b

infomiho requested a review from sodic August 2, 2024 11:45

infomiho added 13 commits August 2, 2024 16:58

Merge branch 'main' into miho-login-hooks

a992c69

Merge branch 'miho-login-hooks' into miho-refresh-token

b918cb1

Adds docs

60e82ee

Merge branch 'miho-login-hooks' into miho-refresh-token

76217bc

Docs and formatting

5afc8b0

e2e tests

3c09189

PR comments

2608397

PR comments #2

4af397b

PR comments #3

9586476

Merge branch 'miho-login-hooks' into miho-refresh-token

5d4b8dd

Update providerName type to be consistent

9c5ab8c

Update comment

78a6317

Merge branch 'miho-login-hooks' into miho-refresh-token

e450e6e

Base automatically changed from miho-login-hooks to main August 8, 2024 16:28

Merge branch 'main' into miho-refresh-token

2f050ea

sodic reviewed Aug 8, 2024

View reviewed changes

infomiho commented Aug 8, 2024

View reviewed changes

waspc/data/Generator/templates/server/src/auth/providers/oauth/handler.ts Outdated Show resolved Hide resolved

infomiho added 3 commits August 9, 2024 16:23

Cleanup docs

1a36669

Tap out

e11cb65

e2e tests

aff1acb

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

infomiho requested a review from sodic August 20, 2024 12:51

sodic reviewed Aug 21, 2024

View reviewed changes

infomiho added 2 commits August 22, 2024 13:05

Merge branch 'main' into miho-refresh-token

d19cac7

PR comments

a7e8493

infomiho force-pushed the miho-refresh-token branch from 0ee588e to a7e8493 Compare August 22, 2024 13:01

infomiho added 2 commits August 22, 2024 15:15

PR comments #2

6fbeeda

Define OAuth provider object in SDK explicitly

2df9b52

infomiho force-pushed the miho-refresh-token branch from 6170080 to 2df9b52 Compare August 22, 2024 13:41

Update docs

2e42655

infomiho requested a review from sodic August 22, 2024 13:47

sodic approved these changes Aug 23, 2024

View reviewed changes

PR comments

48c110b

infomiho merged commit a6573c2 into main Aug 26, 2024
6 checks passed

infomiho deleted the miho-refresh-token branch August 26, 2024 12:32

dschwertfeger mentioned this pull request Aug 27, 2024

2016/twitter: Add 𝕏 (fka Twitter) auth to social auth providers #2253

Open

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refresh token impl #2212

Refresh token impl #2212

infomiho commented Jul 30, 2024 •

edited

Loading

sodic left a comment

sodic left a comment •

edited

Loading

sodic Aug 21, 2024

infomiho Aug 22, 2024

sodic Aug 23, 2024 •

edited

Loading

infomiho Aug 26, 2024

sodic Aug 21, 2024

infomiho Aug 22, 2024 •

edited

Loading

sodic Aug 23, 2024

infomiho Aug 26, 2024

sodic left a comment

sodic Aug 23, 2024 •

edited

Loading

sodic Aug 23, 2024

sodic Aug 23, 2024

sodic Aug 23, 2024

sodic Aug 23, 2024

		@@ -0,0 +1,50 @@
		import { createJWT, validateJWT, TimeSpan } from '../../auth/jwt.js'

		export const tokenStore = createTokenStore();

Refresh token impl #2212

Refresh token impl #2212

Conversation

infomiho commented Jul 30, 2024 • edited Loading

What was done

Left to do

sodic left a comment

Choose a reason for hiding this comment

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

sodic Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infomiho Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sodic left a comment

Choose a reason for hiding this comment

sodic Aug 23, 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

infomiho commented Jul 30, 2024 •

edited

Loading

sodic left a comment •

edited

Loading

sodic Aug 23, 2024 •

edited

Loading

infomiho Aug 22, 2024 •

edited

Loading

sodic Aug 23, 2024 •

edited

Loading