You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2020/07/13 13:56:38 UTC
[GitHub] [pulsar] KannarFr opened a new pull request #7523: [Issue 5720][authz] - WIP add topics authz granularity
KannarFr opened a new pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523
Fixes a part of #5720
### Motivation
add granularity in topics api authz
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on a change in pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on a change in pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#discussion_r607794153
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java
##########
@@ -542,30 +543,56 @@ private void validatePoliciesReadOnlyAccess() {
String role,
TopicOperation operation,
AuthenticationDataSource authData) {
+ log.debug("Check allowTopicOperationAsync [" + operation.name() + "] on [" + topicName.toString() + "].");
+
CompletableFuture<Boolean> isAuthorizedFuture;
switch (operation) {
- case LOOKUP: isAuthorizedFuture = canLookupAsync(topicName, role, authData);
+ case LOOKUP:
+ case GET_STATS:
+ case GET_SUBSCRIPTIONS:
Review comment:
@zymap
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-804719349
@codelipenghui Can you review too?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on a change in pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#discussion_r595569400
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java
##########
@@ -293,7 +294,8 @@ default Boolean allowTenantOperation(String tenantName, String role, TenantOpera
NamespaceOperation operation,
AuthenticationDataSource authData) {
return FutureUtil.failedFuture(
- new IllegalStateException("NamespaceOperation is not supported by the Authorization provider you are using."));
+ new IllegalStateException("NamespaceOperation [" + operation.name() + "] is not supported by "
Review comment:
You include the `policy.name()` in the change of line 368. But why didn't you add the same change here?
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/client/api/AuthenticatedProducerConsumerTest.java
##########
@@ -245,15 +245,8 @@ public void testAnonymousSyncProducerAndConsumer(int batchMessageDelayMs) throws
replacePulsarClient(PulsarClient.builder().serviceUrl(pulsar.getBrokerServiceUrl())
.operationTimeout(1, TimeUnit.SECONDS));
- // unauthorized topic test
- Exception pulsarClientException = null;
- try {
- pulsarClient.newConsumer().topic("persistent://my-property/my-ns/other-topic")
- .subscriptionName("my-subscriber-name").subscribe();
- } catch (Exception e) {
- pulsarClientException = e;
- }
- Assert.assertTrue(pulsarClientException instanceof PulsarClientException);
+ pulsarClient.newConsumer().topic("persistent://my-property/my-ns/other-topic")
+ .subscriptionName("my-subscriber-name").subscribe();
Review comment:
No. `anonymousUser` shouldn't have the rights to the topics. This looks like a break change.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-785085889
@zymap this PR is painful to maintain with multiple rebases, can you have a look quickly?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a change in pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#discussion_r627769234
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/lookup/TopicLookupBase.java
##########
@@ -126,14 +128,17 @@ protected void internalLookupTopicAsync(TopicName topicName, boolean authoritati
private void validateAdminAndClientPermission(TopicName topic) throws RestException, Exception {
try {
- validateAdminAccessForTenant(topic.getTenant());
+ validateTopicOperation(topic, TopicOperation.LOOKUP);
} catch (Exception e) {
- checkConnect(topic);
+ // unknown error marked as internal server error
+ log.warn("Unexpected error while authorizing TopicOperation.LOOKUP. topic={}, role={}. Error: {}",
Review comment:
Do we need to log here? The exception would be expected if the client is not authorized and since it's already bubbled up, we would end up logging that twice.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on a change in pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on a change in pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#discussion_r484122817
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java
##########
@@ -489,4 +490,36 @@ default Boolean allowTopicOperation(TopicName topicName,
throw new RestException(e.getCause());
}
}
+
+ /**
+ * Check if a given <tt>role</tt> is allowed to execute a given topic <tt>operation</tt> on topic's <tt>policy</tt>.
+ *
+ * @param topic topic name
+ * @param role role name
+ * @param operation topic operation
+ * @param authData authenticated data
+ * @return CompletableFuture<Boolean>
+ */
+ default CompletableFuture<Boolean> allowTopicPolicyOperationAsync(TopicName topic,
+ String role,
+ PolicyName policy,
+ PolicyOperation operation,
+ AuthenticationDataSource authData) {
+ return FutureUtil.failedFuture(
Review comment:
@sijie
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on a change in pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on a change in pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#discussion_r627777823
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/lookup/TopicLookupBase.java
##########
@@ -126,14 +128,17 @@ protected void internalLookupTopicAsync(TopicName topicName, boolean authoritati
private void validateAdminAndClientPermission(TopicName topic) throws RestException, Exception {
try {
- validateAdminAccessForTenant(topic.getTenant());
+ validateTopicOperation(topic, TopicOperation.LOOKUP);
} catch (Exception e) {
- checkConnect(topic);
+ // unknown error marked as internal server error
+ log.warn("Unexpected error while authorizing TopicOperation.LOOKUP. topic={}, role={}. Error: {}",
Review comment:
Dropped in 596d85f418efe40b50114da3d4a6d126cb796882.
PR rebased due to a recent conflict.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on a change in pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on a change in pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#discussion_r483518537
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java
##########
@@ -489,4 +490,36 @@ default Boolean allowTopicOperation(TopicName topicName,
throw new RestException(e.getCause());
}
}
+
+ /**
+ * Check if a given <tt>role</tt> is allowed to execute a given topic <tt>operation</tt> on topic's <tt>policy</tt>.
+ *
+ * @param topic topic name
+ * @param role role name
+ * @param operation topic operation
+ * @param authData authenticated data
+ * @return CompletableFuture<Boolean>
+ */
+ default CompletableFuture<Boolean> allowTopicPolicyOperationAsync(TopicName topic,
+ String role,
+ PolicyName policy,
+ PolicyOperation operation,
+ AuthenticationDataSource authData) {
+ return FutureUtil.failedFuture(
Review comment:
So call `return isTenantAdmin(topic.getTenant(), role, tenantInfo , authData);`, correct? How to get tenantInfo from here?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on a change in pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#discussion_r556757442
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java
##########
@@ -489,4 +490,36 @@ default Boolean allowTopicOperation(TopicName topicName,
throw new RestException(e.getCause());
}
}
+
+ /**
+ * Check if a given <tt>role</tt> is allowed to execute a given topic <tt>operation</tt> on topic's <tt>policy</tt>.
+ *
+ * @param topic topic name
+ * @param role role name
+ * @param operation topic operation
+ * @param authData authenticated data
+ * @return CompletableFuture<Boolean>
+ */
+ default CompletableFuture<Boolean> allowTopicPolicyOperationAsync(TopicName topic,
+ String role,
+ PolicyName policy,
+ PolicyOperation operation,
+ AuthenticationDataSource authData) {
+ return FutureUtil.failedFuture(
Review comment:
@zymap is working on some authorization changes. @zymap Can you take a look at this pull request?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-820561514
@zymap @315157973 @hangc0276
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-798988339
@zymap @sijie can you trigger with more that 45min of run?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr removed a comment on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr removed a comment on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-776020646
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on a change in pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on a change in pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#discussion_r568657436
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java
##########
@@ -489,4 +490,36 @@ default Boolean allowTopicOperation(TopicName topicName,
throw new RestException(e.getCause());
}
}
+
+ /**
+ * Check if a given <tt>role</tt> is allowed to execute a given topic <tt>operation</tt> on topic's <tt>policy</tt>.
+ *
+ * @param topic topic name
+ * @param role role name
+ * @param operation topic operation
+ * @param authData authenticated data
+ * @return CompletableFuture<Boolean>
+ */
+ default CompletableFuture<Boolean> allowTopicPolicyOperationAsync(TopicName topic,
+ String role,
+ PolicyName policy,
+ PolicyOperation operation,
+ AuthenticationDataSource authData) {
+ return FutureUtil.failedFuture(
Review comment:
It does.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-802244383
/pulsarbot run-failure-checks
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-760280345
https://github.com/apache/pulsar/issues/9179
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Geal commented on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
Geal commented on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-831106081
@sijie @zymap hey, is there anything really blocking this PR, except a review? How can we help you get it merged?
This PR has been going on for over a year now, with @KannarFr spending significant time rebasing it over and over. And we identified the need for Pulsar to get truly multitenant more than 2 years ago ( https://github.com/apache/pulsar/issues/5720 ). So this is getting a bit long for a feature that is advertised on Pulsar's website from day one.
The fact is that it has been running fine on our systems for a long time, where we manage very fine rights, so there is much less risks in merging it that you might think.
So, again, how can we help you get this merged?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-801891017
rebased
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr removed a comment on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr removed a comment on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-777172053
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on a change in pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on a change in pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#discussion_r568657436
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java
##########
@@ -489,4 +490,36 @@ default Boolean allowTopicOperation(TopicName topicName,
throw new RestException(e.getCause());
}
}
+
+ /**
+ * Check if a given <tt>role</tt> is allowed to execute a given topic <tt>operation</tt> on topic's <tt>policy</tt>.
+ *
+ * @param topic topic name
+ * @param role role name
+ * @param operation topic operation
+ * @param authData authenticated data
+ * @return CompletableFuture<Boolean>
+ */
+ default CompletableFuture<Boolean> allowTopicPolicyOperationAsync(TopicName topic,
+ String role,
+ PolicyName policy,
+ PolicyOperation operation,
+ AuthenticationDataSource authData) {
+ return FutureUtil.failedFuture(
Review comment:
It does.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-826773874
@zymap ?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-823689937
Rebased to support 889b9b8e5efc62d2d0cbc761205fba5759c97af0.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on a change in pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on a change in pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#discussion_r595065707
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/client/api/AuthenticatedProducerConsumerTest.java
##########
@@ -245,15 +245,8 @@ public void testAnonymousSyncProducerAndConsumer(int batchMessageDelayMs) throws
replacePulsarClient(PulsarClient.builder().serviceUrl(pulsar.getBrokerServiceUrl())
.operationTimeout(1, TimeUnit.SECONDS));
- // unauthorized topic test
- Exception pulsarClientException = null;
- try {
- pulsarClient.newConsumer().topic("persistent://my-property/my-ns/other-topic")
- .subscriptionName("my-subscriber-name").subscribe();
- } catch (Exception e) {
- pulsarClientException = e;
- }
- Assert.assertTrue(pulsarClientException instanceof PulsarClientException);
+ pulsarClient.newConsumer().topic("persistent://my-property/my-ns/other-topic")
+ .subscriptionName("my-subscriber-name").subscribe();
Review comment:
@sijie can you confirm that is change is correct regarding the test `anonymousUser` has all rights to the topics so this newConsumer should pass. Correct?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] kyomster commented on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
kyomster commented on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-789876182
I too need this PR. please @sijie
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zymap commented on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
zymap commented on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-785554197
ping @sijie. Please take a review. Thanks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr removed a comment on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr removed a comment on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-776606883
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-803940121
@sijie @zymap this is ready to review
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-776014685
@zymap can you run the pulsar-broker test for this branch and resolve the issue between allowTopicOperation and checkPermission for subscriptions. Can't make it work. I think you have a vision of how you want to manage permission for the internal authz provider.
Can't make testSubscriberPermission pass.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr removed a comment on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr removed a comment on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-672866523
@sijie A lot of conflicts due to 48f5a2f62c148b3df617be060fefed51f3145979 :o, anyway can you review this?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-776611079
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] frankjkelly commented on a change in pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
frankjkelly commented on a change in pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#discussion_r479665361
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -846,4 +847,36 @@ public void validateNamespacePolicyOperation(NamespaceName namespaceName,
}
}
}
+
+ public void validateTopicPolicyOperation(TopicName topicName, PolicyName policy, PolicyOperation operation) {
+ if (pulsar().getConfiguration().isAuthenticationEnabled() && pulsar().getBrokerService().isAuthorizationEnabled()) {
+ if (!isClientAuthenticated(clientAppId())) {
+ throw new RestException(Status.FORBIDDEN, "Need to authenticate to perform the request");
Review comment:
Probably should return `Status.UNAUTHORIZED` rather than `Status.FORBIDDEN` to avoid confusion?
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -846,4 +847,36 @@ public void validateNamespacePolicyOperation(NamespaceName namespaceName,
}
}
}
+
+ public void validateTopicPolicyOperation(TopicName topicName, PolicyName policy, PolicyOperation operation) {
+ if (pulsar().getConfiguration().isAuthenticationEnabled() && pulsar().getBrokerService().isAuthorizationEnabled()) {
+ if (!isClientAuthenticated(clientAppId())) {
+ throw new RestException(Status.FORBIDDEN, "Need to authenticate to perform the request");
+ }
+
+ Boolean isAuthorized = pulsar().getBrokerService().getAuthorizationService()
+ .allowTopicPolicyOperation(topicName, policy, operation, originalPrincipal(), clientAppId(), clientAuthData());
+
+ if (!isAuthorized) {
+ throw new RestException(Status.FORBIDDEN, String.format("Unauthorized to validateTopicPolicyOperation for" +
+ " operation [%s] on topic [%s] on policy [%s]", operation.toString(), topicName, policy.toString()));
+ }
+ }
+ }
+
+ public void validateTopicOperation(TopicName topicName, TopicOperation operation) {
+ if (pulsar().getConfiguration().isAuthenticationEnabled() && pulsar().getBrokerService().isAuthorizationEnabled()) {
+ if (!isClientAuthenticated(clientAppId())) {
+ throw new RestException(Status.UNAUTHORIZED, "Need to authenticate to perform the request");
Review comment:
Same as above?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-796800293
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zymap commented on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
zymap commented on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-833150597
Sorry for the delay. I will take a look soon.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie merged pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
sijie merged pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on a change in pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#discussion_r591696226
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java
##########
@@ -545,29 +546,88 @@ private void validatePoliciesReadOnlyAccess() {
CompletableFuture<Boolean> isAuthorizedFuture;
switch (operation) {
- case LOOKUP: isAuthorizedFuture = canLookupAsync(topicName, role, authData);
+ case LOOKUP:
+ case GET_STATS:
+ case GET_SUBSCRIPTIONS:
+ isAuthorizedFuture = canLookupAsync(topicName, role, authData)
+ .thenCombine(validateTenantAdminAccess(topicName.getTenant(), role, authData),
Review comment:
for `LOOKUP`, the original behavior is checking `canLookupAsync`. Why do we need to check `validateTenantAdminAccess`?
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java
##########
@@ -545,29 +546,88 @@ private void validatePoliciesReadOnlyAccess() {
CompletableFuture<Boolean> isAuthorizedFuture;
switch (operation) {
- case LOOKUP: isAuthorizedFuture = canLookupAsync(topicName, role, authData);
+ case LOOKUP:
+ case GET_STATS:
+ case GET_SUBSCRIPTIONS:
+ isAuthorizedFuture = canLookupAsync(topicName, role, authData)
+ .thenCombine(validateTenantAdminAccess(topicName.getTenant(), role, authData),
+ (isTenantAdmin, isAuthorized) -> {
+ if (log.isDebugEnabled()) {
+ log.debug("Verify if role {} is allowed to {} to topic {}:"
+ + " isSuperUser={}, isTenantAdmin={}",
+ role, operation, topicName, isTenantAdmin, isAuthorized);
+ }
+ return isTenantAdmin || isAuthorized;
+ });
+ break;
+ case PRODUCE:
+ isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case TERMINATE:
+ isAuthorizedFuture = canProduceAsync(topicName, role, authData)
Review comment:
why do we need to check `canProduceAsync` for `TERMINATE`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-776606883
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zymap commented on a change in pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
zymap commented on a change in pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#discussion_r606658664
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java
##########
@@ -542,30 +543,56 @@ private void validatePoliciesReadOnlyAccess() {
String role,
TopicOperation operation,
AuthenticationDataSource authData) {
+ log.debug("Check allowTopicOperationAsync [" + operation.name() + "] on [" + topicName.toString() + "].");
+
CompletableFuture<Boolean> isAuthorizedFuture;
switch (operation) {
- case LOOKUP: isAuthorizedFuture = canLookupAsync(topicName, role, authData);
+ case LOOKUP:
+ case GET_STATS:
+ case GET_SUBSCRIPTIONS:
Review comment:
I checked the get subscriptions API, originally, it uses `validationReadOperationOnTopic` which requires a tenant admin permission or a consume permission. After this change, it will also request produce permission.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-784914049
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-777159446
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on a change in pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on a change in pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#discussion_r607715091
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java
##########
@@ -542,30 +543,56 @@ private void validatePoliciesReadOnlyAccess() {
String role,
TopicOperation operation,
AuthenticationDataSource authData) {
+ log.debug("Check allowTopicOperationAsync [" + operation.name() + "] on [" + topicName.toString() + "].");
+
CompletableFuture<Boolean> isAuthorizedFuture;
switch (operation) {
- case LOOKUP: isAuthorizedFuture = canLookupAsync(topicName, role, authData);
+ case LOOKUP:
+ case GET_STATS:
+ case GET_SUBSCRIPTIONS:
Review comment:
Fixed.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-803657557
@zymap @sijie can you rerun with more time before autocancel? And review?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on a change in pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#discussion_r483377081
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java
##########
@@ -489,4 +490,36 @@ default Boolean allowTopicOperation(TopicName topicName,
throw new RestException(e.getCause());
}
}
+
+ /**
+ * Check if a given <tt>role</tt> is allowed to execute a given topic <tt>operation</tt> on topic's <tt>policy</tt>.
+ *
+ * @param topic topic name
+ * @param role role name
+ * @param operation topic operation
+ * @param authData authenticated data
+ * @return CompletableFuture<Boolean>
+ */
+ default CompletableFuture<Boolean> allowTopicPolicyOperationAsync(TopicName topic,
+ String role,
+ PolicyName policy,
+ PolicyOperation operation,
+ AuthenticationDataSource authData) {
+ return FutureUtil.failedFuture(
Review comment:
Can you make the default implementation to use the existing method? If people wrote its own plugin, it will fail all the topic policy operations when it upgrades to a newer version of Pulsar.
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java
##########
@@ -564,6 +564,36 @@ private void validatePoliciesReadOnlyAccess() {
break;
case CONSUME: isAuthorizedFuture = canConsumeAsync(topicName, role, authData, authData.getSubscription());
break;
+ case COMPACT: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
Review comment:
compact should be an admin operation, correct?
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java
##########
@@ -564,6 +564,36 @@ private void validatePoliciesReadOnlyAccess() {
break;
case CONSUME: isAuthorizedFuture = canConsumeAsync(topicName, role, authData, authData.getSubscription());
break;
+ case COMPACT: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case EXPIRE_MESSAGES: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case OFFLOAD: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case PEEK_MESSAGES: isAuthorizedFuture = canConsumeAsync(topicName, role, authData, authData.getSubscription());
+ break;
+ case RESET_CURSOR: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case SKIP: isAuthorizedFuture = canConsumeAsync(topicName, role, authData, authData.getSubscription());
+ break;
+ case TERMINATE: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case UNLOAD: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case ADD_BUNDLE_RANGE: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case GET_BUNDLE_RANGE: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case DELETE_BUNDLE_RANGE: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
Review comment:
admin?
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java
##########
@@ -564,6 +564,36 @@ private void validatePoliciesReadOnlyAccess() {
break;
case CONSUME: isAuthorizedFuture = canConsumeAsync(topicName, role, authData, authData.getSubscription());
break;
+ case COMPACT: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case EXPIRE_MESSAGES: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case OFFLOAD: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
Review comment:
admin?
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java
##########
@@ -564,6 +564,36 @@ private void validatePoliciesReadOnlyAccess() {
break;
case CONSUME: isAuthorizedFuture = canConsumeAsync(topicName, role, authData, authData.getSubscription());
break;
+ case COMPACT: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case EXPIRE_MESSAGES: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
Review comment:
expire messages is a consume operation, correct?
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationService.java
##########
@@ -364,13 +353,11 @@ private boolean isProxyRole(String role) {
* Grant authorization-action permission on a tenant to the given client
*
* @param tenantName tenant name
- * @param operation tenant operation
- * @param role role name
- * @param authData
- * additional authdata in json for targeted authorization provider
+ * @param operation tenant operation
Review comment:
Can you avoid formatting the file with other changes?
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java
##########
@@ -564,6 +564,36 @@ private void validatePoliciesReadOnlyAccess() {
break;
case CONSUME: isAuthorizedFuture = canConsumeAsync(topicName, role, authData, authData.getSubscription());
break;
+ case COMPACT: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case EXPIRE_MESSAGES: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case OFFLOAD: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case PEEK_MESSAGES: isAuthorizedFuture = canConsumeAsync(topicName, role, authData, authData.getSubscription());
+ break;
+ case RESET_CURSOR: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case SKIP: isAuthorizedFuture = canConsumeAsync(topicName, role, authData, authData.getSubscription());
+ break;
+ case TERMINATE: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case UNLOAD: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case ADD_BUNDLE_RANGE: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
Review comment:
admin?
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java
##########
@@ -564,6 +564,36 @@ private void validatePoliciesReadOnlyAccess() {
break;
case CONSUME: isAuthorizedFuture = canConsumeAsync(topicName, role, authData, authData.getSubscription());
break;
+ case COMPACT: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case EXPIRE_MESSAGES: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case OFFLOAD: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case PEEK_MESSAGES: isAuthorizedFuture = canConsumeAsync(topicName, role, authData, authData.getSubscription());
+ break;
+ case RESET_CURSOR: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case SKIP: isAuthorizedFuture = canConsumeAsync(topicName, role, authData, authData.getSubscription());
+ break;
+ case TERMINATE: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case UNLOAD: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
Review comment:
admin?
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java
##########
@@ -564,6 +564,36 @@ private void validatePoliciesReadOnlyAccess() {
break;
case CONSUME: isAuthorizedFuture = canConsumeAsync(topicName, role, authData, authData.getSubscription());
break;
+ case COMPACT: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case EXPIRE_MESSAGES: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case OFFLOAD: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case PEEK_MESSAGES: isAuthorizedFuture = canConsumeAsync(topicName, role, authData, authData.getSubscription());
+ break;
+ case RESET_CURSOR: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
Review comment:
consume?
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java
##########
@@ -564,6 +564,36 @@ private void validatePoliciesReadOnlyAccess() {
break;
case CONSUME: isAuthorizedFuture = canConsumeAsync(topicName, role, authData, authData.getSubscription());
break;
+ case COMPACT: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case EXPIRE_MESSAGES: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case OFFLOAD: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case PEEK_MESSAGES: isAuthorizedFuture = canConsumeAsync(topicName, role, authData, authData.getSubscription());
+ break;
+ case RESET_CURSOR: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case SKIP: isAuthorizedFuture = canConsumeAsync(topicName, role, authData, authData.getSubscription());
+ break;
+ case TERMINATE: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case UNLOAD: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case ADD_BUNDLE_RANGE: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
+ break;
+ case GET_BUNDLE_RANGE: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
Review comment:
admin?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-777172053
@zymap @sijie can you review?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zymap commented on a change in pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
zymap commented on a change in pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#discussion_r559294631
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java
##########
@@ -557,28 +557,65 @@ private void validatePoliciesReadOnlyAccess() {
AuthenticationDataSource authData) {
CompletableFuture<Boolean> isAuthorizedFuture;
- switch (operation) {
- case LOOKUP: isAuthorizedFuture = canLookupAsync(topicName, role, authData);
- break;
- case PRODUCE: isAuthorizedFuture = canProduceAsync(topicName, role, authData);
- break;
- case CONSUME: isAuthorizedFuture = canConsumeAsync(topicName, role, authData, authData.getSubscription());
- break;
- default: isAuthorizedFuture = FutureUtil.failedFuture(
- new IllegalStateException("TopicOperation is not supported."));
- }
+ try {
+ TenantInfo tenantInfo = configCache.propertiesCache().get(path(POLICIES, topicName.getTenant())).get();
+
+ switch (operation) {
Review comment:
Can we merge some operations like this? For example:
```
case CONSUME:
case EXPIRE_MESAGES:
...
case SKIP:
isAuthorizedFuture = canConsumeAsync(topicName, role, authData, authData.getSubscription());
break;
```
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java
##########
@@ -489,4 +490,36 @@ default Boolean allowTopicOperation(TopicName topicName,
throw new RestException(e.getCause());
}
}
+
+ /**
+ * Check if a given <tt>role</tt> is allowed to execute a given topic <tt>operation</tt> on topic's <tt>policy</tt>.
+ *
+ * @param topic topic name
+ * @param role role name
+ * @param operation topic operation
+ * @param authData authenticated data
+ * @return CompletableFuture<Boolean>
+ */
+ default CompletableFuture<Boolean> allowTopicPolicyOperationAsync(TopicName topic,
+ String role,
+ PolicyName policy,
+ PolicyOperation operation,
+ AuthenticationDataSource authData) {
+ return FutureUtil.failedFuture(
Review comment:
Overall looks good. This PR looks like only add the validation for the topic level authorization.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zymap commented on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
zymap commented on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-790479687
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-778362011
@sijie @zymap are you ok?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-672866523
@sijie A lot of conflicts due to 48f5a2f62c148b3df617be060fefed51f3145979 :o, anyway can you review this?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-776020646
In AuthorizationProducerConsumerTest.testSubscriberPermission,
```java
// grant namespace-level authorization to the subscriptionRole
tenantAdmin.namespaces().grantPermissionOnNamespace(namespace, subscriptionRole,
Collections.singleton(AuthAction.consume));
// subscriptionRole has namespace-level authorization
sub1Admin.topics().resetCursor(topicName, subscriptionName, 10); // THIS PASS
// grant subscription access to specific different role and only that role can access the subscription
String otherPrincipal = "Principal-1-to-access-sub";
tenantAdmin.namespaces().grantPermissionOnSubscription(namespace, subscriptionName,
Collections.singleton(otherPrincipal));
// now, subscriptionRole doesn't have subscription level access so, it will fail to access subscription
try {
sub1Admin.topics().resetCursor(topicName, subscriptionName, 10); // THIS PASS BUT MUST NOT PASS
fail("should have fail with authorization exception");
} catch (org.apache.pulsar.client.admin.PulsarAdminException.NotAuthorizedException e) {
// Ok
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on a change in pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on a change in pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#discussion_r595065707
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/client/api/AuthenticatedProducerConsumerTest.java
##########
@@ -245,15 +245,8 @@ public void testAnonymousSyncProducerAndConsumer(int batchMessageDelayMs) throws
replacePulsarClient(PulsarClient.builder().serviceUrl(pulsar.getBrokerServiceUrl())
.operationTimeout(1, TimeUnit.SECONDS));
- // unauthorized topic test
- Exception pulsarClientException = null;
- try {
- pulsarClient.newConsumer().topic("persistent://my-property/my-ns/other-topic")
- .subscriptionName("my-subscriber-name").subscribe();
- } catch (Exception e) {
- pulsarClientException = e;
- }
- Assert.assertTrue(pulsarClientException instanceof PulsarClientException);
+ pulsarClient.newConsumer().topic("persistent://my-property/my-ns/other-topic")
+ .subscriptionName("my-subscriber-name").subscribe();
Review comment:
@sijie can you confirm that this change is correct regarding the test `anonymousUser` has all rights to the topics so this new consumer should pass. Correct?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#issuecomment-789869881
@sijie @zymap rebased
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on a change in pull request #7523: [Issue 5720][authz] - add topics authz granularity
Posted by GitBox <gi...@apache.org>.
KannarFr commented on a change in pull request #7523:
URL: https://github.com/apache/pulsar/pull/7523#discussion_r483520677
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -846,4 +847,36 @@ public void validateNamespacePolicyOperation(NamespaceName namespaceName,
}
}
}
+
+ public void validateTopicPolicyOperation(TopicName topicName, PolicyName policy, PolicyOperation operation) {
+ if (pulsar().getConfiguration().isAuthenticationEnabled() && pulsar().getBrokerService().isAuthorizationEnabled()) {
+ if (!isClientAuthenticated(clientAppId())) {
+ throw new RestException(Status.FORBIDDEN, "Need to authenticate to perform the request");
Review comment:
Probably but the rest of this stuff content is using forbidden today.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -846,4 +847,36 @@ public void validateNamespacePolicyOperation(NamespaceName namespaceName,
}
}
}
+
+ public void validateTopicPolicyOperation(TopicName topicName, PolicyName policy, PolicyOperation operation) {
+ if (pulsar().getConfiguration().isAuthenticationEnabled() && pulsar().getBrokerService().isAuthorizationEnabled()) {
+ if (!isClientAuthenticated(clientAppId())) {
+ throw new RestException(Status.FORBIDDEN, "Need to authenticate to perform the request");
+ }
+
+ Boolean isAuthorized = pulsar().getBrokerService().getAuthorizationService()
+ .allowTopicPolicyOperation(topicName, policy, operation, originalPrincipal(), clientAppId(), clientAuthData());
+
+ if (!isAuthorized) {
+ throw new RestException(Status.FORBIDDEN, String.format("Unauthorized to validateTopicPolicyOperation for" +
+ " operation [%s] on topic [%s] on policy [%s]", operation.toString(), topicName, policy.toString()));
+ }
+ }
+ }
+
+ public void validateTopicOperation(TopicName topicName, TopicOperation operation) {
+ if (pulsar().getConfiguration().isAuthenticationEnabled() && pulsar().getBrokerService().isAuthorizationEnabled()) {
+ if (!isClientAuthenticated(clientAppId())) {
+ throw new RestException(Status.UNAUTHORIZED, "Need to authenticate to perform the request");
Review comment:
Same as above.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org