Fixing merge

[TiagoBrenck]

Purpose

• Cleaned appsettings
• Removed DisallowsSameSiteNone from WebAppServiceCollectionExtensions because it got moved to another class
• Added edit profile button on login bar
• Fixed merge on AuthorizeForScopesAttribute
• Added options.TokenValidationParameters.NameClaimType = "name" on web api Startup to fix todo list

Does this introduce a breaking change?

[ ] Yes
[x] No

Pull Request Type

What kind of change does this Pull Request introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[x] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ ] Other... Please describe:

[TiagoBrenck]

@jmprieur @jennyf19 I have a few observations that I would like to discuss:

1- We have some sort of method duplication in AuthorityHelpers.cs IsV2Authority(string authority) and WebApiServiceCollectionExtensions.cs EnsureAuthorityIsV2_0(JwtBearerOptions options). Can we use just one? If so, which one?

2- Could you confirm if the following behavior is expected?

• User signs-in > click on edit profile > save > click on TodoList > MsalUIException is throw.
• Repeating this process results in the same behavior. Shouldnt we have an IAccount for edit_profile at this point?

3- For B2C, the remove account might have a bug. Since one user can have many IAccounts (because of the policies), we need to find a solution that gets all of the accounts of that user (regardless the policy) and remove from the cache. The current implementation is not doing it.

4- The current port used in the client app is localhost:5000. This port is not aligned with the Readme. Should we update the Readme or the port?

[jmprieur]

LGTM
Thanks @TiagoBrenck

[jmprieur]

@TiagoBrenck : we'll answer your questions.

[jmprieur]

LGTM.
Thanks @TiagoBrenck

We also need to update the Web APIs so that they check the scopes (they don't at the moment)

[TiagoBrenck]

LGTM.
Thanks @TiagoBrenck

We also need to update the Web APIs so that they check the scopes (they don't at the moment)

Are you talking about this check?

                options.Events.OnTokenValidated = async context =>
                {
                    // This check is required to ensure that the Web API only accepts tokens from tenants where it has been consented and provisioned.
                    if (!context.Principal.Claims.Any(x => x.Type == ClaimConstants.Scope)
                    && !context.Principal.Claims.Any(y => y.Type == ClaimConstants.Scp)
                    && !context.Principal.Claims.Any(y => y.Type == ClaimConstants.Roles)
                    && !context.Principal.Claims.Any(y => y.Type == ClaimConstants.Role))
                    {
                        throw new UnauthorizedAccessException("Neither scope or roles claim was found in the bearer token.");
                    }

                    await tokenValidatedHandler(context).ConfigureAwait(false);
                };

[jmprieur]

No, this verifies that there are either roles or scopes, but we also need to have the controller actions verify the scopes.
See for example https://github.com/Azure-Samples/active-directory-dotnet-native-aspnetcore-v2/blob/bd5bd598b980fa92da80b88008c748320cc5ab87/1.%20Desktop%20app%20calls%20Web%20API/TodoListService/Controllers/TodoListController.cs#L35

The doc is https://docs.microsoft.com/en-us/azure/active-directory/develop/scenario-protected-web-api-verification-scope-app-roles

[jmprieur]

@TiagoBrenck. here are the answers to your questions
cc: @jennyf19

1- We have some sort of method duplication in AuthorityHelpers.cs IsV2Authority(string authority) and WebApiServiceCollectionExtensions.cs EnsureAuthorityIsV2_0(JwtBearerOptions options). Can we use just one? If so, which one?

I've extracted EnsureAuthorityIsV2_0 from the AddXX method so that we can unit test it. I think it should stay there. It's unit tested. it's also more robust as it process correctly authorities that end in / or not. We cannot, tough use EnsureAuthorityIsV2 as is in the AddSignIn method (which is not robust), as it takes a JwtBearerOption. I've raised issue AzureAD/microsoft-identity-web#19 to fix this.

2- Could you confirm if the following behavior is expected?

• User signs-in > click on edit profile > save > click on TodoList > MsalUIException is throw.
• Repeating this process results in the same behavior. Shouldnt we have an IAccount for edit_profile at this point?

I think it's fine (it works functionally?). We might want to raise an issue in the sample to fix it later.

3- For B2C, the remove account might have a bug. Since one user can have many IAccounts (because of the policies), we need to find a solution that gets all of the accounts of that user (regardless the policy) and remove from the cache. The current implementation is not doing it.

Good catch. I've raised an issue for this AzureAD/microsoft-identity-web#20

4- The current port used in the client app is localhost:5000. This port is not aligned with the Readme. Should we update the Readme or the port?

Let's update the documentation (we have the app registration right)

[jmprieur]

A few suggestions;

[jmprieur]

Would we be able to specialize OperationAuthorizationRequirement to ScopeRequirement and have a property named RequiredScope ?

How do we allow several scopes (for instance a given method can be called if the scope is user.read or user.write?

[jmprieur]

I'd like to be able to write new OperationScopeRequirement {RequiredScope = "user.read"}

[jennyf19]

LGTM.
left a few suggestions

[jmprieur]

Thanks @TiagoBrenck
LGTM.