feat: improved prompt handling - #55
Conversation
|
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. |
… 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.
0044a73 to
599cef1
Compare
| Sets the prompt modes that are supported by IdentityServer. | ||
| Defaults to *login*, *consent*, *select_account* and *none*. | ||
| When *CreateAccountUrl* is set, then *create* is also added to the supported prompt modes. | ||
|
|
There was a problem hiding this comment.
Can you add a line here stating that unsupported prompts will cause an authorization request to fail
| public string CreateAccountReturnUrlParameter { get; set; } = Constants.UIConstants.DefaultRoutePathParams.CreateAccount; | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the supported prompt modes. |
There was a problem hiding this comment.
Also add to the summary that prompts not in the supported collection will cause authorization requests to fail
JoStevensRSK
left a comment
JoStevensRSK
left a comment
There was a problem hiding this comment.
Just those two docs, and can you target v3, as this pr is a breaking change
|
@patchandthat and @JoStevensRSK I send you an email today. Have you recieved it? I also sent two e-mails to @JoStevensRSK earlier this week. |
4 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.