BearerToken model #373
BearerToken model #373
Conversation
| */ | ||
| public static boolean isValid(String input) { | ||
| return input != null; | ||
| } |
There was a problem hiding this comment.
Now that you have the BearerToken class, I think you also have a place to formalize what it means for the actual token to be null, give that thing a name, and encapsulate that concern instead of having to do null-checks anywhere.
I don't know if I have the name right here...but maybe it's as simple as Unauthenticated? 🤷
| } | |
| public static BearerToken unauthenticated() { | |
| return new BearerToken(null); | |
| } | |
| public boolean isAuthenticated() { | |
| return value != null; | |
| } |
There was a problem hiding this comment.
Semantically, I want to be pretty clear about what we're actually providing here. What the current implementation offers is: "WDS found an Authentication: Bearer … header, and saved its contents for later use". What I am trying to avoid is: "WDS has an OAuth token that is currently valid for use in Terra".
The latter implies that the token isn't expired, decodes as a JWT, has the right scopes, was issued by the right OAuth issuer, etc. I'm trying to keep WDS out of this business, and instead delegate to Sam and Azure b2c for this work.
The former makes very few promises. This keeps WDS simple: if it finds some opaque value in the http header, it will then pass that same http header onwards when it makes outbound requests.
Does this stance 1) make sense, and 2) change anything about the model you're suggesting?
Perhaps it's just a terminology thing, I could be satisfied with simply changing BearerToken.isAuthenticated() to BearerToken.isEmpty().
There was a problem hiding this comment.
Totally agree with your separation of concerns here, keeping WDS lean and mean wrt to the assertions it makes about this token. I'm mainly motivated to drive nulls out the interaction here, and completely incapable of coming up with a good name for what you describe. If you're good with isEmpty I am too!
| public static String getToken(String initialValue) { | ||
| return getToken(initialValue, () -> null); | ||
| public static BearerToken getToken(String initialValue) { | ||
| return getToken(new BearerToken(initialValue), () -> null); |
There was a problem hiding this comment.
Here you could potentially use () => BearerToken.unauthenticated()
| public static String getToken(String initialValue, Supplier<String> orElse) { | ||
| if (initialValue != null) { | ||
| public static BearerToken getToken(BearerToken initialValue, Supplier<BearerToken> orElse) { | ||
| if (initialValue != null && initialValue.value() != null) { |
There was a problem hiding this comment.
Here you could potentially use initialValue.isAuthenticated() instead of checking initialValue.value() != null (subject to that being a reasonable name/concept to formalize)
|
Kudos, SonarCloud Quality Gate passed! |
| } | ||
|
|
||
| /** indicates if this BearerToken is not empty; that is, has a non-null `value` */ | ||
| public boolean nonEmpty() { |
There was a problem hiding this comment.
The double negative here isn't the easiest to read, but I don't have a better alternative.
|
|
||
| /** returns a BearerToken with the specified `value` */ | ||
| public static BearerToken of(String val) { | ||
| return new BearerToken(val); |
There was a problem hiding this comment.
only thought I had is - would it be useful to check if the token actually contains "Bearer" (or contains more then one) and if it doesnt throw a specific error that format is not correct or something like that? Maybe a very rare occurrence but when I was testing cloning I had Bearer in the token twice and it took an embarrassing amount of time to figure out :D
There was a problem hiding this comment.
See my previous reply to Josh: #373 (comment) … I am hoping to stay away from anything beyond the most basic validation. That said, if I'm in the minority here, we can add it …
Incorporate a
BearerTokenclass as a safer and more encapsulated option instead of using rawStrings.