Skip to content

fix: accessToken scopes clean serialization and default as empty list#1125

Merged
TimurSadykov merged 8 commits intomainfrom
stim-smbios
Jan 11, 2023
Merged

fix: accessToken scopes clean serialization and default as empty list#1125
TimurSadykov merged 8 commits intomainfrom
stim-smbios

Conversation

@TimurSadykov
Copy link
Copy Markdown

@TimurSadykov TimurSadykov commented Jan 7, 2023

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

@TimurSadykov TimurSadykov requested review from a team and sai-sunder-s January 7, 2023 12:26
@product-auto-label product-auto-label Bot added the size: m Pull request size is medium. label Jan 7, 2023
String scopes =
OAuth2Utils.validateOptionalString(
List<String> scopes =
OAuth2Utils.validateOptionalListString(
Copy link
Copy Markdown

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?

Copy link
Copy Markdown
Author

@TimurSadykov TimurSadykov Jan 9, 2023

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.

Copy link
Copy Markdown
Author

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

Copy link
Copy Markdown

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;
Copy link
Copy Markdown
Contributor

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>

Copy link
Copy Markdown
Author

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.

Copy link
Copy Markdown

@wangyutongg wangyutongg left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size: m Pull request size is medium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants