Skip to content

Conversation

@d-trace
Copy link
Contributor

@d-trace d-trace commented Jun 11, 2021

Description

Add roles parsing logic to JWT parser and expose roles in request context.
The main motivation is to be able to reuse this logic in other parts of codebase, specifically in graphql context

Testing

Added unit tests

Checklist:

  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

Documentation

Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.

@codecov
Copy link

codecov bot commented Jun 11, 2021

Codecov Report

Merging #17 (f24d4e5) into main (eb6b0d1) will increase coverage by 0.61%.
The diff coverage is 80.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main      #17      +/-   ##
============================================
+ Coverage     71.49%   72.10%   +0.61%     
- Complexity       78       80       +2     
============================================
  Files            16       16              
  Lines           228      233       +5     
  Branches         12       13       +1     
============================================
+ Hits            163      168       +5     
+ Misses           55       54       -1     
- Partials         10       11       +1     
Flag Coverage Δ
unit 72.10% <80.00%> (+0.61%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...g/hypertrace/core/grpcutils/context/JwtParser.java 85.18% <75.00%> (-1.78%) ⬇️
...ertrace/core/grpcutils/context/RequestContext.java 64.28% <100.00%> (+5.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb6b0d1...f24d4e5. Read the comment docs.

@github-actions

This comment has been minimized.

surajpuvvada
surajpuvvada previously approved these changes Jun 11, 2021
Copy link

@surajpuvvada surajpuvvada left a comment

Choose a reason for hiding this comment

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

lgtm - but pls wait for one more review

private final String testJwtName = "Johnny Rocket";
private final String testJwtPictureUrl = "www.example.com";
private final String testJwtEmail = "jrocket@example.com";
private final Set<String> testRoles = ImmutableSet.of("traceable", "user", "billing_admin");
Copy link
Contributor

Choose a reason for hiding this comment

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

We can drop the traceable role - want to leak as little as possible here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Done

@github-actions

This comment has been minimized.

@d-trace
Copy link
Contributor Author

d-trace commented Jun 11, 2021

@aaron-steinfeld addressed comments. Please have another look

avinashkolluru
avinashkolluru previously approved these changes Jun 14, 2021
@github-actions

This comment has been minimized.

@d-trace
Copy link
Contributor Author

d-trace commented Jun 14, 2021

hey @aaron-steinfeld, addressed issues we discussed offline. Please have a look

@d-trace d-trace merged commit a86db35 into main Jun 14, 2021
@d-trace d-trace deleted the eng_9494_expose_roles_in_request_context branch June 14, 2021 21:10
@github-actions
Copy link

Unit Test Results

11 files  ±0  11 suites  ±0   8s ⏱️ -3s
53 tests +4  53 ✔️ +4  0 💤 ±0  0 ❌ ±0 

Results for commit a86db35. ± Comparison against base commit eb6b0d1.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants