-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Replace originalIdentity's groups with identity's groups #26344
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
Conversation
@homar can you please provide the review here? |
@andythsu can you please provide unit tests? |
Yep that was clearly a mistake from my side. If you check SessionRepresentation it is clear that we need groups from identity here not originalIdentity. |
I believe such test could be added to |
There's also https://github.com/trinodb/trino/blob/master/testing/trino-tests/src/test/java/io/trino/execution/TestUserImpersonationAccessControl.java that although the name suggests "test impersonation", there are no tests in this file that actually touches on the impersonation part. I believe we can merge it with https://github.com/trinodb/trino/blob/master/testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java. Thoughts? (I can do it in a separate PR) |
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: asu80.
|
@kokosing I added a unit test |
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.
Thank you!
@Timeout(10) | ||
public void testSessionRepresentationReturnsCorrectGroupsDuringImpersonation() | ||
{ | ||
// given |
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.
please remove these comments.
.build(); | ||
|
||
// then | ||
Set<String> originalUserGroups = aliceImpersonationSession.toSessionRepresentation() |
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.
Please instead run the SQL to check the. See https://trino.io/docs/current/functions/session.html and current_groups()
.
Also please make sure that this test is failing when you revert the change to make sure this test actually provides a true coverage.
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.
The current_groups()
actually shows the right groups. This bug only happens in
show schemas from <catalog>
show tables from <catalog>.<schema>
Other operations will work fine.
It looks like other operations don't call toSessionRepresentation
but only these two operations will call this method, and hence this error.
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.
Ok, please use current_group()
and also test:
show schemas from <catalog>
show tables from <catalog>.<schema>
@kokosing I moved my tests to a new file because it requires quite different setup from https://github.com/trinodb/trino/blob/master/testing/trino-tests/src/test/java/io/trino/security/TestImpersonation.java In addition, I ran these tests without my change and it returned ![]() which is expected |
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.
Please squash the commits. Apply comment. Ping me once it is green to be merged. Thank you!
@@ -0,0 +1,144 @@ | |||
package io.trino.security; |
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.
add copyright
85cdc67
to
4961940
Compare
@kokosing everything passed! |
I improved release notes a bit. Thank you! |
Description
Use identity's groups instead of OriginalIdentity's groups in session. Issue is described here
This PR specifically resolves Option 2 in the issue
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(X) Release notes are required, with the following suggested text: