Skip to content
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 appending of user_code query parameter. #127

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

karelmaxa
Copy link
Member

This PR fixes handling of the user_code query parameter in gotoUrl during the OAuth2 authorization process. Old implementation does not check if the query parameter already exists. As a result, the DuplicateRequestParameterValidator throws a DuplicateRequestParameterException for URL with existing user_code query parameter.

@karelmaxa karelmaxa marked this pull request as draft June 26, 2023 08:40
@karelmaxa
Copy link
Member Author

I'm not sure if my flow was correct. The user_code attribute should probably not be part of the OAuth2 authorize request. I will convert the PR to a draft and do more detailed analysis later.

@pavelhoral
Copy link
Member

I'm not sure if my flow was correct. The user_code attribute should probably not be part of the OAuth2 authorize request. I will convert the PR to a draft and do more detailed analysis later.

You are probably right. It does not make sense to actually have user_code part of the /oauth2/authorize request. When the OAuth2 client can open user agent (e.g. in case of mobile app), it can surely handle custom redirect URI (app links / universal links). Device code flow with its user_code attribute is when the browser is being opened on a different device. The only case when the user_code can be used like this is for example when the whole authorization URL is being e-mailed... not sure if that is a valid use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants