Prevent infinite loop when max_age=0

[pecanw]

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:

if (request.PromptModes.Contains(OidcConstants.PromptModes.Create))
{
    Logger.LogInformation("Showing create account: request contains prompt=create");
    request.RemovePrompt();
    result = new InteractionResponse
    {
        IsCreateAccount = true
    };
}

My proposal is to remove the parameter from the further chain:

request.Raw.Remove("max_age");

Potentially, it could be done only when MaxAge==0.

[josephdecock]

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.

[brockallen]

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?

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?

[josephdecock]

The OIDC spec does say

max_age=0 is equivalent to prompt=login.

So I was thinking along those lines. Maybe max_age=0 becomes prompt=login during validation? But it does feel very "clever".

[pecanw]

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).

[pecanw]

@josephdecock, @brockallen can you review my latest proposal?

[brockallen]

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.

[josephdecock]

Closing this in favor a new PR that is rebased onto the correct branch.

[tiagofe]

thanks for all involved 👍 Great work!!!