Adjust required permissions for system schema #7579
Conversation
|
will need to adjust tests to handle when the full IT test suite is run (since cluster state is not isolated across tests, things like "total stored segment size on historicals" change) |
| authorizerMapper | ||
| ); | ||
| if (!stateAccess.isAllowed()) { | ||
| throw new ForbiddenException("Insufficient permission to view servers :" + stateAccess); |
There was a problem hiding this comment.
Whitespace should be after the colon here.
|
|
||
| final Access stateAccess = AuthorizationUtils.authorizeAllResourceActions( | ||
| authenticationResult, | ||
| Collections.singletonList(new ResourceAction(new Resource("STATE", ResourceType.STATE), Action.READ)), |
There was a problem hiding this comment.
new Resource("STATE", ResourceType.STATE) is a very specific thing and should probably be a constant in some file or other.
There was a problem hiding this comment.
Made this a constant in Resource
| authenticationResult, | ||
| Collections.singletonList(new ResourceAction(new Resource("STATE", ResourceType.STATE), Action.READ)), | ||
| authorizerMapper | ||
| ); |
There was a problem hiding this comment.
Since authenticationResult and stateAccess are being used in ServersTable as well as here in ServerSegmentsTable, may be this part can be placed in a common method like
private static boolean accessAllowed(DataContext root, AuthorizerMapper authorizerMapper)
{
final AuthenticationResult authenticationResult =
(AuthenticationResult) root.get(PlannerContext.DATA_CTX_AUTHENTICATION_RESULT);
final Access access = AuthorizationUtils.authorizeAllResourceActions(
authenticationResult,
Collections.singletonList(new ResourceAction(new Resource("STATE", ResourceType.STATE), Action.READ)),
authorizerMapper
);
return access.isAllowed();
}
There was a problem hiding this comment.
Moved this to a checkStateReadAccessForServers method
|
I updated the SQL docs with info on necessary permissions, also added tests for a user without access to the |
There was a problem hiding this comment.
By the way, it's a bit weird that the Druid permissions model is documented in the druid-basic-security extension, which isn't responsible for defining the permissions model. It makes it sound like the model is not a core thing. I think it'd be good to fix this at some point - would you mind raising an issue (or, better, a new PR)?
|
|
||
| Queries on Druid datasources require DATASOURCE READ permissions for the specified datasource. | ||
|
|
||
| Queries on the [information schema tables](../../querying/sql.html#information-schema) require DATASOURCE READ access for the specified datasource. |
There was a problem hiding this comment.
Please adjust the language a bit- there is not necessarily any 'specified datasource' with the information schema. The query might just be SELECT * FROM INFORMATION_SCHEMA.COLUMNS. I'd suggest:
Queries on the [INFORMATION_SCHEMA tables](../../querying/sql.html#information-schema) will
return information about datasources that the caller has DATASOURCE READ access to. Other
datasources will be omitted.
|
LGTM other than the comment about doc wording. |
|
Fixed tests:
|
This PR implements the following change described in #7563:
A STATE.READ permission check is added to
server_segmentssys table handling (serversalready had one, and #7567 removed the unnecessarysysdatasource check).This PR also adds integration tests for sys schema + auth. A new segment and task for
auth_testdatasource are inserted directly into metadata during IT setup, and this datasource is used for testing the auth checks.