Skip to content

Fix redirect_uri resolver #1013

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

Fix redirect_uri resolver #1013

wants to merge 4 commits into from

Conversation

leshalv
Copy link
Contributor

@leshalv leshalv commented Dec 18, 2022 •
edited by jgrandja
Loading

Fixes gh-1012

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 18, 2022
jgrandja
jgrandja requested changes Jan 10, 2023
View reviewed changes
Copy link
Collaborator

@jgrandja jgrandja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR @leshalv.

Please see the review comment and also please add a test to confirm the fix.

@jgrandja jgrandja changed the title Fix: The authorization code authentication error and the redirect URI selection is incorrect Fix redirectUri selection Jan 10, 2023
@jgrandja jgrandja changed the title Fix redirectUri selection Correct redirectUri resolver Jan 10, 2023
@jgrandja jgrandja self-assigned this Jan 10, 2023
@jgrandja jgrandja added status: duplicate A duplicate of another issue type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 10, 2023
@jgrandja jgrandja added this to the 0.4.1 milestone Jan 10, 2023
@leshalv
Copy link
Contributor Author

leshalv commented Jan 12, 2023

Ok, I've made the modification.

jgrandja
jgrandja requested changes Jan 16, 2023
View reviewed changes
Copy link
Collaborator

@jgrandja jgrandja left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @leshalv. Please see review comments for minor formatting changes.

Also, before we merge this, we need a test that proves the fix has been applied. You should be able to modify the existing test OAuth2AuthorizationCodeRequestAuthenticationProviderTests.authenticateWhenPkceRequiredAndMissingCodeChallengeThenThrowOAuth2AuthorizationCodeRequestAuthenticationException() to demonstrate the fix was applied. Thanks.

@jgrandja jgrandja mentioned this pull request Jan 23, 2023
Closed
@jgrandja
Copy link
Collaborator

@leshalv As mentioned in this comment, a test needs to be added as part of this PR. Are you able to add it?

@leshalv
Copy link
Contributor Author

leshalv commented Jan 30, 2023

@leshalv As mentioned in this comment, a test needs to be added as part of this PR. Are you able to add it?

Can you help me improve it?

@jgrandja
Copy link
Collaborator

jgrandja commented Feb 1, 2023

@leshalv

Can you help me improve it?

Do you need help with writing the test? Do you want me to write it and then merge?

@leshalv
Copy link
Contributor Author

leshalv commented Feb 1, 2023 •
edited
Loading

@leshalv

Can you help me improve it?

Do you need help with writing the test? Do you want me to write it and then merge?

Yes.

@jgrandja jgrandja changed the title Correct redirectUri resolver Fix redirect_uri resolver Feb 15, 2023
jgrandja added a commit that referenced this pull request Feb 15, 2023
@jgrandja
Polish gh-1013
1783bf7
@jgrandja
Copy link
Collaborator

@leshalv Thanks again for reporting this. I added a polish commit with tests and is now merged.

@jgrandja jgrandja closed this Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgrandja jgrandja jgrandja requested changes

Assignees

@jgrandja jgrandja

Labels
status: duplicate A duplicate of another issue type: bug A general bug
Projects
None yet
Milestone
0.4.1
Development

Successfully merging this pull request may close these issues.
3 participants
@leshalv @jgrandja @spring-projects-issues