-
Notifications
You must be signed in to change notification settings - Fork 126
chore(auth): Add typesafe classes for handling auth tokens #3123
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
base: main
Are you sure you want to change the base?
Conversation
8f4a00f
to
b94e0e2
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3123 +/- ##
==========================================
+ Coverage 54.27% 54.29% +0.02%
==========================================
Files 1040 1040
Lines 32071 32077 +6
Branches 4712 4721 +9
==========================================
+ Hits 17406 17417 +11
+ Misses 12824 12815 -9
- Partials 1841 1845 +4 🚀 New features to boost your workflow:
|
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.
Nice simplification!
"accessKeyId = ${accessKeyId?.substring(0..4)}***, " + | ||
"secretAccessKey = ${secretAccessKey?.substring(0..4)}***, " + | ||
"sessionToken = ${sessionToken?.substring(0..4)}***, " + | ||
"accessKeyId = ${accessKeyId.mask()}, " + |
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 really probably doesn't matter since its a masked value but this method includes 1 less character.
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.
Noted, but agree that doesn't matter. We can make it 4 or 5 but it should be consistent.
@@ -95,16 +96,22 @@ internal data class FederatedToken(val token: String, val providerName: String) | |||
*/ | |||
@Serializable | |||
internal data class CognitoUserPoolTokens( |
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.
What does the toString look like now? I think the tokens are still tracked properly after traversing through the code, but notice expiration is now included (probably for the best).
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.
I had not noticed that expiration was excluded before, but that seems like an error. The updated toString looks like this:
CognitoUserPoolTokens(idToken=eyJh***, accessToken=eyJh***, refreshToken=eyJh***, expiration=1758044817)
Issue #, if available:
Description of changes:
Updates internal handling of auth tokens to wrap them in typesafe classes instead of bare strings. This makes the codebase easier to understand, drastically cuts down on the number of times the JWT strings are parsed, and makes it less likely that token content is accidentally leaked (because the masking is applied at the wrapper class level).
How did you test these changes?
Documentation update required?
General Checklist
fix(storage): message
,feat(auth): message
,chore(all): message
)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.