Support passing groups in OAuth access token claim#10262
Merged
kokosing merged 11 commits intotrinodb:masterfrom Jan 24, 2022
Merged
Support passing groups in OAuth access token claim#10262kokosing merged 11 commits intotrinodb:masterfrom
kokosing merged 11 commits intotrinodb:masterfrom
Conversation
kokosing
reviewed
Dec 10, 2021
Member
kokosing
left a comment
kokosing
left a comment
There was a problem hiding this comment.
Let's add a product tests that would call current_groups() To make sure it works end to end.
Looks nice!
core/trino-main/src/test/java/io/trino/server/security/TestResourceSecurity.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/test/java/io/trino/server/security/TestResourceSecurity.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/test/java/io/trino/server/security/TestResourceSecurity.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/server/security/oauth2/OAuth2Authenticator.java
Outdated
Show resolved
Hide resolved
Member
There was a problem hiding this comment.
identity = identity.withPrincipal(new BasicPrincipal(principal));?
Member
There was a problem hiding this comment.
withPrincipal just modifies the builder and returns this, thus reassignment is unnecessary.
core/trino-main/src/test/java/io/trino/server/security/TestResourceSecurity.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/test/java/io/trino/server/security/TestResourceSecurity.java
Outdated
Show resolved
Hide resolved
Member
Author
There was a problem hiding this comment.
It's not important actually whether it allows impersonation or not but I think it's better to deny by default.
The important part is:
@Override
public void checkCanReadSystemInformation(SystemSecurityContext context)
{
if (!context.getIdentity().getUser().equals(MANAGEMENT_USER)) {
denyReadSystemInformationAccess();
}
}as it's being tested by assertAuthenticationAutomatic
mat3e
approved these changes
Dec 13, 2021
Contributor
There was a problem hiding this comment.
nit: we can skip this rnn here and in other places as groups field is an empty Optional by default in the config.
Contributor
There was a problem hiding this comment.
Maybe not for this PR, but would it be possible to make this filter using OAuth2Authenticator? Seems they share more logic now than previously.
Member
Author
There was a problem hiding this comment.
I initially thought about it and I think it's generally a good idea but it would require a bit broader refactor. The important difference between OAuth2Authenticator and this filter is that protocol authenticator throws an Authentication exception (401 Unauthorized) whereas this filter redirects the browser to authentication url (303 See Other). It's doable ofc but as I said, it requires a bit of refactoring.
11xor6
approved these changes
Dec 14, 2021
Comment on lines
66
to
71
Member
There was a problem hiding this comment.
You can continue the chain and avoid the if statement here by using map.
Member
Author
There was a problem hiding this comment.
I'm afraid it would not work. User mapping throws UserMappingException which is checked exception.
Member
There was a problem hiding this comment.
No need for flatMap as map handles null correctly.
Member
Author
There was a problem hiding this comment.
Optional.map(o -> Optional.of(o)) returns Optional<Optional<>>
whereas
Optional.flatMap(o -> Optional.of(o)) returns Optional<>
Member
There was a problem hiding this comment.
withPrincipal just modifies the builder and returns this, thus reassignment is unnecessary.
Member
There was a problem hiding this comment.
Again map here is cleaner and clearer.
Member
Author
There was a problem hiding this comment.
userMapping.mapUser(principalName) throws checked UserMappingException
lukasz-walkiewicz
force-pushed
the
oauth-groups-claim
branch
from
4b3bfdb to
13d4aea
Compare
kokosing
approved these changes
Jan 6, 2022
Member
Author
There was a problem hiding this comment.
It's already done:
* - ``http-server.authentication.oauth2.groups-field``
- The field of the access token used for Trino groups. The corresponding claim value must be an array.Would like to add something?
kokosing
reviewed
Jan 7, 2022
...main/java/io/trino/tests/product/launcher/env/environment/EnvSinglenodeOauth2HttpsProxy.java
Outdated
Show resolved
Hide resolved
lukasz-walkiewicz
force-pushed
the
oauth-groups-claim
branch
from
13d4aea to
dabb828
Compare
lukasz-walkiewicz
force-pushed
the
oauth-groups-claim
branch
from
dabb828 to
338f27a
Compare
Member
Is this related? |
kokosing
reviewed
Jan 18, 2022
...duct-tests-launcher/src/main/java/io/trino/tests/product/launcher/env/EnvironmentModule.java
Outdated
Show resolved
Hide resolved
lukasz-walkiewicz
force-pushed
the
oauth-groups-claim
branch
from
338f27a to
f9e5058
Compare
Member
Author
|
I don't why this happend tbh. The container simply didn't start. Let's give it another try. |
Member
This is not an option. It seems that the environment is flaky. Please stress it out instead to see how frequent it is. Also IIRC we do retry setup of each environment which may suggest that this env failed couple times in row. I am not sure if retry happened here. |
Member
Author
|
Fair point. I'll stress test it. |
Member
Also you can stress test the master to see if it is something or pre-existing issue. |
Member
Author
|
I was able to reproduce it locally and it looks like the problem is that |
Member
👍 Thank you! |
lukasz-walkiewicz
force-pushed
the
oauth-groups-claim
branch
3 times, most recently
from
26c8c76 to
a79ca06
Compare
Member
Author
|
I've fixed problems with the startup check. Stress tested in: https://github.com/trinodb/trino/actions/runs/1735765630 |
Temporal containers can finish faster than the startup probe can check. The check interval comes from the docker client with rate limiting which queries the docker service at most once per second. This can create a race condition if the container can complete it's job in less or around 1 second.
Instead of using selenium driver to log in and accept the consent request use a simple python implementation which accepts all requests thus eliminating the need of web driver entirely. Fixes trinodb#6991
lukasz-walkiewicz
force-pushed
the
oauth-groups-claim
branch
from
a79ca06 to
d842185
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves: #10220