Allow prompt parameter with PAR

[josephdecock]

This fixes a bug that prevents prompt parameters from being used with Pushed Authorization (#1562). However, there are some changes that are right at the edge of how much we should do in a patch release, so we need to review this carefully.

Issue Details

Normally prompt values are used in the authorization interaction response generator, and then "processed". The original parameter is removed and the old parameter value is recorded. However, with pushed authorization, the pushed authorize request is not updated in this process, so we never process the prompt and ultimately the user can't proceed past the UI that that prompt value sends them to.

Solution

Update the par record when we remove the prompt in the interaction response generator.

Issues

Needed New Property in ValidatedAuthorizeRequest

To update the record efficiently using the APIs available today, we need to pass the complete record to the store. By the time we get to the response generator, we no longer the original record, so we recreate it from the data that we have in the validated request. The only piece of data that is missing is the expiration timestamp, so this change adds the par expiration to the validated authorize request (similar to the existing way that we track the par reference value).

No dedicated update method on the store

There's no dedicated update method in the PAR store to make arbitrary updates to a pushed request. We have the existing StoreAsync method, and I'm using that. Our existing store implementation always tries to insert, so I've changed the EF store to instead update if we know of an existing pushed request. I had performance concerns here though - I didn't want to query the database every time we make an update to check if we needed to do an update or insert. So, I'm checking if we're already tracking the pushed request in the dbcontext, since we will have already loaded the pushed request at this point with any usage that we have. It does mean that if you were using the EF par store in some other context where the pushed request hadn't been loaded previously and wanted to make arbitrary updates to the par records, you'd have to go retrieve the record first. I think that slight inconvenience is okay though, because everything about that scenario sounds far-fetched. I'd also appreciate extra attention in review of this EF store update, just because it is a little unusual (at least for me).

The bigger discussion is that this arguably is extending the semantic of the store interface. If someone has a custom PAR store, they very well could run into the same need to change their implementation that I did. I think we should still do the change because a) it only affects you if you are combining PAR and the prompt parameter, which is broken today b) we never said explicitly in the documentation or xmldoc that StoreAsync was only an insert and not an insert or update.

New Dependency in AuthorizeInteractionResponseGenerator

In order for the response generator to update the PAR record, it fundamentally has to take a dependency on the PAR store, which it doesn't do today. This means that anyone with a derived custom response generator will have to make a code change to get this update to compile.

RemovePrompt is an Extension Method

Finally, the RemovePrompt method that does the work of removing the prompt values from the validated request is an extension method. I would have liked to have changed that method to also update the store. But because it is an extension method, there isn't a good way to do this. So for now, whenever you call RemovePrompt you have to remember to also clean up the store. I'd like to add a new service that processes the prompt in the model and also updates the store in a future release. For now, we just have to remember (and I suppose, anyone calling RemovePrompt will have to too).

[brockallen]

Should this be done instead in a major version? Is this currently a blocker for the customer?

[andrew-from-toronto]

Should this be done instead in a major version? Is this currently a blocker for the customer?

This is blocking us on one change we are looking to make. We need the shortened URLs the PAR provides

[brockallen]

Closing in favor of: https://github.com/DuendeSoftware/IdentityServer/pull/1566