Support passing groups in OAuth access token claim #10262
Merged
Conversation
This file contains 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
kokosing
reviewed
Dec 10, 2021
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
Show resolved
Hide resolved
core/trino-main/src/test/java/io/trino/server/security/TestResourceSecurity.java
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/server/security/oauth2/OAuth2Authenticator.java
Outdated
Show resolved
Hide resolved
| .withPrincipal(new BasicPrincipal(principal)) | ||
| .build()); | ||
| Identity.Builder identity = Identity.forUser(userMapping.mapUser(principal)); | ||
| identity.withPrincipal(new BasicPrincipal(principal)); |
There was a problem hiding this comment.
identity = identity.withPrincipal(new BasicPrincipal(principal));?
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
Show resolved
Hide resolved
| .toInstance(tokenServer.getOAuth2Client()); | ||
| }) | ||
| .build()) { | ||
| server.getInstance(Key.get(AccessControlManager.class)).addSystemAccessControl(TestSystemAccessControl.NO_IMPERSONATION); |
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
| this.service = requireNonNull(service, "service is null"); | ||
| this.principalField = config.getPrincipalField(); | ||
| groupsField = requireNonNull(config.getGroupsField(), "groupsField is null"); |
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.
| identity.withPrincipal(new BasicPrincipal(principalName)); | ||
| groupsField.flatMap(field -> Optional.ofNullable((List<String>) claims.get().get(field))) | ||
| .ifPresent(groups -> identity.withGroups(ImmutableSet.copyOf(groups))); | ||
| setAuthenticatedIdentity(request, identity.build()); |
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.
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
| if (principal.isEmpty()) { | ||
| return Optional.empty(); | ||
| } | ||
| return Optional.of(Identity.forUser(userMapping.mapUser(principal.get())) | ||
| .withPrincipal(new BasicPrincipal(principal.get())) | ||
| .build()); |
There was a problem hiding this comment.
You can continue the chain and avoid the if statement here by using map.
There was a problem hiding this comment.
I'm afraid it would not work. User mapping throws UserMappingException which is checked exception.
| String principal = (String) claims.get().get(principalField); | ||
| Identity.Builder identity = Identity.forUser(userMapping.mapUser(principal)); | ||
| identity.withPrincipal(new BasicPrincipal(principal)); | ||
| groupsField.flatMap(field -> Optional.ofNullable((List<String>) claims.get().get(field))) |
There was a problem hiding this comment.
No need for flatMap as map handles null correctly.
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<>
| .withPrincipal(new BasicPrincipal(principal)) | ||
| .build()); | ||
| Identity.Builder identity = Identity.forUser(userMapping.mapUser(principal)); | ||
| identity.withPrincipal(new BasicPrincipal(principal)); |
There was a problem hiding this comment.
withPrincipal just modifies the builder and returns this, thus reassignment is unnecessary.
| .build()); | ||
| Identity.Builder identity = Identity.forUser(userMapping.mapUser(principalName)); | ||
| identity.withPrincipal(new BasicPrincipal(principalName)); | ||
| groupsField.flatMap(field -> Optional.ofNullable((List<String>) claims.get().get(field))) |
There was a problem hiding this comment.
Again map here is cleaner and clearer.
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
| return groupsField; | ||
| } | ||
|
|
||
| @Config("http-server.authentication.oauth2.groups-field") |
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
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
Is this related? |
kokosing
reviewed
Jan 18, 2022
...duct-tests-launcher/src/main/java/io/trino/tests/product/launcher/env/EnvironmentModule.java
Show resolved
Hide resolved
lukasz-walkiewicz
force-pushed
the
oauth-groups-claim
branch
from
338f27a
to
f9e5058
Compare
|
I don't why this happend tbh. The container simply didn't start. Let's give it another try. |
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. |
|
Fair point. I'll stress test it. |
Also you can stress test the master to see if it is something or pre-existing issue. |
|
I was able to reproduce it locally and it looks like the problem is that |
👍 Thank you! |
lukasz-walkiewicz
force-pushed
the
oauth-groups-claim
branch
3 times, most recently
from
26c8c76
to
a79ca06
Compare
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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