Allow user to choose what tokens to expose

[ispyinternet]

Would you consider exposing the id_token too?

(That may be an AWS Cognito specific thing? - but very desireable!)

[lf94]

I don't recall id_token being part of the spec. goes to check Nope, not in https://tools.ietf.org/html/rfc6749 .

I haven't looked at AWS Cognito at all, but I assume they are using not just authorization code & pkce extension. Maybe my assumption is wrong but I'll need time to investigate. ☺

If they are going off-spec, it would be better if they corrected the issue. I think this is an issue they would correct (or maybe I'm too optimistic).

[lf94]

So I've looked into it very briefly.

https://openid.net/specs/openid-connect-core-1_0.html#IDToken appears to describe what it is.

https://openid.net/specs/openid-connect-core-1_0.html#Authentication shows that what you are asking is for Implicit flow, not Authorization Code flow.

Since this package focuses only on handling Authorization Code flow, it follows that we can't handle this.

[ispyinternet]

I would comment that, yes ID Token is part of OpenID Connect 1.0 an identity layer on top of OAuth 2.0. In your second link, that table of response_type | flow, can easily be misinterpreted, but the value of response_type has nothing to do with the ID or Access tokens returned, its just an identifier that tells the server which auth flow is happening, I don't think that the spec says anywhere that the ID token should only be returned in the implicit flow, in fact the table above specifies all tokens are returned from endpoint under Authorization Code flow type and with cognito, I am using authorization code flow and ID token is returned.

[lf94]

I would comment that, yes ID Token is part of OpenID Connect 1.0 an identity layer on top of OAuth 2.0.

This client doesn't support that identity layer - as I've said this client is very focused as to what it does. This makes it easier to maintain by anyone and easier to audit.

in fact the table above specifies all tokens are returned

Yea I understand that to be the case too now that I've re-read as you suggested.

I don't think that the spec says anywhere that the ID token should only be returned in the implicit flow

Right, but id_token is an OpenID-ism. How would you suggest we expose these additional properties? What are the implications?

From what I understand it's just a matter of exposing this property returned from after swapping an authorization code for an access token (& id_token). My issue is this signals we support OpenID in some way, which we don't. I want to signal we expose additional params passed back but again, I'm not sure what the implications of this are...And right now I don't have the time to think about it, but I'm open to your thoughts. :)

[ispyinternet]

I don't think that the spec says anywhere that the ID token should only be returned in the implicit flow

Right, but id_token is an OpenID-ism. How would you suggest we expose these additional properties? What are the implications?

I'm not really sure ¯_(ツ)_/¯

From what I understand it's just a matter of exposing this property returned from after swapping an authorization code for an access token (& id_token). My issue is this signals we support OpenID in some way, which we don't. I want to signal we expose additional params passed back but again, I'm not sure what the implications of this are...And right now I don't have the time to think about it, but I'm open to your thoughts. :)

I can't really share any thoughts about what is the correct path for your library to take. In terms of implemenation, if you don't want to support it directly, perhaps a config option that indicated which keys to look for / expose from the server response e.g.

 oauth = new OAuth2AuthCodePKCE({
            ...
            scopes: ['profile email openid'],
            tokens: [ 'access', 'refresh', 'id'] or ['accessToken','refreshToken','id_token'] 
            // with default being access,refresh
            ...
})

[lf94]

perhaps a config option that indicated which keys to look for / expose from the server response

That's an excellent idea. At least this way, the user must explicitly say what they want and the implications of their choice fall on them. If tokens is not defined then by default I guess returning access and refresh tokens is good enough.

[lf94]

@ispyinternet if you could please try this patch and get back to me, I would appreciate it. Our GitLab mirror is not setup to mirror branches so this is probably the best way to get the changes to you. I have nothing setup to test against OpenID. I have done some tests in the node REPL and things seem fine.

Apply it to latest master.

expose-more-tokens.txt

[lf94]

Closing - if someone needs this, they can use the patch provided.

[chrisjuchem]

Hi @lf94. That patch no longer applies cleanly to the latest master. However, I was able to manually apply the patch bit by bit and tested it out. I needed to modify exchangeAuthCodeForAccessToken the same way you modified exchangeRefreshTokenForAccessToken, but then it worked great.

I would urge you to include this in the library as running OIDC and PKCE together is an extremely common use case. I can understand the desire to keep this library as simple as possible, but as is, it is unusable for me and, I assume, many others who need OIDC.

If you don't want to add any logic specifically related to tokens, would you at least be able to allow passing a callback as a configuration option that is called with the IdP json response whenever a code or refresh token is exchanged for the access token so that we can extract the id_token ourselves?

Thanks.

For reference, here's what my final patch ended up looking like:

diff --git a/node_modules/@bity/oauth2-auth-code-pkce/index.ts b/node_modules/@bity/oauth2-auth-code-pkce/index.ts
index 33229cc..7857b4b 100644
--- a/node_modules/@bity/oauth2-auth-code-pkce/index.ts
+++ b/node_modules/@bity/oauth2-auth-code-pkce/index.ts
@@ -5,6 +5,7 @@
 export interface Configuration {
   authorizationUrl: URL;
   clientId: string;
+  explicitlyExposedTokens?: string[];
   onAccessTokenExpiry: (refreshAccessToken: () => Promise<AccessContext>) => Promise<AccessContext>;
   onInvalidGrant: (refreshAuthCodeOrRefreshToken: () => Promise<void>) => void;
   redirectUrl: URL;
@@ -23,6 +24,7 @@ export interface State {
   authorizationCode?: string;
   codeChallenge?: string;
   codeVerifier?: string;
+  explicitlyExposedTokens?: ObjStringDict;
   hasAuthCodeBeenExchangedForAccessToken?: boolean;
   refreshToken?: RefreshToken;
   stateQueryParam?: string;
@@ -42,9 +44,11 @@ export type Scopes = string[];

 export interface AccessContext {
   token?: AccessToken;
+  explicitlyExposedTokens?: ObjStringDict;
   scopes?: Scopes;
 };

+export type ObjStringDict = { [_: string]: string };
 export type HttpClient = ((...args: any[]) => Promise<any>);
 export type URL = string;

@@ -315,6 +319,7 @@ export class OAuth2AuthCodePKCE {
     const {
       accessToken,
       authorizationCode,
+      explicitlyExposedTokens,
       hasAuthCodeBeenExchangedForAccessToken,
       refreshToken,
       scopes
@@ -338,7 +343,7 @@ export class OAuth2AuthCodePKCE {
       return onAccessTokenExpiry(() => this.exchangeRefreshTokenForAccessToken());
     }

-    return Promise.resolve({ token: accessToken, scopes });
+    return Promise.resolve({ token: accessToken, explicitlyExposedTokens, scopes });
   }

   /**
@@ -347,7 +352,7 @@ export class OAuth2AuthCodePKCE {
   public exchangeRefreshTokenForAccessToken(): Promise<AccessContext> {
     this.assertStateAndConfigArePresent();

-    const { onInvalidGrant, tokenUrl } = this.config;
+    const { tokenUrl } = this.config;
     const { refreshToken } = this.state;

     if (!refreshToken) {
@@ -366,8 +371,11 @@ export class OAuth2AuthCodePKCE {
       }
     })
     .then(res => res.status === 400 ? Promise.reject(res.json()) : res.json())
-    .then(({ access_token, expires_in, refresh_token, scope }) => {
+    .then((json) => {
+      const { access_token, expires_in, refresh_token, scope } = json;
+      const { explicitlyExposedTokens } = this.config;
       let scopes = [];
+      let tokensToExpose = {};

       const accessToken: AccessToken = {
         value: access_token,
@@ -382,6 +390,15 @@ export class OAuth2AuthCodePKCE {
         this.state.refreshToken = refreshToken;
       }

+      if (explicitlyExposedTokens) {
+        tokensToExpose = explicitlyExposedTokens.reduce(
+          (a: ObjStringDict, token: string) =>
+            json[token] ? { ...a, [token]: json[token] } : a,
+          {}
+        );
+        this.state.explicitlyExposedTokens = tokensToExpose;
+      }
+
       if (scope) {
         // Multiple scopes are passed and delimited by spaces,
         // despite using the singular name "scope".
@@ -390,10 +407,23 @@ export class OAuth2AuthCodePKCE {
       }

       localStorage.setItem(LOCALSTORAGE_STATE, JSON.stringify(this.state));
-      return { token: accessToken, scopes };
+
+      if (Object.keys(tokensToExpose).length > 0) {
+        return {
+          explicitlyExposedTokens: tokensToExpose,
+          token: accessToken,
+          scopes
+        };
+      }
+
+      return {
+        token: accessToken,
+        scopes
+      };
     })
     .catch(jsonPromise => Promise.reject(jsonPromise))
     .catch(data => {
+      const { onInvalidGrant } = this.config;
       const error = data.error || 'There was a network error.';
       switch (error) {
         case 'invalid_grant':
@@ -509,8 +539,11 @@ export class OAuth2AuthCodePKCE {
         });
       }

-      return jsonPromise.then(({ access_token, expires_in, refresh_token, scope }) => {
+      return jsonPromise.then((json) => {
+        const { access_token, expires_in, refresh_token, scope } = json;
+        const { explicitlyExposedTokens } = this.config;
         let scopes = [];
+        let tokensToExpose = {};
         this.state.hasAuthCodeBeenExchangedForAccessToken = true;
         this.authCodeForAccessTokenRequest = undefined;

@@ -527,6 +560,15 @@ export class OAuth2AuthCodePKCE {
           this.state.refreshToken = refreshToken;
         }

+        if (explicitlyExposedTokens) {
+          tokensToExpose = explicitlyExposedTokens.reduce(
+            (a: ObjStringDict, token: string) =>
+              json[token] ? { ...a, [token]: json[token] } : a,
+            {}
+          );
+          this.state.explicitlyExposedTokens = tokensToExpose;
+        }
+
         if (scope) {
           // Multiple scopes are passed and delimited by spaces,
           // despite using the singular name "scope".
@@ -535,7 +577,19 @@ export class OAuth2AuthCodePKCE {
         }

         localStorage.setItem(LOCALSTORAGE_STATE, JSON.stringify(this.state));
-        return { token: accessToken, scopes };
+
+        if (Object.keys(tokensToExpose).length > 0) {
+          return {
+            explicitlyExposedTokens: tokensToExpose,
+            token: accessToken,
+            scopes
+          };
+        }
+
+        return {
+          token: accessToken,
+          scopes
+        };
       });
     });
   }

[lf94]

@chrisjuchem the only reason I didn't include my patch is because there was no feedback, since this is not a feature I'd use. If you can provide feedback about how the patch is working for you, over a period of time, I will gladly add it. :slightly_smiling_face:

[chrisjuchem]

After using my patch for a bit I realized that my oauth server (Auth0) wasn't granting refresh tokens correctly. I had to modify the patch to allow me to pass some custom params to the authorize and refresh routes.

I also ran into an issue where getting an invalid_grant error from the refresh token route did not properly trigger onInvalidGrant because the raw promise was being passed in here instead of the json data. I did not confirm that this is an issue without my changes, but I suspect that it is.

Here is the updated patch that is now working for me. If it continues working for me I can open a pull request with the changes. @lf94

[lf94]

Sounds good to me @chrisjuchem