IdentityServer / IdentityServer3.AspNetIdentity

ASP.NET Identity support for Thinktecture IdentityServer3
Apache License 2.0
64 stars 51 forks source link

Update claims from external provider #36

Closed huysentruitw closed 9 years ago

huysentruitw commented 9 years ago

When the service processes an external identity, the claims are not updated.

Lets say we use ADFS WS-Federation as external identity provider and an existing user logs in for the second time but with updated claims (because the user account in Active Directory was updated), then it is desired that the claims in the database are also updated.

Furthermore, UpdateAccountFromExternalClaimsAsync is only called in case of a new external identity and not in case of a returning external identity.

If you don't mind I would prefer to create a PR for this rather then running our own implementation. What do you think?

brockallen commented 9 years ago

Sure, but there's already the ProcessExistingExternalAccountAsync you can override.

huysentruitw commented 9 years ago

Yeah we can override things and implement it, but I think this is basic functionality others might also benefit from.

brockallen commented 9 years ago

Sure, send a PR then.

brockallen commented 9 years ago

Any update?

huysentruitw commented 9 years ago

Was on my backlog and has now been added to my sprint. I'll probably start working on this tomorrow.

brockallen commented 9 years ago

Update?

kvoyk commented 9 years ago

I would like to have this functionality too. To make it work for now I overrode ProcessExistingExternalAccountAsync and modified UpdateAccountFromExternalClaimsAsync to keep claims in sync. Is it right place to do it?

    protected override async Task<AuthenticateResult> ProcessExistingExternalAccountAsync(string userID, string provider, string providerId, IEnumerable<Claim> claims)
    {
        await UpdateAccountFromExternalClaimsAsync(userID, provider, providerId, claims);
        return await SignInFromExternalProviderAsync(userID, provider);
    }
    protected override async Task<AuthenticateResult> UpdateAccountFromExternalClaimsAsync(string userID, string provider, string providerId, IEnumerable<Claim> claims)
    {
        var existingClaims = await userManager.GetClaimsAsync(userID);
        var intersection = existingClaims.Intersect(claims, new ClaimComparer());
        var newClaims = claims.Except(intersection, new ClaimComparer());
        var oldClaims = existingClaims.Except(intersection, new ClaimComparer());

        foreach (var claim in newClaims)
        {
            var result = await userManager.AddClaimAsync(userID, claim);
            if (!result.Succeeded)
                return new AuthenticateResult(result.Errors.First());
        }
        foreach (var claim in oldClaims)
        {
            if (claim.Issuer.Equals("LOCAL AUTHORITY", StringComparison.InvariantCultureIgnoreCase))
                continue;

            var result = await userManager.RemoveClaimAsync(userID, claim);
            if (!result.Succeeded)
                return new AuthenticateResult(result.Errors.First());
        }
        return null;
    }
huysentruitw commented 9 years ago

Sorry for the delay: we rewrote the implementation since we wanted to keep the link between external claim and external provider. Because the user could have changed his e-mail address for his local account and you don't want to override that when the e-mail address from the external provider changes.

AusRob commented 9 years ago

Hi folks, We're struggling with this exact scenario - any word on when we might see this supported? R

brockallen commented 9 years ago

@AusRob you have the user service -- in 2.0 this is a code file from nuget -- you can modify it (or derive) and modify it any way you want.

AusRob commented 9 years ago

HI @brockallen .. I did an implementation similar to @kvoyk 's with no issues. I'm worried that the default functionality of only storing a user's claims when they first authenticate (and not refreshing ever) might be a bit counter intuitive to folks who aren't expecting this behaviour?

brockallen commented 9 years ago

@AusRob different people have different requirements shrug

huysentruitw commented 9 years ago

@AusRob at first sight yes. But when you think of it, when the user changes his/her profile on your webapp, you don't want to override that e-mail address the next time the user signs in using an external identity provider.

We are now keeping external claims in a separate table and each claim has a foreign key to the external provider. This way we never loose any information.

As brockallen says: different people, ... :)