-
Notifications
You must be signed in to change notification settings - Fork 218
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: accessToken scopes clean serialization and default as empty list #1125
Conversation
String scopes = | ||
OAuth2Utils.validateOptionalString( | ||
List<String> scopes = | ||
OAuth2Utils.validateOptionalListString( |
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.
For the existing token with string-typed scopes in the token store, this validation will fail, and they need to refresh to get a new access token?
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.
The feature is not shipped yet, it was checked in late last year. Basically trying to improve it before it is shipped.
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.
added this detail to the change description
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.
This makes sense as we do not need to worry about breaking existing customers.
throw new IOException( | ||
String.format(VALUE_WRONG_TYPE_MESSAGE, errorPrefix, "List<String>", key)); | ||
} | ||
return (List<String>) value; |
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.
There is no type check for List<String>
. Check is for List
. What happens if value
is something like List<Int>
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.
Should be acceptable as we serialize ourselves. And if the type is wrong its most likely tampered and will fail anyways.
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.
LGTM
Removing extra processing from access token copes serialization
Making empty list of scopes as default, no null
The original change that introduces Scopes into AccessToken is not shipped yet, therefore this change does not change the existing behavior