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

[msal-core] Update and refactor scope validation #1681

Closed
wants to merge 63 commits into from

Conversation

hectormmg
Copy link
Member

@hectormmg hectormmg commented May 19, 2020

Description

This PR updates the way scopes are validated and dynamically generated for login requests as a primer for token request type definition behavior changes (PR #1628).

Overview

  1. Rather than having a single validateRequest method in RequestUtils that checks whether the request is of "Login" type through a boolean argument, this PR adds a separate validateLoginRequest method that is called from our login APIs, and wraps the validateRequest method which is called from acquireToken APIs.
  2. The new RequestUtils.validateLoginRequest method is responsible for appending openid and profile to the request scopes before calling the more general validateRequest method.
  3. This PR refactors UrlUtils.translateclientIdUsedInScope out into ScopeSet.generateLoginScopes in order to have that behavior under the right namespace.
  4. Updates all automated tests that break because of the new scopes validation and manipulation behavior
  5. Add tests for ScopeSet and RequestUtils classes.

Main changes per file:

ScopeSet.ts

  1. validateInputScopes: Since RequestUtils.validateRequest doesn't need to check for !isLoginCall anymore and the check for !scope is carried out regardless, ScopeSet.validateInputScopes can assume request is non-null. This means validateInputScopes now only throws in the following cases:

    1. Null scopes in request
    2. Non-Array scopes in request (i.e. space separeated scope strings)
    3. Empty scopes arrays
  2. generateLoginScopes: Since UrlUtils.translateclientIdUsedInScope deals with scopes, it is moved to ScopeSet and refactored as generateLoginScopes. This method is always called in RequestUtils.validateLoginRequest for login API calls, and is called in RequestUtils.validateRequest when any of the three recognized "login scopes" is included in the request scopes, as determined by the result of calling ScopeSet.isLoginScopes. This forces login API calls to contain 'openid' and 'profile' scopes, and allows access token requests to also make id token requests (i.e. id_token token response type). This method is written in a way that avoids adding duplicates of login scopes.

  3. isLoginScopes: A simple new method that returns true if any of the login scopes ('openid', 'profile' or the clientId value) are included in the scopes. This method determines which non-login API acquire token calls should also have 'openid' and 'profile' scopes appended and, therefore, obtain an id token as well as an access token.

ServerRequestParameters.ts

  1. Refactor scopes assignment into a single line that trims and converts scopes array to lowercase or sets scopes to empty array if request scopes are null.

UserAgentApplication:

  1. Replace call to RequestUtils.validateRequest with RequestUtils.validateLoginRequest in login API calls, as well as update parameters in all validateRequest calls given the change in the method's signature (isLoginCall param no longer required).

  2. acquireTokenHelper: Remove scope assignment for registerCallback call at the top of the method and add an updated version that simply joins the request scopes, which have already been trimmed, downcased and validated, within the this.registerCallback call arguments.

  3. buildIDTokenRequest Remove client id from request, replace with a new array build from spreading whatever scopes the request already has.

ClientConfigurationError.ts

  1. Remove client_id_input_scopes_error since we're no longer forcing clientId as only scope.

RequestUtils.ts

  1. validateRequest:

    1. Remove isLoginCall boolean parameter from method signature
    2. Remove scopes validation logic (move to new validateLoginRequest).
    3. Add a line that checks whether scopes are/should be login scopes and generatesLoginScopes if true (for access token requests that also obtain an idToken).
    4. Remove extra scopes to consent logic and move to validateLoginRequest.
  2. validateLoginRequest: New method in RequestUtils that wraps validateRequest, only called from login APIs in UserAgentApplication.

    1. Appends extra scopes to consent to scopes array
    2. Forces generation of login scopes (i.e. add 'openid' and 'profile' if they're not already in scopes array).
    3. Calls validateRequest to continue regular request validation.

UrlUtils.ts

  1. createNavigateUrlString:
    1. Remove line that appends clientId to request scopes (this line mutated the scopes)
    2. Remove login scopes "translation" logic.
      • openid and profile are always added, regardless of whether clientId is present or not
      • clientId is not being removed anymore, so it is added as a scope to the request URL string (in accordance to the B2C use case where clientId value is a valid scope).
        2.translateClientIdUsedInScope: Method refactored out into ScopeSet.generateLoginScopes and ScopeSet.isLoginScopes.

lib/msal-core/src/UserAgentApplication.ts Outdated Show resolved Hide resolved
lib/msal-core/src/UserAgentApplication.ts Outdated Show resolved Hide resolved
lib/msal-core/src/utils/RequestUtils.ts Outdated Show resolved Hide resolved
lib/msal-core/src/utils/RequestUtils.ts Outdated Show resolved Hide resolved
lib/msal-core/src/ScopeSet.ts Outdated Show resolved Hide resolved
lib/msal-core/src/ScopeSet.ts Outdated Show resolved Hide resolved
@hectormmg hectormmg requested a review from jmckennon as a code owner June 2, 2020 20:40
Copy link
Contributor

@pkanher617 pkanher617 left a comment

Choose a reason for hiding this comment

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

Great job here. Could maybe add some comments or docs around scopes, but can be done in a separate PR.

Copy link
Contributor

@jasonnutter jasonnutter left a comment

Choose a reason for hiding this comment

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

Great job @Technical-Boy and @jo-arroyo!

Héctor Morales and others added 4 commits July 13, 2020 15:56
* Update UserAgentApplication.ts

refactor and extend getTokenType to remove the need for silentCall boolean and use scopes to determine return types

* Update getTokenType and it's references in UserAgentApplication to correctly implement the scopes to response type mapping table from the design spec

* Add newest draft of response-types doc

Co-authored-by: Jo Arroyo <joliuac@gmail.com>
Co-authored-by: Jo Arroyo <joarroyo@microsoft.com>
@hectormmg hectormmg closed this Jul 23, 2020
@hectormmg
Copy link
Member Author

Closing in favor of going with the solution implemented in #2022. The approach and original goals of this PR were based on a slight misunderstanding of the problem, so the solution isn't appropriate.

@jasonnutter jasonnutter removed this from the msal@1.4.0 - Release milestone Aug 12, 2020
@jo-arroyo jo-arroyo deleted the update-scope-validation branch April 27, 2023 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
msal@1.x Related to msal@1.x (implicit flow)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants