-
Notifications
You must be signed in to change notification settings - Fork 205
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
Customizable JWT audiences #16330
Customizable JWT audiences #16330
Conversation
This reverts commit ae485a0.
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.
Nice work! This looks good to me in terms of functionality & UX, I think testing needs to be a bit more thorough around IDP support. I'll leave detailed review of the implementation to your team.
ledger-api/grpc-definitions/com/daml/ledger/api/v1/admin/identity_provider_config_service.proto
Outdated
Show resolved
Hide resolved
...local-store/src/test/lib/com/daml/platform/localstore/IdentityProviderConfigStoreTests.scala
Show resolved
Hide resolved
…ity_provider_config_service.proto Co-authored-by: Moritz Kiefer <moritz.kiefer@purelyfunctional.org>
todo removal
ledger-api/grpc-definitions/com/daml/ledger/api/v1/admin/identity_provider_config_service.proto
Outdated
Show resolved
Hide resolved
…ity_provider_config_service.proto Co-authored-by: mziolekda <marcin.ziolek@digitalasset.com>
...-api/src/main/scala/platform/apiserver/services/admin/ApiIdentityProviderConfigService.scala
Outdated
Show resolved
Hide resolved
|
||
it should "deny calls with user token for 'unknown_user' without expiration" taggedAs adminSecurityAsset | ||
.setAttack( | ||
attackPermissionDenied(threat = "Present a user JWT for 'unknown_user' without expiration") |
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.
What is the deciding factor for this threat to be permission_denied and for others to be unauthenticated?
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.
Somehow this is what was there in the impl before. Depending on which layer authorization fails - it gives either permission denied or unauthenticated. Permission denied usually is thrown when something has been correctly provided. "Aud" claim check happens during parsing of the token, so it fails very early which is why it throws "unauthenticated"
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.
Left a few comments and questions
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, please merge it in
…6.x (#16631) * Add identity provider config to ledger client (#16625) * IdentityProviderConfigService through ledger client: create config * IdentityProviderConfigService through ledger client: get config * IdentityProviderConfigService through ledger client: list configs * IdentityProviderConfigService through ledger client: delete config * Fix updating of the audience field in IDP (#16630) * Customizable JWT audiences (#16330) --------- Co-authored-by: mziolekda <marcin.ziolek@digitalasset.com> Co-authored-by: Sergey Kisel <98825453+skisel-da@users.noreply.github.com>
No description provided.