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-1417] Customizable default scopes #435

Merged
merged 17 commits into from Apr 30, 2020

Conversation

stevehobbsdev
Copy link
Contributor

@stevehobbsdev stevehobbsdev commented Apr 24, 2020

Description

Adds the ability to customize the default scopes that are used with authorization requests.

Today, the SDK defaults these scopes to openid profile email plus whatever the developer specifies. However, there is a need to customize what those defaults are.

This PR adds a new advancedOptions object to the SDK configuration were the defaults can be specified.

With this implementation, the openid scope is still always applied regardless of the default scope configuration. However, the developer has the ability now to remove profile and email scopes should they wish.

This PR also refactors out the getUniqueScopes mock so that the actual implementation is used in all cases. To do this, getUniqueScopes has been pulled out of utils.ts and moved to a new module scope.ts. Tests moved as well to match.

References

This PR is customized from #389 and takes precedence.

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 feature request A feature has been asked for or suggested by the community CH: Added PR is adding feature or functionality medium labels Apr 24, 2020
@stevehobbsdev stevehobbsdev added this to the vNext milestone Apr 24, 2020
@stevehobbsdev stevehobbsdev requested a review from a team April 24, 2020 14:58
@stevehobbsdev stevehobbsdev changed the title Feature/advance default scope [SDK-1417] Customizable default scopes Apr 24, 2020

expect(utils.getUniqueScopes).toHaveBeenCalledWith(
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to do

import * as scope from '../src/scope';

beforeEach(() => {
  jest.spyOn(scope, 'getUniqueScopes');
});

it('should do something', () => {
  // do something
  expect(scope.getUniqueScopes).toHaveBeenCalledWith(...);
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did try this but it wasn't working thanks to doing jest.resetAllMocks() as well. Turns out you need to manually restore a spy, as resetting a spy results in a function with no return value.

However, I don't believe the tests need to assert this any more as we're already asserting that the scopes are correct in assertions for cache.get, utils.createQueryParams, etc. We already have a test suite that asserts usage of getUniqueScopes too.

I will leave the spy setup in though in case we want to mock it in the future and we forget about this problem ☝️

src/scope.ts Outdated
* @ignore
*/
export const getUniqueScopes = (...scopes: string[]) => {
const scopeString = scopes.filter(Boolean).join();
Copy link
Contributor

Choose a reason for hiding this comment

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

(optional) since split takes a regex you could simplify to:

return dedupe(scopes.join(' ').split(/\s+/).join(' '));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also needed a trim() in there, but yes - implemented this.

src/scope.ts Outdated
/**
* @ignore
*/
const dedupe = arr => arr.filter((x, i) => arr.indexOf(x) === i);
Copy link
Contributor

Choose a reason for hiding this comment

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

(optional) now we have Set, you can do:

const dedupe = arr => [...new Set(arr)]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to change it to [...(new Set(arr))], but yes - using Set here makes sense now that we have the polyfill available.

However, doing this requires us to use the --downlevelIteration compiler flag which adds a bit of bloat to the code. The code can be changed to Array.from(new Set(arr)) which does not require this flag. Possibly a little more readable too.

src/global.ts Outdated Show resolved Hide resolved
src/global.ts Outdated Show resolved Hide resolved
@stevehobbsdev stevehobbsdev mentioned this pull request Apr 29, 2020
4 tasks
@stevehobbsdev stevehobbsdev merged commit 0af848a into master Apr 30, 2020
picosam added a commit to picosam/auth0-spa-js that referenced this pull request May 12, 2020
* Fix typings to allow custom claims in ID token (auth0#386)

* Update global.ts

* fix: allow any value in unknown id token claim

* Run release:clean at the end of the release process (auth0#395)

* Merge 1.7.0 beta branch (auth0#419)

* Export types from global TypeScript file.

* Fix BaseLoginOptions JSDoc.

* Fix Auth0Client export and integration tests.

* Prevent breaking changes with type and import.

* Add export for Auth0Client type.

* [SDK-1178] Local Storage caching mechanism (auth0#303)

* Refactored existing cache

* Renamed to InMemoryCache
* Removed default export
* Added ICache
* Refactored tests
* Auth0Client now uses ICache instead of implemented cache type

* Added ability to configure cache strategy

* Added VS Code debug configuration for running tests

* Implemented local storage cache

* Made use of "in" to check presence of key in object

* Refactored playground page to use Vue + Bootstrap

* Added ability to clear the token cache

Added clear() to the ICache interface, meaning that this had to be
applied to the memory and localstorage caches.

Made use of jest-localstorage-mock package for easier testing with
localstorage, making the clear method easier to test for the
LocalStorageCache implementation.

* Revamped the playground page with Vue and Bootstrap additions

* Vulnerable dependency update

* Added a section in the readme about the caching strategy

* Fixed integration tests

* Readme wording

* Refactored how items are cleared from local storage

* Refactored cache key

* Readme tweak to make what 'data' is more clear

* Renamed cacheStrategy option to cacheLocation

* Cache now includes client_id in key

* [SDK-1179] Support for rotating refresh tokens (auth0#315)

* Refactored getting token using iframe into its own method

* Implemented getTokenUsingRefreshToken

* Fixed up the playground page to support refresh tokens

* Set offline_access scope during initialization

* Added error condition for when a refresh token isn't stored or no cache exists

* Removed specification of audience when calling token endpoint

* Clarified docs on useRefreshTokens

* Simplified usage of getUniqueScopes in index.ts

* Fixed some playground syntax issues for IE11

* Playground now shows auth info on load if authenticated

* Simplified integration tests

* Added more integration tests around getting access tokens

* Encoded the nonce value when building authorize URLs

* Renamed encodeState to encode

* Fixed broken integration test

* Release 1.7.0-beta.1 (auth0#327)

* Release 1.7.0-beta.1

* Tweaked intermittently-failing test

* Fixed issue with cache not retaining refresh token (auth0#333)

* Fixed issue with cache not retaining refresh token

* Fix integration tests

* Removed unused core-js import

* Extracted 1 day in seconds value to a constant

* Applied comment to be consistant with related test

* Applied brace styling for consistancy

* Reworked expiry tests to mock date instead of using negative exp

* Added some comments to the cache tests to explain the test scenario

* Cleaned up JS return statement styling

* Prepare 1.7.0-beta.2 (auth0#334)

* Wrapped InMemoryCache implementation in a closure (auth0#337)

* Reinstated lock on getTokenSilently

* Fixed up code + tests after rebase

* Fixed up types

* Removed undesirables from the docs generation

* [SDK-1352] Removed setTimeout cache removal in favour of removal-on-read (auth0#354)

* Removed setTimeout cache expiry in favour of expiry-on-read

* Replace magic values with a constant

* [SDK-1279] getTokenSilently retry logic (auth0#336)

* Added retry logic to getJSON

* Moved retry count to a constant

* Reverted changes to oauthToken

* Reduced retry count to 3

* Implemented a timeout around the fetch call

* Made the fetch timeout a default value and adjusted tests

* Fixed broken test after merge

* Implemented AbortController to abort fetch on timeout

* Added abortcontroller polyfill

* Created factory function for AbortController to be mocked and tested

* [SDK-1352] Stop checking `isAuthenticated` cookie on initialization when using local storage (auth0#352)

* Changes to the initialization strategy

* Removed unused import from a test

* Release 1.7.0-beta.3 (auth0#358)

* Fix error in library type definitions (auth0#367)

`// @ts-ignore` comment is not preserved in the generated type definition, which means that library ships broken type definitions and consumers will get an error when they attempt to use it.

Reproduction:

```
$ npm i @auth0/auth0-spa-js@1.7.0-beta.3 typescript
$ cat index.ts
import c from '@auth0/auth0-spa-js';
$ ./node_modules/.bin/tsc --noEmit index.ts
node_modules/@auth0/auth0-spa-js/dist/typings/index.d.ts:9:8 - error TS2440: Import declaration conflicts with local declaration of 'Auth0Client'.

9 import Auth0Client from './Auth0Client';
         ~~~~~~~~~~~

Found 1 error.
```

* [SDK-1386] Fall back to iframe method if no refresh token is available (auth0#364)

* Logic falls back to the iframe method when no refresh token is found

* Cleaned up a variable name

* Updated integration test

* Release 1.7.0-beta.4 (auth0#370)

* Updated cache configuration instructions in the readme

* Removed unused cacheStrategy param from buildAuthorizeUrl

* [SDK-1379] Export constructor (auth0#385)

Export constructor

* Release 1.7.0-beta.5 (auth0#393)

* [SDK-1507] Dependency upgrade (auth0#405)

* Ran npm audit fix

* Updated packages within semver

* Updated typedoc

* Updated rollup to 2.3.3 + plugins

* Updated idtoken-verifier to 2.0.2

* Fixed warnings on async describe blocks

* Updated prettier/pretty-quick

* Updated Husky and ran husky-upgrade

* Updated Cypress, wait-on and concurrently

* Upgraded tslint

* Updated circle image

* [SDK-1516] Web Workers  (auth0#409)

* fetch in a web worker

* token worker

* known issue: doesn't work if user already logged in (need authorization_code grant_type to populate the refresh token)

* add iframe fallback

* fix tests

* We want to load:
`rollup-plugin-worker-loader::module:./token.worker.ts`
But not:
rollup-plugin-worker-loader::module:/Users/adammcgrath/dev/auth0-spa-js/src/token.worker.ts
TODO: check windows

* Fixed ES5 transpilation for rollup worker plugin

* Make messages serializable using `JSON.parse(JSON.stringify({}))`
Swap imports per https://github.com/mo/abortcontroller-polyfill/blob/3f1c13d2e4087ee15ded81786f1110ae547931bb/README.md#using-it-on-internet-explorer-11-msie11

* only use worker for non ie, local refresh token opts

TODO: fix tests

* Fix tests

* Removed refresh token from worker memory when not included in response

* Moved offline_access scope configuration to constructor

* Modified playground to use both factory func and constructor

* Remove Object.assign

* Remove checks to fix rebuild issue

* Abort timed out requests in the Web Worker

* Errors

* Fix tests

* Add some more tests

* DRY up the tests a little

* Moar tests

* unused import

* update rollup-plugin-web-worker-loader
don't run `addEventListener` in tests
add test for missing refresh token and localstorage

* add timeout tests

* add browser tests

* Only include files in the typings copy process

* Fix fallback logic when no RT and no worker

* add browser tests and comments

* bump node version in Jenkinsfile

* Removed unused import

* Added sanity check for web worker support

* Fixed tests for window.Worker check

* Moved constructor tests into Auth0Client

Co-authored-by: Steve Hobbs <steve.hobbs.mail@gmail.com>

* Updated readme with info on refresh tokens (auth0#415)

* Implemented fallback to iframe when given specific audience (auth0#414)

* Check if iframe is still in body before removing (auth0#399)

If the iframe is removed from the DOM prior to the timeout it would error on removeChild.

Error thrown: `Uncaught DOMException: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.`

Bug introduced in auth0#376

Co-authored-by: Steve Hobbs <steve.hobbs@auth0.com>

* Check if source of event exists before closing it (auth0#410)

When the iframe is closed, the source of the event message is null, resulting in an error: Cannot read property 'close' of undefined (Chrome).

Co-authored-by: Steve Hobbs <steve.hobbs@auth0.com>

* Removed unused error import

Co-authored-by: maxswa <mkswartwout@gmail.com>
Co-authored-by: Yaroslav Admin <devoto13@gmail.com>
Co-authored-by: Adam Mcgrath <adam.mcgrath@auth0.com>
Co-authored-by: Paul Falgout <paul@otterball.com>
Co-authored-by: gerritdeperrit <gertjansterrenburg@gmail.com>

* Release 1.7.0 (auth0#421)

* Updated readme to include information about the RT fallback (auth0#423)

* Reset prettier config to pre-2.0 defaults, reformatted some files (auth0#431)

* include polyfill for Set (auth0#426)

* Updated `login_hint` js docs to clarify usage with Lock (auth0#441)

* Moved es-check to build script (auth0#442)

* Update rollup-plugin-web-worker-loader to 1.1.1 (auth0#443)

* Upgraded rollup-plugin-web-worker-loader to 1.1.1

This fixes an issue with Blob during Gatsby/SSR build

* Removed 'check' and other TS config

This is no longer needed now that the rollup plugin is fixed.

* Formatting

* Fixed playground string interp issue for IE11

* [SDK-1417] Customizable default scopes (auth0#435)

* Extracted changes needed to customize defaultScope

* Moved existing defaultScopes test to the right place

* getUniqueScopes moved into scope.ts

* Refactor getUniqueScopes into its own module

This allows it to me mocked or unmocked separately from utils.
index.test.ts has been completely refactored to use an unmocked version
and the expectations have changed as a result.

* Stop mutating optios.scope and store in separate var

* Added tests for relevant functions for using advanced default scopes

* Fix constructor after merge

* advancedOptions.defaultScope can accept empty/null value

* Added advanced section to readme

Docs build to follow in the release PR

* Set up proper spy for getUniqueScopes

* Fixed types in JS docs

* Simplified getUniqueScopes implementation

* Cleaned up index test file

* Simplified the defaultScope check using null chaining operator

Co-authored-by: Sri Hari Raju Penmatsa <pshraju@gmail.com>

* Release 1.8.0 (auth0#445)

* Fix issue with create-react-app webpack build (auth0#451)

* Release 1.8.1 (auth0#453)

Co-authored-by: Steve Hobbs <steve.hobbs@auth0.com>
Co-authored-by: maxswa <mkswartwout@gmail.com>
Co-authored-by: Yaroslav Admin <devoto13@gmail.com>
Co-authored-by: Adam Mcgrath <adam.mcgrath@auth0.com>
Co-authored-by: Paul Falgout <paul@otterball.com>
Co-authored-by: gerritdeperrit <gertjansterrenburg@gmail.com>
Co-authored-by: Tony Knight <tony@aquicore.com>
Co-authored-by: Sri Hari Raju Penmatsa <pshraju@gmail.com>
@stevehobbsdev stevehobbsdev deleted the feature/advance-default-scope branch May 21, 2020 22:10
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 feature request A feature has been asked for or suggested by the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants