suneet-s commented on a change in pull request #11718:
URL: https://github.com/apache/druid/pull/11718#discussion_r721644565
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/common/task/IndexTaskUtils.java
##########
@@ -56,28 +56,25 @@
}
/**
- * Authorizes action to be performed on a task's datasource
+ * Authorizes WRITE action on a task's datasource
*
- * @return authorization result
+ * @throws ForbiddenException if not authorized
*/
- public static Access datasourceAuthorizationCheck(
+ public static void datasourceAuthorizationCheck(
Review comment:
nit: similar comment to `AbstractTask.authorizeRequest`
Renaming this to something that indicates that we're checking for DATASOURCE
WRITE would make the intent of this function clearer to its' callers.
##########
File path:
indexing-service/src/test/java/org/apache/druid/indexing/common/task/IndexTaskTest.java
##########
@@ -2604,6 +2613,81 @@ private static IndexIngestionSpec createIngestionSpec(
}
}
+ @Test
+ public void testAuthorizeRequest() throws Exception
+ {
+ // Need to run this only once
+ if (lockGranularity == LockGranularity.SEGMENT) {
Review comment:
I don't understand this part of the test.
##########
File path:
indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/OverlordResourceTest.java
##########
@@ -116,10 +117,22 @@ public Authorizer getAuthorizer(String name)
@Override
public Access authorize(AuthenticationResult authenticationResult,
Resource resource, Action action)
{
- if (resource.getName().equals("allow")) {
- return new Access(true);
- } else {
- return new Access(false);
+ final String username = authenticationResult.getIdentity();
+ switch (resource.getName()) {
+ case "allow":
+ return new Access(true);
+ case Datasources.WIKIPEDIA:
+ // All users can read wikipedia but only writer can write
+ return new Access(
+ action == Action.READ || Users.WIKI_WRITER.equals(username)
Review comment:
```suggestion
action == Action.READ || (action == Action.WRITE &&
Users.WIKI_WRITER.equals(username))
```
I think a similar change is needed on line 132
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/common/task/AbstractTask.java
##########
@@ -164,6 +167,16 @@ public TaskStatus success()
return TaskStatus.success(getId());
}
+ /**
+ * Authorizes WRITE action on a task's datasource
+ *
+ * @throws ForbiddenException if not authorized
+ */
+ public void authorizeRequest(HttpServletRequest request, AuthorizerMapper
authorizerMapper)
Review comment:
nit: If the ForbiddenException is part of the javadoc, we should make it
part of the method signature
Also I think the name can be more informative, since the underlying methods
are explicitly checking DATASOURCE WRITE permissions - I'm open to a better
name for the method.
```suggestion
public void authorizeRequestForDatasourceWrite(HttpServletRequest request,
AuthorizerMapper authorizerMapper) throws ForbiddenException
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
