[Bug] RolesRequiredHttpContextExtensions.ValidateAppRole will only check first role claim and fail when there are many role claims

[adriannasui]

Hi, I encountered an issue on version 0.2.1-preview where calling ValidateAppRole will not work as expected.

Issue happens when i call ValidateAppRole and my access token has a 'roles' claim containing an array of roles. Somwhow, my 'roles' claim gets transformed into multiple 'role' claims in context.HttpContext.User. And the code inside ValidateAppRole is not properly written to handle this scenario, it only looks at the first 'role' claim and my call fails with 403.

This is the line where I found the issue.

microsoft-identity-web/src/Microsoft.Identity.Web/Resource/RolesRequiredHttpContextExtensions.cs

Line 51 in f6c3110

rolesClaim = context.User.FindFirst(ClaimConstants.Role);

This code will incorrectly stop on the first role claim.

Please let me know it any more info is needed, I could even do a PR to solve this, I will do a quick custom implementation of ValidateAppRole just to move on with my project.
Thanks

[jennyf19]

@adriannasui thanks you. we definitely welcome outside contributions, so feel free to propose a PR.

[jennyf19]

@adriannasui We are thinking of throwing now if the roles do not match, verses returning to the controller, because the controller could continue to do things it shouldn't at that point. We are still looking at the user experience here, but if you have thoughts on this, let us know. Changes are in this branch. thank you.

[adriannasui]

sorry about not answering, seems my github notifications are not properly setup on my part.
About throwing I suspect for the caller it looks like a 403 (Forbidden) with a custom message in both cases (throw vs no throw).
Throwing seems safer to me, it conveys the fact that stopping at this point is mandatory due to authorization issues.
Was looking at your code and it looks fine, a bit slimmer than my implementation, so I do not plan on doing a PR at this point.
Thanks
Adrian

[jennyf19]

Thanks for the quick feedback @adriannasui ....we agree that throwing seems safer and less "magical"

[jennyf19]

Included in 0.2.2-preview release