IClaimsTransformation implementation, getting confused about concurrency precautions

30 views Asked by At

I reviewed multiple IClaimsTransformation implementations in github, and more I examine the concurrency issues more getting confused.

Many implementation uses the form:

public Task<ClaimsPrincipal> TransformAsync(ClaimsPrincipal principal)
{
    // create the result principal
    return Task.FromResult(principal);
}

instead of:

public async Task<ClaimsPrincipal> TransformAsync(ClaimsPrincipal principal)
{
    // create the result principal
    return principal;
}

Question, why? Is there any reason of this?

An other thing I noticed, some implementations are completely recreating the principal, (and the identity in it) instead of just adding the claims to the existing principal's existing identity, my question is there anything wrong with "just adding"?

if (<guard against adding claims in case they were already added>) {
    return ...
}
var claims = new List<Claim>()
// add new custom or transformed claims
// ...
// copy existing other claims from the original identity:
claims.AddRange(identity.Claims);
var newIdentity = new ClaimsIdentity(claims, identity.AuthenticationType);
return Task.FromResult(new ClaimsPrincipal(newIdentity));

but some implementations instead of simply just adding the claims:

var identity = principal.Identity as ClaimsIdentity;
if (<guard against adding claims in case they were already added>) {
    return ...
}
// add new custom or transformed claims
identity.AddClaim(...)
// return with the original principal, with the original identity in it:
return Task.FromResult(principa);

Question: Is there any reason of completely recreating the whole thing? Is this about concurrency?

Regarding concurrency, I noticed, the my implementation reenters concurrently. However I am not sure the principal instance which we are getting as parameter is the same instance or not. In case we are different thread, but the principal instance is different instance thread by thread, then we do not have to deal with concurrency, but if that principal instance is the same, then either lock() {} (sounds extreme) either treating the principal as immutable and recreating the whole thing makes sense...

0

There are 0 answers