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

Cannot manually add role claims from AAD and complete user sign-in successfully to Oqtane #2432

Closed
ptzedakis opened this issue Sep 25, 2022 Discussed in #2405 · 7 comments

Comments

@ptzedakis
Copy link

Discussed in #2405

Originally posted by ptzedakis September 7, 2022
Hi everybody, I am Panos and I am quite new to Oqtane. :)

I am using the Azure AD IdP to sign-in users to my Oqtane installation (External Login).
I have setup the relevant Azure app registration to return a user's AD group membership as role claims in the ID token returned by AAD.

I have not found an OOB way for Oqtane to retrieve the additional role claims (groups) from the IdP, so I added some code in the OnTokenValidated method the OqtaneSiteAuthenticationBuilderExtensions static class to populate the sign-in user's identity with the additional 'role' claims that I need.

However, when I manually add the 'role' claims to the "identity" Oqtane does not complete the login and returns the user to the login screen.

If I add a bogus claim to the "identity" of the sign-in user, everything works fine! Hmm.

non-working code (user does not login to Oqtane site)

    ...
        // validate user
        var identity = await ValidateUser(email, id, claims, context.HttpContext);
        if (identity.Label == ExternalLoginStatus.Success)
        {
            // NOTE: The following code adds the role claims but the user is not logged in and no error is generated.
            foreach (var claim in context.Principal.Claims.ToList().FindAll(p => p.Type == ClaimTypes.Role))
            {
                identity.AddClaim(new Claim(claim.Type, claim.Value));
            }

            identity.AddClaim(new Claim("access_token", context.SecurityToken.RawData));
            context.Principal = new ClaimsPrincipal(identity);
        }

working code (bogus claim gets successfully added to context.Principal and user is successfully logged on to the site)

    ...
        // validate user
        var identity = await ValidateUser(email, id, claims, context.HttpContext);
        if (identity.Label == ExternalLoginStatus.Success)
        {
            identity.AddClaim(new Claim("bogus", "my-bogus-claim-value"));

            identity.AddClaim(new Claim("access_token", context.SecurityToken.RawData));
            context.Principal = new ClaimsPrincipal(identity);
        }

What I am doing wrong?

Thank you for your time to help me clarify this.

@sbwalker
Copy link
Member

sbwalker commented Sep 26, 2022

The current auth implementation was designed to only support internal role assignments (and it appears you are trying to make it work for external role assignments). The framework has a PrincipalValidator class which contains a ValidateAsync() method. Its purpose is to validate that the principal is valid for the current site (which is required as Oqtane is multi-tenant). As part of this validation it loads the list of roles assigned to the user from the database and rebuilds the principal. I am guessing that this logic is overwriting your custom role assignments.

@sbwalker
Copy link
Member

After researching this further I believe it should be possible to enable external role assignment while still providing multi-tenant validation.

@sbwalker
Copy link
Member

@ptzedakis quick question... were you hoping to use your AAD role claims INSTEAD OF the internal role assignments in Oqtane... or IN ADDITION TO the internal role assignments in Oqtane?

@sbwalker
Copy link
Member

sbwalker commented Sep 29, 2022

@ptzedakis I am not so sure now that it was the PrincipalValidator causing the issue for you. Is it possible that this was caused by duplicate role claims? The external login process will automatically create users and assign them to the internal Registered Users role. I am not sure what role claims were being passed by your AAD provider but if it was passing a "Registered Users" role claim then there would be a duplicate, and the logic you added was not validating this condition. Perhaps you should try logic such as below:

                    foreach (var claim in context.Principal.Claims.Where(item => item.Type == ClaimTypes.Role))
                    {
                        if (!identity.Claims.Any(item => item.Type == claim.Type && item.Value == claim.Value))
                        {
                            identity.AddClaim(new Claim(claim.Type, claim.Value));
                        }
                    }

@sbwalker
Copy link
Member

@ptzedakis the other possible issue could be the role type names used in the token returned by AAD. By default the constant value for ClaimTypes.Role is "http://schemas.microsoft.com/ws/2008/06/identity/claims/role". However many Identity Providers do not follow this standard and actually use a different Type name for their roles. So your logic of context.Principal.Claims.ToList().FindAll(p => p.Type == ClaimTypes.Role) may actually return nothing if AAD does not use default role claim type names. Which means that your roles will not be added to the ClaimsIdentity. However if this was the case the user should still be logged in as they would already have the Registered Users internal role assigned to them.

sbwalker added a commit that referenced this issue Sep 29, 2022
fix #2432 - add support for roles as part of external login via OIDC
@sbwalker
Copy link
Member

After reviewing the documentation:

https://github.com/Azure-Samples/active-directory-aspnetcore-webapp-openidconnect-v2/blob/master/5-WebApp-AuthZ/5-1-Roles/README.md

A few code changes were required to the framework:

In order to support older legacy applications, .NET Core attempts to "re-map" claims in the JWT token to use different names. This can cause problems if you are not aware of this unexpected internal behavior (a good article which explains this problem is here: https://mderriey.com/2019/06/23/where-are-my-jwt-claims/). Note that this a breaking behavioral change for sites which may have been using the re-mapped claim names in their configuration - however I believe it is the right decision to avoid future support issues.

            // prevent remapping of claims
            JwtSecurityTokenHandler.DefaultMapInboundClaims = false;

The roles in a JWT token are actually a collection (ie. "roles": ["Admin", "SuperUser"]) which are part of a specific claim. The OIDC configuration needs to know which specific claim to parse in order to create the Principal.Claims collection (a good article which describes this is here: https://www.jerriepelser.com/blog/using-roles-with-the-jwt-middleware/) which requires the specification of the RoleClaimType:

                        options.TokenValidationParameters.RoleClaimType = "roles";

In order to support this I added a new option to the User Settings - External Login for Role Claim Type. It is optional, but if it is specified it will be used for the RoleClaimType specification above and will also enable the logic for the addition of external roles to the ClaimsIdentity.

@ptzedakis
Copy link
Author

Hi Shaun, thank you for your help and effort with regards to my query. I shall review the material you sent and go at it again. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants