Skip to content
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

feat: introduce jwt-spi and jwt-core modules #1867

Merged
merged 1 commit into from
Aug 25, 2022
Merged

feat: introduce jwt-spi and jwt-core modules #1867

merged 1 commit into from
Aug 25, 2022

Conversation

ndr-brt
Copy link
Member

@ndr-brt ndr-brt commented Aug 23, 2022

What this PR changes/adds

refactor token-validation-lib and token-generation-lib to the spi/core pattern

Why it does that

To accomplish project structure review

Further notes

  • I called the modules jwt-spi and jwt-core as they are specific to JWT (using nimbus-jose jwt related classes)
  • As pointed out in the issue Project structure: introduce token-spi module #1834 there is some duplication that could be avoided, but it was too much for this PR, will be something to be fixed in the future
  • With the new project structure seems that the modules in the common folder could be moved in core/common, as in the end they are core modules as well.

Linked Issue(s)

Closes #1834

Checklist

  • added appropriate tests?
  • performed checkstyle check locally?
  • added/updated copyright headers?
  • documented public classes/methods?
  • added/updated relevant documentation?
  • assigned appropriate label? (exclude from changelog with label no-changelog)
  • formatted title correctly? (take a look at the CONTRIBUTING and styleguide for details)

@ndr-brt ndr-brt added enhancement New feature or request refactoring Cleaning up code and dependencies labels Aug 23, 2022
@ndr-brt ndr-brt requested a review from jimmarino August 23, 2022 13:09
@codecov-commenter
Copy link

codecov-commenter commented Aug 23, 2022

Codecov Report

Merging #1867 (2e73140) into main (76a3825) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1867      +/-   ##
==========================================
- Coverage   62.54%   62.53%   -0.01%     
==========================================
  Files         782      780       -2     
  Lines       16592    16587       -5     
  Branches     1079     1079              
==========================================
- Hits        10377    10373       -4     
+ Misses       5767     5766       -1     
  Partials      448      448              
Impacted Files Coverage Δ
...ceconnector/core/jwt/JwtDecoratorRegistryImpl.java 0.00% <ø> (ø)
...connector/core/jwt/TokenGenerationServiceImpl.java 60.86% <ø> (ø)
...tor/core/jwt/TokenValidationRulesRegistryImpl.java 100.00% <ø> (ø)
...connector/core/jwt/TokenValidationServiceImpl.java 76.31% <ø> (ø)
...r/ids/token/validation/rule/IdsValidationRule.java 56.00% <ø> (ø)
.../dataspaceconnector/iam/daps/DapsJwtDecorator.java 100.00% <ø> (ø)
...paceconnector/iam/oauth2/core/Oauth2Extension.java 0.00% <ø> (ø)
...or/iam/oauth2/core/identity/Oauth2ServiceImpl.java 25.71% <ø> (ø)
...ector/iam/oauth2/core/jwt/DefaultJwtDecorator.java 100.00% <ø> (ø)
...re/jwt/Oauth2JwtDecoratorRegistryRegistryImpl.java 0.00% <ø> (ø)
... and 10 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ndr-brt
Copy link
Member Author

ndr-brt commented Aug 24, 2022

@jimmarino , yesterday @bscholtes1A pointed out to the fact that the jwt-spi module I introduced needs the nimbus-jose library, I don't know if this is accettable, the alternative would be to introduce some interfaces/classes to represent the JWT concept and replace/wrap the nimbus ones

@jimmarino
Copy link
Contributor

@jimmarino , yesterday @bscholtes1A pointed out to the fact that the jwt-spi module I introduced needs the nimbus-jose library, I don't know if this is accettable, the alternative would be to introduce some interfaces/classes to represent the JWT concept and replace/wrap the nimbus ones

@ndr-brt In this case, it may be onerous to wrap and unwrap Nimbus classes. Are there a lot of classes that need to be wrapped?

@ndr-brt
Copy link
Member Author

ndr-brt commented Aug 24, 2022

@ndr-brt In this case, it may be onerous to wrap and unwrap Nimbus classes. Are there a lot of classes that need to be wrapped?

In the spi we are using three nimbus-jose classes:

  • JWSHeader
  • JWTClaimsSet
  • SignedJWT

the first two of them are actually used paired on the JwtDecorator.decorate method so they can be wrapped by a single class.
I think this is doable, maybe we can address this to another issue for M7, wdyt @jimmarino ?

@jimmarino
Copy link
Contributor

@ndr-brt In this case, it may be onerous to wrap and unwrap Nimbus classes. Are there a lot of classes that need to be wrapped?

In the spi we are using three nimbus-jose classes:

  • JWSHeader
  • JWTClaimsSet
  • SignedJWT

the first two of them are actually used paired on the JwtDecorator.decorate method so they can be wrapped by a single class. I think this is doable, maybe we can address this to another issue for M7, wdyt @jimmarino ?

Sounds good to me.

@ndr-brt
Copy link
Member Author

ndr-brt commented Aug 25, 2022

#1879 created.
Are we good to go here? @jimmarino @bscholtes1A

@ndr-brt ndr-brt requested a review from jimmarino August 25, 2022 08:54
@ndr-brt ndr-brt merged commit d120860 into eclipse-edc:main Aug 25, 2022
@ndr-brt ndr-brt deleted the feature/1834-token-spi branch August 25, 2022 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request refactoring Cleaning up code and dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Project structure: introduce token-spi module
4 participants