IdentityServer icon indicating copy to clipboard operation
IdentityServer copied to clipboard

Minor logic issues in EntityFramework DeviceFlowStore

Open tillig opened this issue 2 years ago • 2 comments

Which version of Duende IdentityServer are you using?

6.0.4

Which version of .NET are you using?

.NET 6

Describe the bug

Something I noticed while trying to use the EntityFramework storage objects as an example so I can create a backing data store in Cosmos DB, both of which are in DeviceFlowStore:

  1. When the ToEntity method converts a DeviceCode to a DeviceFlowCodes entity, it does not set the DeviceFlowCodes.SessionId property. I'm not sure if this is intentional or an oversight.
  2. In UpdateByUserCodeAsync the incoming DeviceCode gets converted to an entity but the converted entity isn't really used; instead, it seems like an end-around to avoid calling the IPersistentGrantSerializer.Serialize method directly. Again, unclear if this is intentional or an oversight.

I don't actually use Entity Framework so while I recognize there's some magic going on with entities getting changed and marked dirty and database contexts determining what changes to save and so on, I'm not sure if that's the reason this stuff is happening.

If it turns out these are oversights, I'd be happy to submit a PR to fix it.

tillig avatar Feb 14 '22 19:02 tillig

Hi, thanks for opening this issue. Just now had time to investigate.

1: Yep, you're right. Also, Description is not being set either. But re-examining the code, ToModel is only ever used on creation and those values won't be present since that's during a backchannel call from the device client.

2: Yes, also a good observation. This is where the SessionId and Description should be set, and I agree the call to ToModel is unnecessary. So a better approach would be to call the serializer directly. The only concern here is that ToModel is virtual, so this would be a breaking change. I know we have people using device flow, but I don't know if they override this API.

brockallen avatar Mar 11 '22 20:03 brockallen

I think I will do a small fix now to update the SessionId and Description. And then for v7.0 re-think the ToModel API.

brockallen avatar Mar 11 '22 20:03 brockallen

Looks like ToModel was used elsewhere to, so I just removed the unused call to it and did the serialization call directly.

brockallen avatar Jun 02 '23 19:06 brockallen