-
Notifications
You must be signed in to change notification settings - Fork 245
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: IATP: STS core services #3551
feat: IATP: STS core services #3551
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #3551 +/- ##
==========================================
+ Coverage 72.43% 72.49% +0.06%
==========================================
Files 871 877 +6
Lines 17394 17474 +80
Branches 988 994 +6
==========================================
+ Hits 12599 12668 +69
- Misses 4379 4390 +11
Partials 416 416
☔ View full report in Codecov by Sentry. |
6d84803
to
0698b75
Compare
01b1e0e
to
cc1e5a0
Compare
@Override | ||
public Result<TokenRepresentation> generate(@NotNull JwtDecorator... decorators) { | ||
var key = privateKeyResolver.resolvePrivateKey(keyAlias, PrivateKey.class); | ||
return new TokenGenerationServiceImpl(key).generate(decorators); |
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.
Looks a bit convoluted...
Could we modify the current TokenGenerationService that it does not take directly a private key in the ctor, but instead a Function<String,PrivateKey> (the input string being the alias)?
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.
That's a refactor that i would do later, if i change the TokenGenerationServiceImpl
not i might end up changing a lot of files :)
I would raise an issue and address this in another PR wdyt?
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.
Agreed, it would be good to look at simplifying this in a separate PR
...re/src/main/java/org/eclipse/edc/iam/identitytrust/sts/core/StsDefaultServicesExtension.java
Show resolved
Hide resolved
76e1e74
to
61fbb4f
Compare
61fbb4f
to
da1fb7a
Compare
.../src/main/java/org/eclipse/edc/iam/identitytrust/sts/service/StsTokenGenerationProvider.java
Show resolved
Hide resolved
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.
Looks good so far, a few comments/clarifications
core/common/jwt-core/src/main/java/org/eclipse/edc/jwt/LazyTokenGenerationService.java
Outdated
Show resolved
Hide resolved
@Override | ||
public Result<TokenRepresentation> generate(@NotNull JwtDecorator... decorators) { | ||
var key = privateKeyResolver.resolvePrivateKey(keyAlias, PrivateKey.class); | ||
return new TokenGenerationServiceImpl(key).generate(decorators); |
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.
Agreed, it would be good to look at simplifying this in a separate PR
What this PR changes/adds
Implements STS core services specifically
STS-SPI
Added module
:spi:common:identity-trust-sts-spi
for sts specific interfaces and models:StsClient
: modeling the clients in STSStsClientService
: Service layer for StsClientsStsClientStore
: Persistence layer for StsClients.StsClientTokenGeneratorService
: Token generator forStsClients
STS-Core
Added module
:extensions:common:iam:identity-trust:identity-trust-sts-core
with the default implementationof the
sts-spi
services.Why it does that
iatp adoption
Further notes
And additional token generation service has been implemented
LazyTokenGenerationService
to be able to work with non cached keys.Linked Issue(s)
Closes #3550