-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Enhance validation for configured Issuer #649
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
Conversation
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.
Thanks for the PR @NotFound403. Please see review comments.
...ork/security/config/annotation/web/configuration/OAuth2AuthorizationServerConfiguration.java
Outdated
Show resolved
Hide resolved
...otation/web/configurers/oauth2/server/authorization/OAuth2AuthorizationServerConfigurer.java
Show resolved
Hide resolved
...n/java/org/springframework/security/oauth2/server/authorization/config/ProviderSettings.java
Outdated
Show resolved
Hide resolved
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.
@jgrandja I have modified my code as you suggested.
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.
Thanks for the updates @NotFound403. Please see review comments.
Also, please squash commits and rebase on main. There should only be 1 commit as part of this change.
...otation/web/configurers/oauth2/server/authorization/OAuth2AuthorizationServerConfigurer.java
Outdated
Show resolved
Hide resolved
...otation/web/configurers/oauth2/server/authorization/OAuth2AuthorizationServerConfigurer.java
Outdated
Show resolved
Hide resolved
...a/org/springframework/security/oauth2/server/authorization/config/ProviderSettingsTests.java
Outdated
Show resolved
Hide resolved
161f789
to
29f4362
Compare
Thanks for the updates @NotFound403. This is now merged in FYI, I added a bit of polish to the tests. |
Thank you,Joe |
rfc8414#section-2
The "https" schema in development may be unsuited, but others should be vailidated.
I'm not sure if the above has been considered, further discussion is required