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.

Sign up for GitHub

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

Jump to bottom

[msal-core] Fix response type configuration by basing it mainly on scopes #2022

Merged
merged 36 commits into from Aug 17, 2020
Merged

[msal-core] Fix response type configuration by basing it mainly on scopes #2022

merged 36 commits into from Aug 17, 2020

Conversation

hectormmg
Copy link
Member

@hectormmg hectormmg commented Jul 23, 2020 •
edited

This PR:

  • Fixes acquireTokenSilent() doesn't get new ID token #1241 by enabling the id_token token response type for acquireToken APIs when scopes configured include openid or profile
  • Removes clientId as only scope restriction, while keeping the behavior sending in clientId as a single scope enabled (i.e. setting response_type to id_token for acquireToken calls)
  • Refactors the logic that adds openid and profile to the request scopes by default if they are missing
  • Refactors the logic that removes clientId from final scopes to make sure it is only removed when it is the only scope (otherwise it is now treated as a resource scope)
  • Adds through tests for the mapping of scopes and account configuration to the resulting response_type for acquireToken logic.
  • Adds docs on Scopes and Response Types behavior for msal-core

@hectormmg
Add initial respnse type tests to UserAgentApplication
4eb8a1b
@hectormmg
Enable empty scopes array for login API calls
8787b8b
@hectormmg
Enable getting an id token from acquireToken APIs if only OIDC scopes…
66e2f6b 
… are configured or clientId is the only scope
@hectormmg
Add new getTokenType logic for response type configuration for acquir…
e4ae5cb 
…eToken APIs as well as remove clientId as single scope restriction from msal-core.
@hectormmg
Finish passing tests for response_type behavior for acquireToken APIs
02c2b6c
@hectormmg
Refactor getTokenType for code readability and correct class responsi…
5d9f580 
…bility
@hectormmg
Refactor clientId translation and OIDC scope addition into ScopeSet.n…
fe4fd4d 
…ormalize
@hectormmg
Fix bug where clientId was being spliced even when it wasn't the only…
42a8d51 
… scope
@hectormmg
Update documentation on scopes behavior
e2fea1a
@github-actions github-actions bot added the msal@1.x Related to msal@1.x (implicit flow) label Jul 23, 2020
@hectormmg hectormmg mentioned this pull request Jul 23, 2020
Closed
@coveralls
Copy link

coveralls commented Jul 23, 2020 •
edited

Coverage Status

Coverage increased (+0.5%) to 82.159% when pulling 9819533 on scopes-and-response-types into 16b6380 on dev.

@hectormmg hectormmg mentioned this pull request Jul 23, 2020
Closed
5 tasks
@hectormmg hectormmg added this to the msal@1.4.0 - Release milestone Jul 24, 2020
tnorling
tnorling reviewed Jul 24, 2020
View reviewed changes
Copy link
Collaborator

@tnorling tnorling left a comment

Choose a reason for hiding this comment

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

Overall nice work! Just a few comments about cleaning things up a bit

lib/msal-core/docs/response-types.md Outdated Show resolved Hide resolved
lib/msal-core/src/ScopeSet.ts Outdated Show resolved Hide resolved
lib/msal-core/src/UserAgentApplication.ts Outdated Show resolved Hide resolved
jmckennon
jmckennon approved these changes Jul 24, 2020
View reviewed changes
Copy link
Contributor

@jmckennon jmckennon left a comment

Choose a reason for hiding this comment

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

Looks good!

tnorling
tnorling approved these changes Jul 27, 2020
View reviewed changes
Copy link
Collaborator

@tnorling tnorling left a comment

Choose a reason for hiding this comment

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

Just a few nitpick suggestions but looks good otherwise

lib/msal-core/src/ScopeSet.ts Outdated Show resolved Hide resolved
lib/msal-core/src/ScopeSet.ts Outdated Show resolved Hide resolved
lib/msal-core/src/UserAgentApplication.ts Outdated Show resolved Hide resolved
@tnorling tnorling mentioned this pull request Jul 27, 2020
Merged
@hectormmg hectormmg requested a review from sameerag July 27, 2020 20:26
sameerag
sameerag approved these changes Jul 27, 2020
View reviewed changes
Copy link
Member

@sameerag sameerag left a comment

Choose a reason for hiding this comment

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

LGTM.

jo-arroyo
jo-arroyo approved these changes Jul 27, 2020
View reviewed changes
Copy link
Collaborator

@jo-arroyo jo-arroyo left a comment

Choose a reason for hiding this comment

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

LGTM! A few nits

lib/msal-core/docs/response-types.md Outdated Show resolved Hide resolved
lib/msal-core/docs/response-types.md Outdated Show resolved Hide resolved
lib/msal-core/docs/scopes.md Outdated Show resolved Hide resolved
lib/msal-core/docs/scopes.md Outdated Show resolved Hide resolved
@tnorling tnorling linked an issue Jul 31, 2020 that may be closed by this pull request
acquireTokenSilent - refreshed ID tokens have incomplete scope #1779
Closed
@tnorling tnorling changed the base branch from dev to 1.4.0-release August 17, 2020 18:43
@hectormmg hectormmg merged commit 603c101 into 1.4.0-release Aug 17, 2020
@tnorling tnorling mentioned this pull request Aug 24, 2020
Merged
@kumarunster kumarunster mentioned this pull request Aug 26, 2020
Closed
13 tasks
@tnorling tnorling deleted the scopes-and-response-types branch October 6, 2021 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tnorling tnorling tnorling approved these changes

@jmckennon jmckennon jmckennon approved these changes

@sameerag sameerag sameerag approved these changes

@jo-arroyo jo-arroyo jo-arroyo approved these changes

@DarylThayil DarylThayil Awaiting requested review from DarylThayil

@jasonnutter jasonnutter Awaiting requested review from jasonnutter

@pkanher617 pkanher617 Awaiting requested review from pkanher617

Assignees
No one assigned
Labels
msal@1.x Related to msal@1.x (implicit flow)
Projects
None yet
Milestone
msal@1.4.0 - Release
Development

Successfully merging this pull request may close these issues.

acquireTokenSilent - refreshed ID tokens have incomplete scope acquireTokenSilent() doesn't get new ID token
6 participants
@hectormmg @coveralls @tnorling @jmckennon @sameerag @jo-arroyo