Skip to content
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

Missing group attribute for queries in File system access control #9484

Closed
sbernauer opened this issue Oct 4, 2021 · 3 comments · Fixed by #9811
Closed

Missing group attribute for queries in File system access control #9484

sbernauer opened this issue Oct 4, 2021 · 3 comments · Fixed by #9811
Assignees

Comments

@sbernauer
Copy link
Member

Hi Trino-Team!

Reading and trying the docs https://trino.io/docs/current/security/file-system-access-control.html#query-rules i noticed two issues:

  1. Bug: The attribute "owner" does not get pick up by the JSON Mapper QueryAccessRule. I also don't see any usage of this e.g. in FileBasedSystemAccessControl. IMHO this is a bug, because users limiting user A to kill queries of user B would instead grant user A to kill any query regardless of the owner.
    a. We can either remove the "owner" attribute from the documentation.
    b. I can think of some use-cases where user A should be able to stand in for user B and kill his query
    I would suggest sticking with option b as the interface SystemAccessControl passes the owner attribute e.g. default void checkCanKillQueryOwnedBy(SystemSecurityContext context, String queryOwner)
  2. Missing feature: We are not able to give the group "admin" the ability to kill all queries. The attribute group is missing, we can only grant it to explicit users. This is already implemented for roles in QueryAccessRule so i should be possible to easily implement this

Regards,
Sebastian

@sbernauer sbernauer changed the title Missing group attribute for queries in File system access controll Missing group attribute for queries in File system access control Oct 5, 2021
@cccs-tom
Copy link
Member

+1 We are finding the need for @sbernauer's 2nd point as well (mapping groups to QueryAccessRules). I can contribute that portion, if you like. It looks like @dain did the original work, as well as the recent addition of Roles - any objections?

@kokosing
Copy link
Member

No objects. I will be happy to review your code. What about the first issue? Would it be possible to solve them both at one go?

@cccs-tom
Copy link
Member

@kokosing Sure, I'll give it a shot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants