[SDK-4319] Add support for Edge runtime #1269

adamjmcgrath · 2023-06-26T14:44:09Z

📋 Changes

Have added support for the Edge runtime (in the app directory) to the auth handlers and session helpers:

Auth Handlers
handleAuth
handleLogin
handleCallback
handleLogout
handleProfile

Session Helpers
withApiAuthRequired
withPageAuthRequired
updateSession
getAccessToken
(getSession is already supported)

We now use 2 OIDC clients:

For Node: https://github.com/panva/node-openid-client
For Edge: https://github.com/panva/oauth4webapi

Using Jest "projects" to run the handler tests in both runtimes
Have updated the example app to use edge as well as node
Also support added for middleware responses (see #1150)

📎 References

Fixes #912 #1150 #1257

🎯 Testing

There is an example app running on the edge runtime on Vercel https://app-dir-auth0.vercel.app/ (source)

vercel · 2023-06-26T14:44:13Z

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

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
nextjs-auth0	⬜️ Ignored (Inspect)			Jun 28, 2023 11:20am

Widcket · 2023-06-26T22:20:19Z

example-app/app/nav.tsx

@@ -41,28 +41,52 @@ export default function Nav() {
                <a>Profile</a>
              </Link>
            </li>
+            <li>
+              <Link href="/edge-profile" legacyBehavior>


Is legacyBehavior still needed here (and in other links)?

I was just moving this over from the old example app, I could change them - but I'd rather do that in another PR

Widcket · 2023-06-27T22:00:12Z

src/auth0-session/client/edge-client.ts

+      return auth0LogoutUrl.toString();
+    }
+    if (!as.end_session_endpoint) {
+      throw new Error('no rp inititated logout');


Should this error have a more specific type?

None of our configuration errors have a type, they just need to tell the developer they've configured the SDK wrong

Should this error in particular be a little more descriptive then? If I were a developer new to identity and got that error, I think I'd be confused.

Sure - I can improve the message

Widcket · 2023-06-27T22:24:37Z

src/auth0-session/client/edge-client.ts

+    if (clientPrivateKey && !(clientPrivateKey instanceof CryptoKey)) {
+      clientPrivateKey = await jose.importPKCS8<CryptoKey>(clientPrivateKey, clientAssertionSigningAlg || 'RS256');
+    }
+    const response = await oauth.authorizationCodeGrantRequest(


Can this call throw an exception? If so, should it be caught, wrapped, and rethrown?

This gets caught here https://github.com/auth0/nextjs-auth0/blob/edge-auth-handlers/src/auth0-session/handlers/callback.ts#L61-L79

In the node handler we're wrapping the errors in our own, more specific error types:

nextjs-auth0/src/auth0-session/client/node-client.ts

Line 188 in 141ec7b

throw new IdentityProviderError(err);

authorizationCodeGrantRequest doesn't throw an IdentityProvider error (one with an error / error_description that comes from the IDP) it just returns a fetch response (or throws config errors like, "you forgot the response_type")

processAuthorizationCodeOpenIDResponse returns an IDP error (not throws) which is thrown on L158

Widcket · 2023-06-27T22:25:17Z

src/auth0-session/client/edge-client.ts

+      }
+    );
+
+    const result = await oauth.processAuthorizationCodeOpenIDResponse(


Same as above.

See https://github.com/auth0/nextjs-auth0/pull/1269/files/141ec7bff57684747e99baddf02840706adda33b#r1245019748

Widcket · 2023-06-27T22:25:50Z

src/auth0-session/client/edge-client.ts

+
+  async userinfo(accessToken: string): Promise<Record<string, unknown>> {
+    const [as, client] = await this.getClient();
+    const response = await oauth.userInfoRequest(as, client, accessToken, this.httpOptions());


Can this call throw an exception? If so, should it be caught, wrapped, and rethrown?

This get's caught here https://github.com/auth0/nextjs-auth0/blob/edge-auth-handlers/src/handlers/profile.ts#L237

In the node handler we're wrapping the errors in our own, more specific error types:

nextjs-auth0/src/auth0-session/client/node-client.ts

Line 188 in 141ec7b

throw new IdentityProviderError(err);

Not for userinfo - but yeah - fair enough, I'll create a UserInfoError and add it to the node and edge clients

Widcket · 2023-06-27T22:26:09Z

src/auth0-session/client/edge-client.ts

+
+  async refresh(refreshToken: string, extras: { exchangeBody: Record<string, any> }): Promise<TokenEndpointResponse> {
+    const [as, client] = await this.getClient();
+    const res = await oauth.refreshTokenGrantRequest(as, client, refreshToken, {


Can this call throw an exception? If so, should it be caught, wrapped, and rethrown?

This should throw an AccessTokenError will fix

This seems to be missing in 0bb1e4a

Ah yeah, was going to say - this is the same as #1269 (comment) the error from the idp get's thrown from processRefreshTokenResponse

Widcket · 2023-06-27T22:26:17Z

src/auth0-session/client/edge-client.ts

+      additionalParameters: extras.exchangeBody,
+      ...this.httpOptions()
+    });
+    const result = await oauth.processRefreshTokenResponse(as, client, res);


Same as above.

This doesn't throw, if there's an error oauth.isOAuth2Error(result) is true (which is caught below)

Widcket · 2023-06-27T22:33:18Z

src/auth0-session/handlers/login.ts

+      throw new Error(`response_type should be one of ${validResponseTypes.join(', ')}`);
+    }
+    if (!/\bopenid\b/.test(authParams.scope as string)) {
+      throw new Error('scope should contain "openid"');


Should these errors have a more specific type?

This is a configuration error too - we don't have a type for these. We could look at changing that, but it would be a lot of changes for not much/any benefit.

Widcket · 2023-06-27T22:40:06Z

src/edge.ts

@@ -28,39 +29,57 @@ const genId = () => {
    .join('');
 };

-function getInstance(params?: ConfigParameters): Auth0Edge {
+const telemetry = { name: 'nextjs-auth0', version };


Should this be moved to some common helper, as it's also used in

nextjs-auth0/src/index.ts

Line 26 in 141ec7b

const telemetry = { name: 'nextjs-auth0', version };

?

Yeah, I can move this to shared.ts

Widcket · 2023-06-27T22:47:17Z

src/utils/errors.ts

@@ -110,7 +108,7 @@ export class AccessTokenError extends AuthError {
 /**
 * @ignore
 */
-export type HandlerErrorCause = Error | AuthError | HttpError;
+export type HandlerErrorCause = Error | AuthError | (Error & { status: number; statusCode: number });


Should Error & { status: number; statusCode: number } be its own type?

Sure - I'll make an HttpError type

Widcket · 2023-06-28T08:00:58Z

tests/auth0-session/handlers/callback.test.ts

@@ -436,60 +436,6 @@ describe('callback', () => {
    expect(header.alg).toEqual('RS256');
  });

-  it('should use client secret jwt on token endpoint', async () => {


Apologies if this is something obvious, why is this test being removed?

Auth0 doesn't support client secret jwt and the edge client doesn't so I've removed support

Widcket · 2023-06-28T08:08:03Z

tests/fixtures/app-router-helpers.ts

+  } else {
+    return nodeInitAuth0(config);
+  }


Suggested change

} else {

return nodeInitAuth0(config);

}

}

return nodeInitAuth0(config);

Widcket · 2023-06-28T08:34:32Z

src/auth0-session/client/edge-client.ts

+    if (this.as) {
+      return [this.as, this.client as oauth.Client];
+    }


Not sure how long-lived instances in the Edge runtime can be (apparently there's still some notion of "cold-start"). If these were short-lived, wouldn't we be calling the discovery endpoint a lot?

We can't do any better than keeping them in memory 🤷 (unless Next.js has some built in distributed cache I haven't heard of)

Could we have an option for not using the discovery endpoint?

After all this is an Auth0-specific SDK.

Or maybe not using it by default on the edge client.

Asking because serverless !== edge. As Vercel's architect mentions, "Edge Functions bring another set of trade-offs into the mix".

What was enough/serviceable with serverless might not be with edge functions.

In any case, this probably warrants more investigation and testing with the edge runtime to find out if something more needs to be done.

Since edge functions apparently use Cloudflare Workers, I guess it comes down to the differences between Cloudflare Workers and AWS Lambda.

This SDK is agnostic to a particular edge vendor (Vercel/AWS/Azure/CloudFlare/GCP) - but sure, we can look into how their memory persistence differs between the 2 runtimes. I guess at the very least, Edge functions will be more populous given that their are more Edge locations - so I guess the probability of hitting the same box is less. But on the other hand, the Discovery is served from a CDN, so it's not a huge cost.

Widcket · 2023-06-28T08:59:23Z

tests/handlers/login-page-router.test.ts

+describe('login handler (page router)', () => {
+  afterEach(teardown);
+
+  test('should create a state', async () => {


Suggested change

test('should create a state', async () => {

test('should create a state, nonce, and code verifier', async () => {

adamjmcgrath · 2023-06-28T13:57:33Z

Thanks for the review @Widcket 🙏

Add support for Edge runtime

141ec7b

adamjmcgrath added the review:large Large review label Jun 26, 2023

adamjmcgrath requested a review from a team as a code owner June 26, 2023 14:44

adamjmcgrath changed the title ~~Add support for Edge runtime~~ [SDK-4319] Add support for Edge runtime Jun 26, 2023

Widcket reviewed Jun 26, 2023

View reviewed changes

Widcket reviewed Jun 27, 2023

View reviewed changes

Widcket reviewed Jun 28, 2023

View reviewed changes

adamjmcgrath added 2 commits June 28, 2023 12:17

Address feedback from @Widcket

0bb1e4a

Address feedback from @panva

149ae09

Widcket approved these changes Jun 28, 2023

View reviewed changes

adamjmcgrath merged commit 3adc5d0 into beta Jun 28, 2023
3 checks passed

adamjmcgrath deleted the edge-auth-handlers branch June 28, 2023 13:57

adamjmcgrath mentioned this pull request Jun 28, 2023

Release v3.0.0-beta.3 #1272

Merged

matamatanot mentioned this pull request Oct 11, 2023

RFC: Amplify JS Library v6 Developer Preview - Smaller Bundle Size, Improved TypeScript and Next.js support aws-amplify/amplify-js#12147

Closed

	test('should create a state', async () => {
	test('should create a state, nonce, and code verifier', async () => {

[SDK-4319] Add support for Edge runtime #1269

[SDK-4319] Add support for Edge runtime #1269

Conversation

adamjmcgrath commented Jun 26, 2023

📋 Changes

📎 References

🎯 Testing

vercel bot commented Jun 26, 2023 • 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

Widcket Jun 28, 2023 • 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

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

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

Widcket Jun 28, 2023 • 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

Widcket Jun 28, 2023 • 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

adamjmcgrath commented Jun 28, 2023

vercel bot commented Jun 26, 2023 •

edited

Loading

Widcket Jun 28, 2023 •

edited

Loading

Widcket Jun 28, 2023 •

edited

Loading

Widcket Jun 28, 2023 •

edited

Loading