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

Customize defaultScope used by auth0Client #389

Closed
wants to merge 3 commits into from
Closed

Customize defaultScope used by auth0Client #389

wants to merge 3 commits into from

Conversation

srihari93
Copy link
Contributor

Description

The library includes 'openid profile email' as defaultScope and there is no way for devs to exclude these scopes. Some of our users have lot of unnecessary information in profile and their JWT tokens too big to fit in the Authorization header. We would like to have the flexibility to exclude some scopes, as we have with the auth0-lock.js.

The maintainers suggested that a advancedScope.defaultScope can be used to control defaultScope for cases like us but, keep the default behaviour for other devs. This is an acceptable solution to us and this PR implements the suggestion.

const auth0 = await createAuth0Client({
  client_id: 'cid',
  domain: 'domain.auth0.com',
  advancedOptions: {
    defaultScope: 'openid'
  }
})

The Docs should be changed to add this info.

References

Testing

A simple test is added, please see if thats enough https://github.com/srihari93/auth0-spa-js/blob/a2fc948757362de2bdb3d05f661cd6304fbaacc5/__tests__/index.test.ts#L187-L203

Checklist

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

@srihari93 srihari93 requested a review from a team March 24, 2020 11:34
@srihari93 srihari93 changed the title Extracted changes needed to customize defaultScope Customize defaultScope used by auth0Client Mar 24, 2020
@stevehobbsdev stevehobbsdev linked an issue Mar 24, 2020 that may be closed by this pull request
@stevehobbsdev
Copy link
Contributor

Thanks for this @srihari93.

We will need additional tests that cover the other entry points into the SDK that deal with default scopes. I see you've provided a test for getIdTokenClaims, we will need equivalent tests for:

  • getTokenSilently
  • loginWithRedirect
  • getUser
  • getTokenWithPopup
  • loginWithPopup
  • buildAuthorizeUrl

These all process default scopes, either by calling getUniqueScopes directly or through _getParams.

@damieng
Copy link
Contributor

damieng commented Mar 30, 2020

I think we also want to force openid to always be included as well as part of our OIDC Compliance push.

@stevehobbsdev stevehobbsdev added this to the v1.7.0 milestone Apr 7, 2020
@stevehobbsdev stevehobbsdev added small CH: Added PR is adding feature or functionality labels Apr 7, 2020
@stevehobbsdev
Copy link
Contributor

@srihari93 As mentioned above, we would need additional tests written plus we need to enforce openid even if the developer specifies other defaults.

This could be done in the constructor here, for example.

this.defaultScope = getUniqueScopes(`openid 
      ${
        this.options.advancedOptions &&
        this.options.advancedOptions.defaultScope
          ? this.options.advancedOptions.defaultScope
          : DEFAULT_SCOPE
      }`);

Let me know if you'd be able to make these changes, or whether you're happy for me to make them on your behalf.

Copy link
Contributor

@stevehobbsdev stevehobbsdev left a comment

Choose a reason for hiding this comment

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

Requires changes as mentioned in the comments above. We would like to include this in an up-coming release; let me know if we can make these changes for you.

@stevehobbsdev stevehobbsdev removed this from the v1.7.0 milestone Apr 15, 2020
@srihari93
Copy link
Contributor Author

srihari93 commented Apr 16, 2020

@stevehobbsdev Sorry, for being sparse with my communication. If you can make the changes, please go ahead. I had to deprioritize this among my other tasks, so I would appreciate if you can take the feature ahead yourself.

@stevehobbsdev
Copy link
Contributor

Will do, thanks @srihari93

@conorcussell
Copy link

If you need an extra pair of hands to get this into the next release, I'd happily make the changes mentioned above @stevehobbsdev

@stevehobbsdev
Copy link
Contributor

Thanks @conorcussell. They're already in progress, I just need to push what I have (hopefully tomorrow). Feel free to chime in on a review once they're there 👍

@stevehobbsdev
Copy link
Contributor

I could not push changes to this PR, so I have now opened #435 to implement this.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getUniqueScopes forces DEFAULT_SCOPES to be in scope
4 participants