-
Notifications
You must be signed in to change notification settings - Fork 322
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
Fix logout token iss when issuer is missing #1486
Conversation
Demonstrates that the back channel logout service will create tokens with a bad issuer if it is missing from the logout request
/// <param name="claims">The claims.</param> | ||
/// <returns></returns> | ||
/// <exception cref="System.ArgumentNullException">claims</exception> | ||
public virtual Task<string> IssueJwtAsync(int lifetime, string issuer, IEnumerable<Claim> claims) | ||
public virtual async Task<string> IssueJwtAsync(int lifetime, string tokenType, IEnumerable<Claim> claims) |
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.
Oh wait, this is a breaking change. Need to fix the caller instead I think.
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.
To fix the caller, we have to add a dependency on the IssuerNameService
from the DefaultBackChannelLogoutService
, which technically is also a breaking change. Maybe we don't need this in a 6.3 patch release?
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.
If it's a bug and not working as expected, then I don't think it's a problem to make a breaking change.
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.
Okay, then I'll keep this for the 6.3 branch, but still rework the fix slightly so that we're not changing semantics of the method in IdentityServerTools. This way the "breaking" change only affects the bug, and won't surprise people (the other change would probably have compiled without an error vs being a new ctor parameter that the compiler will make you fix in an obvious way)
Moved the fix from IdentityServerTools into the back channel logout service to avoid breaking changes in IdentityServerTools. While technically this is a breaking change in the back channel service too, this makes the impact much smaller, and more likely to be caught by the compiler. The previous implementation changed the semantics of a string parameter in a way that could have been surprising, vs this way adds a new constructor parameter in a way that will be totally obvious and caught by the compiler.
Note that we now rely on the issuer name service, which indirectly relies on http context, so it is possible that server side session expiry will fail, unless an issuer is explicitly configured. |
Fixes #1481