Oidc-client-js: sub validation regression in 1.3.0

[sveinpg]

After updating to 1.3.0 we have a problem with the sub validation. In our id_token we have the following sub:

"sub": [ "2b6d2f5c9e704bb5b4424a35269f0797", "2b6d2f5c-9e70-4bb5-b442-4a35269f0797" ]

Our userinfo endpoint in IdentityServer only returns the second sub (the guid). Is it intentional that the validation fails in this case? It fails because of a non-equal check between a string and an array.

[lmoe]

Our subject is set to a simple string, equally on both sides. We had this error today too. The only solution was to revert back to version 1.2.2.

[brockallen]

One question I have is why is your subject claim in two different formats? If you put one format in the id token and a different value from user info endpoint, this client side library won't really know that they're meant to be the same value.

[brockallen]

I just tried to repo your issue. I don't get duplicate claims when the value is in both the id_token and from the user info endpoint. If your claim values aren't unique, then you will see the behavior you see above. In short, your STS should not be generating two different sub values for the same user.

Am I misinterpreting the scenario or problem?

[ghost]

I am having the same issue with 1.3.0... Locked version to 1.2.2 and everything was back to normal. In my case STS generates a single sub for a user (same format and everything), which should be fine since it matches in both sides.

I still need to track down the issue further but reverting to 1.2.2 definitely fixes it.

[sveinpg]

@brockallen You were right. Removing the additional subject claim resolved our issue. The sub validation is now working with 1.3.0 in our case.

Our issue was in our UserService. We returned an AuthenticateResult with an account's SubjectId that was formatted as a string not a guid.

[brockallen]

@lmoe and @manurosa any update? As you can see, @sveinpg's issue was that he was providing two different values for sub. Perhaps you have the same issue?

[ghost]

@brockallen Nope. I've got a single value for sub (properly formatted guid), but keep getting the sub from user info endpoint does not match sub in id_token message in 1.3.0.

Will try get some time to look into it this evening.

[lmoe]

Yep, same as @manurosa we use a simple string and it gives out the same message. Debugging right now is not so easy in our current stack, I'll try to figure something out tomorrow.

[brockallen]

Nope. I've got a single value for sub (properly formatted guid), but keep getting the sub from user info endpoint does not match sub in id_token message in 1.3.0.

This was one of the features I added to pass the OIDC RP conformance tests. This tells me your token service might be doing something wrong here. And so yes, please debug and get back to me so we can figure out where the issue is.

[ghost]

Ok so as I suspected the problem is on my side. It has to do with the userinfo response for certain tokens (which I reckon weren't being checked against in v1.2.2). I'll fix it in the sts side as you said this complies with OIDC RP?

However, just so you know, though the log points to the id_token it is validating the access_token. I don't reckon you'll consider that worth of a PR from my side.

Cheers

[brockallen]

Yes, this library is certified: https://openid.net/certification/#RPs

[brockallen]

However, just so you know, though the log points to the id_token it is validating the access_token. I don't reckon you'll consider that worth of a PR from my side.

What do you mean by this? Perhaps it's just a bug in the logging? Sure, if you think it's a bug, feel free to send a PR or point me to where it's wrong.

[BrandonBoone]

@manurosa If you have a minute, could your provide some more details on your fix? Sounds like I may have the same issue. Thanks!

[ghost]

@BrandonBoone you need to make sure your STS is returning the sub claim __from the userinfo endpoint__. This is what the subject is being checked against and possibly the source of your problem. Should be easy enough to test, hope it helps :grin:

[CraigEaston]

I'm still getting this error when I upgrade to 1.3, if I use 1.2.2 I don't get the issue.

I must be setting my claims improperly in my ExternalRegistrationUserService?

Does anyone have a suggestion? Here is my code:

• So do I need add an additional 'sub' claim to my claims list, that is the same as my user.subject?
• Am I missing something with claim mappings?

public override Task AuthenticateExternalAsync(ExternalAuthenticationContext context)
{
         var expiredUser = Users.SingleOrDefault(x => x.Provider == context.ExternalIdentity.Provider && x.ProviderID == context.ExternalIdentity.ProviderId);

         var user = Users.SingleOrDefault(x => x.Provider == context.ExternalIdentity.Provider && x.ProviderID == context.ExternalIdentity.ProviderId);
         string userName = string.Empty;
         string fullName = "Unknown";
         string email = "[email protected]";
         string givenName = string.Empty;
         string familyName = string.Empty;
         string employeeID = string.Empty;
         string phoneNumber = string.Empty;
         if (user == null)
         {
            // new user, so add them here
            string newUserSubject = Guid.NewGuid().ToString();

            var userNameClaim = context.ExternalIdentity.Claims.First(x => x.Type == "winaccountname");
            if (userNameClaim != null) userName = userNameClaim.Value;

            var nameClaim = context.ExternalIdentity.Claims.First(x => x.Type == Constants.ClaimTypes.Name);
            if (nameClaim != null) fullName = nameClaim.Value;

            var emailClaim = context.ExternalIdentity.Claims.First(x => x.Type == Constants.ClaimTypes.Email);
            if (emailClaim != null) email = emailClaim.Value;

            var givenNameClaim = context.ExternalIdentity.Claims.First(x => x.Type == Constants.ClaimTypes.GivenName);
            if (givenNameClaim != null) givenName = givenNameClaim.Value;

            var familyNameClaim = context.ExternalIdentity.Claims.First(x => x.Type == Constants.ClaimTypes.FamilyName);
            if (familyNameClaim != null) familyName = familyNameClaim.Value;

            var employeeIDClaim = context.ExternalIdentity.Claims.First(x => x.Type == "EmployeeID");
            if (employeeIDClaim != null) employeeID = employeeIDClaim.Value;

            var phoneNumberClaim = context.ExternalIdentity.Claims.First(x => x.Type == Constants.ClaimTypes.PhoneNumber);
            if (phoneNumberClaim != null) phoneNumber = phoneNumberClaim.Value;

            user = new CustomUser
            {
               Subject = newUserSubject,
               Provider = context.ExternalIdentity.Provider,
               ProviderID = context.ExternalIdentity.ProviderId,
               Claims = new List<Claim> {
                  new Claim(System.Security.Claims.ClaimTypes.WindowsAccountName, userName),
                  new Claim(Constants.ClaimTypes.Name, fullName),
                  new Claim(Constants.ClaimTypes.Email, email),
                  new Claim(Constants.ClaimTypes.GivenName, givenName),
                  new Claim(Constants.ClaimTypes.FamilyName, familyName),
                  new Claim("EmployeeID", employeeID),
                  new Claim(Constants.ClaimTypes.PhoneNumber, phoneNumber)
               }
            };
            List<Claim> roleClaims = new List<Claim>();
            foreach (Claim adGroup in context.ExternalIdentity.Claims.Where<Claim>(w => w.Type == Constants.ClaimTypes.Role))
               roleClaims.Add(new Claim(adGroup.Value, "true"));

            user.Claims.AddRange(roleClaims);

            Users.Add(user);
         }

         context.AuthenticateResult = new AuthenticateResult(user.Subject, fullName, identityProvider: user.Provider);
         return Task.FromResult(0);
      }

Here is my GetProfileData method:

public override Task GetProfileDataAsync(ProfileDataRequestContext context)
      {
         string key = GetKey(context.Subject, context.RequestedClaimTypes);
         IEnumerable<Claim> tempClaims = cache.GetAsync(key).Result;
         if (tempClaims == null)
         {
            //issue the claims for the user
            var user = Users.SingleOrDefault(x => x.Subject == context.Subject.GetSubjectId());
            if (user != null)
            {
               context.IssuedClaims = user.Claims.Where(x => context.RequestedClaimTypes.Contains(x.Type));
            }
         }
         else
            context.IssuedClaims = tempClaims;

         return Task.FromResult(0);
      }

[CraigEaston]

I think I just figured it out, I needed to add the subject claim to my list of claims.
In my AuthenticateExternalAsync method above, when I add my claims for the user, I added the noted line:

 user = new CustomUser
            {
               Subject = newUserSubject,
               Provider = context.ExternalIdentity.Provider,
               ProviderID = context.ExternalIdentity.ProviderId,
               ExpiresOn = newExpire,
               Claims = new List<Claim> {
       ------>new Claim(Constants.ClaimTypes.Subject, newUserSubject), <--------
                  new Claim(System.Security.Claims.ClaimTypes.WindowsAccountName, userName),
                  new Claim(Constants.ClaimTypes.Name, fullName),
                  new Claim(Constants.ClaimTypes.Email, email),
                  new Claim(Constants.ClaimTypes.GivenName, givenName),
                  new Claim(Constants.ClaimTypes.FamilyName, familyName),
                  new Claim("EmployeeID", employeeID),
                  new Claim(Constants.ClaimTypes.PhoneNumber, phoneNumber)
               }
            };

[brockallen]

Looks like everyone's all set now on this issue. I'll close, since it seems that the issues were in how the claims were being done in IdSvr. Sorry the improvements in this library surfaced the issues elsewhere!

[ghost]

@brockallen Don't be, almost makes me feel better to see sts issues is a more common thing than I expected

[rosdi]

@CraigEaston Thanks for the code sample...