Feat/improved prompt handling - #55
Conversation
… test for max_age=0.
Added RemoveMaxAge to handle max_age the same way.
…eping the values otherwise so that the login page knows way login is shown.
…ocessed and should not re-trigger login so that it will also work with request objects.
Explained the usage of the processed parameters.
|
I hope that I have fixed all your comments now. I'm sorry for missing unit tests and I'll do better next time :) |
| // then we don't want to prompt the user again, so skip handling of the parameter | ||
| var promptProcessed = request.Raw.Get(Constants.ProcessedParameters.PromptProcessed); | ||
|
|
||
| if (!promptProcessed.IsPresent()) |
There was a problem hiding this comment.
Can we remove this PromptProcessed check.
The whole request is meant to be replayed and validated once authentication has taken place. If the login took more time than the max age then it should be failed. It also complicates the flow and requires additional constant values.
|
@equist Jo has some outstanding feedback. Can you act on it? Or would you like a call to agree on a plan so we can commit to this work? We really appreciate your contribution. |
I'm waiting for you to give me feedback on my last comment. I find it hard to act on the original feedback, since the suggested solution will result in impossible to sign-in paths for end users as I explained above. |
3 participants
Description
Improvments of prompt and max_age parameter handling.
Type of change
[ ] Bug fix
[x] Feature
[ ] Refactoring
[ ] Documentation
[ ] Other
Implementation of feature #54.
Does this PR introduce a breaking change?
[x] Yes
[ ] No
The old code ignored unsupported prompt modes, but this implementation returns an error to the client.
Testing
I have fixed an error in an existing integration test.
There are several new integration tests.
LLM Usage
I have not used LLM, except for the extended auto-complete feature primarly used for XML comments.
I used co-pilot to set up some structure for the IdentityServerApplicationBuilderExtensionsTests class.
Other context
I believe this implementation is better aligned with current Duende IdentityServer implementation, but it is a breaking change compared to IdSrv 4 code base which ignored unsupported prompt values.