-
Notifications
You must be signed in to change notification settings - Fork 338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prevent infinite loop when max_age=0 #1544
Conversation
Thanks for this contribution! I agree that the max_age=0 case is bugged, and removing the max_age after we've used it is a good way to fix it. I do think it would be a good idea to only remove in the 0 case so that we can release this quickly. There could be a customization using the max_age in the non-zero case (perhaps in a customized interaction response generator). If we change IdentityServer to always remove max_age, that's technically a breaking change which would have to wait for the next major release. But, since the 0 case is already broken, we can release a bug fix as a patch release. One other thing to consider is that when we remove prompt parameters, we track what the old value was. @brockallen do you think we should do the same here? And finally, we need to add some test automation. Once we get all that in place, we can release this as a patch release. |
Can we reuse the prompt=login approach, since max_age=0 is logically the same? Or is that too clever? If we don't like that, then we'd need some original_max_age? |
The OIDC spec does say
So I was thinking along those lines. Maybe max_age=0 becomes prompt=login during validation? But it does feel very "clever". |
IMO keeping the old value as suppressed_prompt=login is very ugly and confusing. I have added a commit to keep the old value in a suppressed_max_age parameter. I have also added a condition to remove the parameter only in case when max_age=0 to speed the process up (even though in my opinion it is not technically a breaking change). |
@josephdecock, @brockallen can you review my latest proposal? |
I'd like to see a test in our test suite that exercises this. Also, did you @pecanw, how soon did you need this? The branch this PR is against is for our next point release which won't be for a while. We will/can put a patch out sooner, but the PR needs to be against the releases/7.0.x branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just go ahead and do this in a 7.0 patch, since I want to ultimately handle max_age and prompt the same wrt PAR. I'm adding tests and rebasing against the release branch now.
Closing this in favor a new PR that is rebased onto the correct branch. |
thanks for all involved 👍 Great work!!! |
When the authorize endpoint is called with
max_age=0
parameter it is copied to the callback returnUrl. So after successful login the user is redirected to the login page again (with the max_age parameter again in the returnUrl).I believe, that the parameter should be handled similar way as the prompt=login:
My proposal is to remove the parameter from the further chain:
Potentially, it could be done only when
MaxAge==0
.