SessionMigration sample broken with regard to expiration calculation

[ArturDorochowicz]

I think the CreateCookieOptions method in the SessionMigration sample is broken. It turns persistent tickets/cookies into session cookies. Same issue in v7 and v8.

https://github.com/DuendeSoftware/samples/blob/9ab54a3b15e3bc3d630a430ff3729f29bbff7bfd/IdentityServer/v8/SessionMigration/MigratingTicketDataFormat.cs#L87

The method takes the new ticket with empty properties and calculates cookie expiration if Properties.IsPersistent. But that is always false, because these are properties from the new ticket, not the original one. The code also contains setting ticket expiration, but that's wrong and pointless as this ticket should not have it and it has already been serialized.

[wcabus]

Hi @ArturDorochowicz, thanks for raising this. I've prepared a pull request to update the sample in case you already want to have a look before it gets merged.

[ArturDorochowicz]

Thank you. I'd question if there's a need to be setting properties on the new ticket. Only because what seems to be the relevant code in Asp.Net Core just passes null [1]. This ticket seems to be pure data storage, the claims identity is not authenticated either (options.ClaimsIssuer passed as authentication type is typically going to be null).

[1] https://github.com/dotnet/aspnetcore/blob/294cab2f9b2e03af6b953820c7ab497c3c8b7ad9/src/Security/Authentication/Cookies/src/CookieAuthenticationHandler.cs#L350-L354

[wcabus]

I was indeed a bit overzealous: the new cookie doesn't need the AuthenticationProperties since those are stored in the server-side session now, so the AuthenticationTicket within the session cookie can indeed be created without the properties.

When creating the ClaimsPrincipal however, we cannot assume that Options.ClaimsIssuer is a null value, so it is safer to mimic the original behavior there.

[ArturDorochowicz]

When creating the ClaimsPrincipal however, we cannot assume that Options.ClaimsIssuer is a null value, so it is safer to mimic the original behavior there.

Yes, of course. I only meant the fact that typically the identity's IsAuthenticated=false as an argument to back up the idea that this ticket is just storage for the session id and nothing else in it matters.