UserStoreBase.SetTokenAsync does not update token in the data store if already present
- Dominant language
- C#
- Stars
- 38.4k
- Forks
- 10.9k
- Avg merge
- 2d 5h
- Merged PRs (30d)
- 276
Description
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Describe the bug
This issue is mostly the same as #29426, but specifically **not** when using EFCore.
The `UserStoreBase.SetTokenAsync()` method does not save changes to actually update the token when called if a token for the same provider-name pair already exists for the current user. Instead, it just changes the value of the token that is found in memory, and then returns.
https://github.com/dotnet/aspnetcore/blob/7c8a4db4d1019bf716e04cf3b782c76297872156/src/Identity/Extensions.Stores/src/UserStoreBase.cs#L828-L844
This design is problematic and non-intuitive if used with another implementation of the user store which is not based on change tracking, for example [ElCamino.AspNetCore.Identity.AzureTable](https://www.nuget.org/packages/ElCamino.AspNetCore.Identity.AzureTable/). This is called out in this comment which received no reply, after which the issue was auto-closed: https://github.com/dotnet/aspnetcore/issues/29426#issuecomment-776457686
In the case of the above NuGet package, the implementation for finding the token just returns a POCO ([code](https://github.com/dlmelendez/identityazuretable/blob/bc4ba07d6289addd275c425d20db3ee19167805f/src/ElCamino.AspNetCore.Identity.AzureTable/UserOnlyStore.cs#L1054-L1058)). The provider does not track changes to the entity after it is constructed, so the provider has no idea there's any change to persist afterwards, and similarly the store provides no indication to the caller that they need to do anything as the method returns nothing, so the assumption is that the write succeeded.
Eventually I realised my changes weren't being persisted as API calls using a refreshed access token were failing and inspected the contents of my Azure table and found they didn't match. I then dug through the code here to determine why things weren't being updated. For the consuming application, this behaviour was not intuitive, and lead to the "obvious" thing to do in the calling application not working and sort-of leading to data corruption (the tokens in the store were now invalid as they'd been refreshed, and the new values were lost after use in the current HTTP request as they weren't persisted to the store).
I've worked around this in my application by instead explicitly removing the token before adding the new one, and on reflection I'll probably tweak it to instead override `SetTokenAsync()`, but this subtle behaviour which then likely requires most non-EFCore based implementations to override this behaviour with 90% the same code to do an update/upsert suggests to me that the original design of this method is a bit flawed.
Changing the behaviour now is probably going to be quite difficult without breaking something, so maybe this is something that can be improved via documentation? For example, the `///` comments for the method (or `SetAuthenticationTokenAsync()`) used by Intellisense give no indication of this behaviour either.
### Expected Behavior
Calling `SetTokenAsync()` (or `SetAuthenticationTokenAsync()`) updates the token in the backing data store as that is the intuitive intended purpose of these methods to the calling application.
### Steps To Reproduce
_No response_
### Exceptions (if any)
_No response_
### .NET Version
7.0.202
### Anything else?
_No response_
Contributor guide
Assessment
This issue has not been assessed yet.