add adfs compatibility #1
Conversation
| if v.config.AdfsCompatibility { | ||
| // ADFS appends the string "/services/trust" to the issuer string, breaking the standard. | ||
| // More details can be found here: | ||
| // https://github.com/aspnet/Security/issues/1852 |
There was a problem hiding this comment.
great background on the issue there!
| // More details can be found here: | ||
| // https://github.com/aspnet/Security/issues/1852 | ||
| expectedIssuer := v.issuer + "/services/trust" | ||
| if t.Issuer != expectedIssuer { |
There was a problem hiding this comment.
do we need to handle the case of the issuer being put in the access_token_issuer claim instead of the issuer claim for ADFS?
There was a problem hiding this comment.
As far as I understand the description of the issue here this shouldn't be necessary, as long as we can verify the issuer i think we're fine: aspnet/Security#1852 (comment)
however, i've just read the description again and I noticed now that depending on which ADFS endpoint was used to generate an access token that /services/trust string might or might not get appended. So I should add a unit test to test if this also works with ADFS enabled and the "proper" issuer.
There was a problem hiding this comment.
added here and it passes: 00cd75e
thx for reviewing. I answered on your comment on GitHub, and since we also already discussed that via Slack I assume it's fine if I go ahead and merge this. |
This implements the first suggestion of this comment:
aspnet/Security#1852 (comment)