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

AuthorizeForScopesAttribute not detecting MsalUiRequiredException #398

Closed
1 of 8 tasks
cbrianball opened this issue Aug 4, 2020 · 2 comments
Closed
1 of 8 tasks
Assignees
Labels
bug Something isn't working fixed P1
Milestone

Comments

@cbrianball
Copy link

cbrianball commented Aug 4, 2020

Which version of Microsoft Identity Web are you using?
Note that to get help, you need to run the latest version.
v0.2.1-preview

Where is the issue?

  • Web app
    • Sign-in users
    • Sign-in users and call web APIs
  • Web API
    • Protected web APIs (validating tokens)
    • Protected web APIs (validating scopes)
    • Protected web APIs call downstream web APIs
  • Token cache serialization
    • In-memory caches
    • Session caches
    • Distributed caches
  • Other (please describe)

Is this a new or an existing app?
Adding this library to an existing application. The version of this application that is in production is not using this library yet.

Expected behavior
The token has expired (or has been deleted) from the distributed cache. On the next request where the token is needed, the user should be redirected through the authentication flow (since a MsalUiRequiredException is thrown).

Actual behavior
The exception passes through the AuthorizeForScopesAttribute and is not acted upon

Possible solution
The current implementation of AuthroizeForScopesAttribute only looks at the current exception and its inner exception. It should look through all of the nested exceptions to determine if it is caused by MsalUiRequiredException.

I can write my own attribute that inherits from AuthorizeForScopesAttribute, perform the test myself, then call the base method (making sure the MsalUiException is set to the context.Exception property), and everything works as expected.

Additional context / logs / screenshots
image

The order of the screenshot is the reverse nested order (the first exception listed is the innermost exception).

I can submit a PR if that would help, but wanted to double-check to make sure I wasn't doing anything wrong first.

@jmprieur jmprieur self-assigned this Aug 4, 2020
jmprieur added a commit that referenced this issue Aug 4, 2020
@jmprieur jmprieur added bug Something isn't working P1 labels Aug 4, 2020
@jmprieur jmprieur added this to the [3] Fundamentals milestone Aug 4, 2020
jmprieur added a commit that referenced this issue Aug 4, 2020
* Fix for #398

* Adding unit tests
@jmprieur jmprieur added fixed and removed In progress labels Aug 4, 2020
@jmprieur
Copy link
Collaborator

jmprieur commented Aug 5, 2020

Thanks for reporting @cbrianball
It is fixed and we'll release an update of Microsoft.Identity.Web with many bug fixes. No ETA yet as we need to release MSAL.NET first (which also has a fix for us)

@jennyf19
Copy link
Collaborator

jennyf19 commented Aug 7, 2020

Included in 0.2.2-preview release

@jennyf19 jennyf19 closed this as completed Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed P1
Projects
None yet
Development

No branches or pull requests

3 participants