Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SDK-2794] Return token response in getTokenSilently #803

Merged
merged 11 commits into from
Oct 5, 2021

Conversation

stevehobbsdev
Copy link
Contributor

@stevehobbsdev stevehobbsdev commented Sep 22, 2021

Description

This PR adds a new option that allows the developer to get access to the response from the /oauth/token endpoint rather than just the access token when calling getTokenSilently.

await auth0.getTokenSilently({ detailedResponse: true });

The response is returned with a couple of caveats:

  • refresh_token is removed
  • scope returns the scope as returned by /oauth/token and does not try to populate it based on the Auth0Client configuration

In the case where there was a cache hit and ignoreCache is false, the properties that would appear in the token endpoint response are extracted (minus refresh_token) and the scope is taken from a new cache property oauthTokenScope, which stores the scope as returned by /oauth/token.

However, this will continue to return undefined until the user performs an operation that causes the token endpoint to return different scopes to what was asked, and we don't yet allow the developer to de-scope an access token, so I'm not sure this is a real issue.

References

Closes #715

Testing

  • This change adds test coverage for new/changed/fixed functionality

Checklist

  • I have added documentation for new/changed functionality in this PR or in auth0.com/docs
  • All active GitHub checks for tests, formatting, and security are passing
  • The correct base branch is being used, if not master

@stevehobbsdev stevehobbsdev added CH: Added PR is adding feature or functionality review:medium Medium review labels Sep 22, 2021
@stevehobbsdev stevehobbsdev force-pushed the sdk-2794/token-response branch from 3e65a77 to ebd80b9 Compare September 22, 2021 14:55
src/global.ts Outdated
*
* The default is `false`.
*/
verboseResponse?: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not married to the name, if there are better suggestions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no better naming, but I do feel like verbose has a debugging/error connotation to it. But that might just be because I only ever use that term when configuring log levels for errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair, how about detailedResponse?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that should work better as opposed to verbose, ye. detailedResponse or fullResponse.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to detailedResponse.

@@ -68,6 +68,7 @@ export type CacheEntry = {
scope: string;
client_id: string;
refresh_token?: string;
oauthTokenScope?: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in the description, this is an attempt to "remember" the actual scopes that came back from /oauth/token (if any) so that they can be reliably returned in getTokenSilently if the verbose response is asked for, mainly for consistency.

@stevehobbsdev stevehobbsdev marked this pull request as ready for review September 22, 2021 14:58
@stevehobbsdev stevehobbsdev requested a review from a team as a code owner September 22, 2021 14:58
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Sep 22, 2021

This pull request introduces 1 alert when merging ebd80b9 into ecec4df - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

src/global.ts Outdated
*
* The default is `false`.
*/
verboseResponse?: boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no better naming, but I do feel like verbose has a debugging/error connotation to it. But that might just be because I only ever use that term when configuring log levels for errors.

@stevehobbsdev
Copy link
Contributor Author

Unfortunately I can't see what the coverage problem is now, could be a rounding error 🤷🏻
image

windowSpy.mockRestore();
});

it('returns the full token response when returnMode = "verbose"', async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will need to change the test descriptions, this is still using returnMode = 'verbose', while at the moment the property is called verboseReponse.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the test descriptions 👍🏻

@stevehobbsdev stevehobbsdev requested review from frederikprijck and a team September 27, 2021 14:53
@stevehobbsdev stevehobbsdev merged commit 4641135 into master Oct 5, 2021
@stevehobbsdev stevehobbsdev deleted the sdk-2794/token-response branch October 5, 2021 14:12
@stevehobbsdev stevehobbsdev mentioned this pull request Oct 11, 2021
@tmm-aleborgne
Copy link

tmm-aleborgne commented Oct 29, 2021

There is always this line which is problematic

scope: transaction.scope,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CH: Added PR is adding feature or functionality review:medium Medium review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return scope detail during access token retrieval
3 participants