search

DuendeSoftware / IdentityServer

The most flexible and standards-compliant OpenID Connect and OAuth 2.x framework for ASP.NET Core
https://duendesoftware.com/products/identityserver
Other
1.45k stars 337 forks source link

Consider refresh method on IBackChannelAuthenticationRequestStore #1588

Open AndersAbel opened 2 weeks ago

AndersAbel commented 2 weeks ago

The CIBA flow can be used for interacting with eid-solutions such as Mobilt BankID in Sweden or MitID in Denmark. When using those the status of an authentication request is acquired through an external API call. The naive implementation would be to make a custom IBackChannelAuthenticationRequestStore that calls the external API on every Get* operation to ensure that up to date data is returned.

The drawback with this is that the external API is called even before basic client validation and throttling rules have been applied. There should be a way to run the basic validation and throttling code first and only after those pass call the external API.

An implementation suggestion is to add a RefreshState() method to the IBackChannelAuthenticationRequestStore. It would be invoked by the BackchannelAuthenticationRequestIdValidator after basic validation (client binding, lifetime and throttling) have been done but before the IsComplete flag is checked. The default implementation would be a no-op.

brockallen commented 1 week ago

This is a breaking change. I do see/understand the idea of adding a RefreshState method, but that feels a tad too implicit. I'd prefer making this more first class.

What would be better is for the IBackChannelAuthenticationRequestStore to really break down the BackChannelAuthenticationRequest data into two halves -- the polling related data and the user related response. This would then need two APIs to load this data. One is cheap, one is expensive.

Is this worth re-architecting this? Also, could the IBackChannelAuthenticationRequestStore not be so naive, and instead use caching and check the HTTP response headers to know how stale its own data is (etag or whatnot)?

AndersAbel commented 1 week ago

My suggested design was intended to not be a breaking change. If the status is stored together with the other information in the database the current flow would work. This would be a way to offer a separation for those cases where it's needed.

For the implementation it would have to be special (possibly derived) interface that contains the new method to not be a code level breaking change.

brockallen commented 1 week ago

But the proposed change would be a leaky abstraction, I think.

EternamFr commented 1 week ago

Hi, to implement the polling, we went ahead and injected our own BackchannelAuthenticationRequestIdValidator.cs, checking the status of the authentication (calling a 3rd party) here: https://github.com/DuendeSoftware/IdentityServer/blob/f01e45ff6a4e4bd2bd38406c6ee59aa6f8c1f081/src/IdentityServer/Validation/Default/BackchannelAuthenticationRequestIdValidator.cs#L77

It feels like a good fit to call the identity providers we integrate with, when our RPs are calling our /token endpoint as the moment to check the status of an authentication, instead of coming up with our own scheduling/polling mechanism on the side. Specially because some of these IDPs charge for every call we make to them, so we don't want to inquire the status of an authentication if the RP is not going to call /token for that login anymore (*).

We first tried to add that status check call under our own IBackChannelAuthenticationRequestStore.GetByInternalIdAsync() but ran into the issue described in the op: we loose all the validation (client binding, lifetime and throttling). Also, from that store, you can't call (step 5 of your doc) IBackChannelAuthenticationInteractionService.CompleteLoginRequestAsync(), because you run into cyclic reference issues.

Anyway, fiddling with our own BackchannelAuthenticationRequestIdValidator feels like fighting Duende and we see it only as a temporary implementation.

(*): We had to come with our own cancel flow, as some IDPs are national security systems and they only allow one login at a time for a given user, so if a user starts a login with us but changes their mind and try to login to another site, they will get blocked for 5 minutes because the 1st (incomplete) login was not properly cancelled.

@brockallen I'm not sure I understand what you are proposing by "Also, could the IBackChannelAuthenticationRequestStore not be so naive, and instead use caching and check the HTTP response headers to know how stale its own data is (etag or whatnot)?".