U4-11087 - The SecurityStamp isn't written to the identity unless the SessionId is missing, this causes some issues for external auth providers

Created by Shannon Deminick 27 Feb 2018, 07:25:53 Updated by Sebastiaan Janssen 28 Jun 2018, 09:28:46

Relates to: U4-11486

There's a public ticket here: https://github.com/umbraco/UmbracoRestApi/issues/42 which describes the issue and is affecting authenticating via the rest api.

This is a regression issue with changes to auth made in 7.8 where we properly track the session id in the db now.

Comments

Stephan 08 Mar 2018, 12:24:12

Issue is: starting with 7.8, the rest API cannot authenticate. It is possible to get a token, but that token is rejected (401).

Tracing UmbracoAuthorizeAttribute leads to UmbracoBackOfficeIdentity.FromClaimsIdentity() which ensures that all claims defined by RequiredBackOfficeIdentityClaimTypes are present. Starting with 7.8, this list now contains SecurityStamp, which is missing.

That claim is added in UmbracoBackOfficeIdentity.AddUserDataClaims() but only when the SessionId claim is not present. However, when the identity is created by BackOfficeClaimsIdentityFactory.CreateAsync, the initial call to base.CreateAsync() returns an identity that ''does'' contain the SessionId. That claim is copied by AddExistingClaims() and therefore prevents the SecurityStamp claim from being added by AddUserDataClaims().

Should potentially affect anything using token-based identity, not only WebAPI. I.e... kinda critical?

Fixed by making sure we always add the SecurityStamp claim: see PR https://github.com/umbraco/Umbraco-CMS/pull/2510.

Review and repro: see https://github.com/umbraco/UmbracoRestApi/issues/42 Should work with 7.7.13, fail with 7.8 and above, and work again with the PR. Tested OK with a basic RestAPI example.

Suggesting @Shandem has a look, as I quite don't know whether it is all OK, or hiding a more complex issue with identities.


Stephan 08 Mar 2018, 12:25:22

Also, when merging, please update/close the issue on GitHub!


Shannon Deminick 12 Mar 2018, 08:12:58

Perfect. The reason why the SecurityStamp wasn't set unless the sessionid was also null is because previously the sessionid was the security stamp and now it is not. So we should have discovered this issue already but i guess we are lucky that we haven't.


Shannon Deminick 12 Mar 2018, 13:28:21

Tested and all working, nice work on figuring this one out, must have been interesting ;)


Priority: Normal

Type: Bug

State: Fixed

Assignee:

Difficulty:

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions: 7.8.0, 7.9.0, 7.8.1, 7.9.1, 7.9.2

Due in version: 7.9.3

Sprint: Sprint 80

Story Points: 1

Cycle: 8