feat: add jwt decoder #15
Conversation
GurtejSohi
requested review from
avinashkolluru,
surajpuvvada and
tim-mwangi
as code owners
Codecov Report
@@ Coverage Diff @@
## main #15 +/- ##
============================================
+ Coverage 66.47% 67.33% +0.86%
- Complexity 58 64 +6
============================================
Files 14 15 +1
Lines 170 199 +29
Branches 9 9
============================================
+ Hits 113 134 +21
- Misses 49 57 +8
Partials 8 8
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
|
|
||
| public Optional<VerifiedJwt> fromJwt(String jwtValue) { | ||
| try { | ||
| return this.verifiedJwtCache.get(jwtValue, () -> this.decodeAndVerify(jwtValue)); |
There was a problem hiding this comment.
if we need to access it multiple times we could store in the context object instead ?
There was a problem hiding this comment.
So my understanding is that we actually don't need or want to force verification here, instead relying on the proxy to do it. Either way though, everything around JWTs should be package protected, we only want to expose the abstracted information via the context.
If we do need to enforce it, then every service using this will need to take in env-specific values like the jwks url and audience, which is not ideal.
There was a problem hiding this comment.
Yes. I think thats what we decided the other day. Verification is the responsibility of the proxy.
Should we just add both the APIs then? Depending on the use case, the callers can decide.
There was a problem hiding this comment.
What do you mean by both the apis? the verified and unverified one?
There was a problem hiding this comment.
Removed jwt verification and now, exposing the abstracted information via the context.
There was a problem hiding this comment.
Yes. I meant decodeAndVerify and just decode
There was a problem hiding this comment.
I think we can add the verification part later on if required(we may not even need that if we're relying on proxy for verification). Also, the verification part requires passing in some extra values which makes it messy (which we may be able to avoid).
|
Do not forget to include tests on this one. |
| } | ||
| } | ||
|
|
||
| private static final class DefaultVerifiedJwt implements VerifiedJwt { |
There was a problem hiding this comment.
I am curious about this class being final and private. Do we expect others to implement their own VerifiedJwt? if so, will that be based on this default one?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
grpc-context-utils/src/main/java/org/hypertrace/core/grpcutils/context/Jwt.java
Outdated
Show resolved
Hide resolved
grpc-context-utils/src/test/java/org/hypertrace/core/grpcutils/context/JwtParserTest.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
| private static final Logger LOG = LoggerFactory.getLogger(JwtParser.class); | ||
| private static final String BEARER_TOKEN_PREFIX = "Bearer "; | ||
|
|
||
| private final Cache<String, Optional<Jwt>> jwtCache = |
There was a problem hiding this comment.
Yes, using the cache to help avoid decoding the same token again.
Description
Adds JWT decoder