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 2021/12/02 10:47:50 UTC
[GitHub] [pulsar] yuruguo opened a new pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
yuruguo opened a new pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090
### Motivation
Currently, pulsar supports 12 `NamespaceOperation`, so there should not be more than 12 in method `PulsarAuthorizationProvider#allowNamespaceOperationAsync`
https://github.com/apache/pulsar/blob/7adfffdecc2debacba2fa90c443cb992b826b0c0/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java#L525-L550
Otherwise, the undefined `NamespaceOperation` can also be passed by super-user or role with admin privileges.
The purpose of this PR is to optimize the logic of `PulsarAuthorizationProvider#allowNamespaceOperationAsync` so that it only covers the `NamespaceOperation` supported by pulsar.
### Documentation
- [x] `no-need-doc`
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-986239637
@eolivelli @michaeljmarshall ptal again
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo removed a comment on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo removed a comment on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-984596471
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985224325
@merlimat @massakam @codelipenghui @freeznet Can you help review this PR?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985990190
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo removed a comment on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo removed a comment on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985990190
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo removed a comment on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo removed a comment on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-984568763
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli merged pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo removed a comment on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo removed a comment on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-987766735
I have addressed comments.
Would @eolivelli @michaeljmarshall help review this PR again when you have a moment?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo removed a comment on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo removed a comment on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-986239637
@eolivelli @michaeljmarshall ptal again
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985189156
@merlimat @codelipenghui @freeznet Can you help review this PR :)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#discussion_r762397331
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java
##########
@@ -183,6 +183,16 @@ default void initialize(ServiceConfiguration conf, PulsarResources pulsarResourc
CompletableFuture<Boolean> allowSinkOpsAsync(NamespaceName namespaceName, String role,
AuthenticationDataSource authenticationData);
+ /**
+ * Allow package operations with in this namespace
+ * @param namespaceName The namespace that the package operations can be executed in
+ * @param role The role to check
+ * @param authenticationData authentication data related to the role
+ * @return a boolean to determine whether authorized or not
+ */
+ CompletableFuture<Boolean> allowPackageOpsAsync(NamespaceName namespaceName, String role,
Review comment:
Thank you for pointing out that new method. I think we should remove that one too, so I just submitted a PR to do so: https://github.com/apache/pulsar/pull/13133.
Note that I am fine with custom methods to wrap the `packages` operation. However, I think we need to be careful about how we expand our public API.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
KannarFr commented on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-987829239
@yuruguo Please note that authZ is pluggable. Even if the default AuthorizationProvider does not use these operations, it exists plugins that are not role-based but token-based and they need the operations information to authorize. IMO, you should not deprecate the Operations which are not used in default AuthorizationProvider.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on a change in pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on a change in pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#discussion_r760974394
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/NamespaceOperation.java
##########
@@ -24,7 +24,6 @@
*/
public enum NamespaceOperation {
CREATE_TOPIC,
- GET_TOPIC,
Review comment:
unused namespace operation so remove
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo edited a comment on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo edited a comment on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985173926
**Note**: The CI failure of `Unit / unit-tests` has nothing to do with this PR, it may be related to [13087](https://github.com/apache/pulsar/pull/13087)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo removed a comment on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo removed a comment on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-984574152
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985018851
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo removed a comment on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo removed a comment on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985118187
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo removed a comment on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo removed a comment on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985949392
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on a change in pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on a change in pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#discussion_r762401817
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java
##########
@@ -183,6 +183,16 @@ default void initialize(ServiceConfiguration conf, PulsarResources pulsarResourc
CompletableFuture<Boolean> allowSinkOpsAsync(NamespaceName namespaceName, String role,
AuthenticationDataSource authenticationData);
+ /**
+ * Allow package operations with in this namespace
+ * @param namespaceName The namespace that the package operations can be executed in
+ * @param role The role to check
+ * @param authenticationData authentication data related to the role
+ * @return a boolean to determine whether authorized or not
+ */
+ CompletableFuture<Boolean> allowPackageOpsAsync(NamespaceName namespaceName, String role,
Review comment:
Okay! I have remove `allowPackageOpsAsync` method and directly use `allowTheSpecifiedActionOpsAsync`.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on a change in pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on a change in pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#discussion_r761017673
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/NamespaceOperation.java
##########
@@ -24,7 +24,6 @@
*/
public enum NamespaceOperation {
CREATE_TOPIC,
- GET_TOPIC,
Review comment:
I went back a bit, `GET_TOPIC` was originally defined [here](https://github.com/apache/pulsar/pull/6428/files#diff-d7e8390b6795670a5d992eea3966a36e08a572a08d32ea77da370018ea086830R27-R28) but it was not used, and `GET_TOPICS` was used.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo removed a comment on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo removed a comment on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-984556672
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-984611553
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo removed a comment on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo removed a comment on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-984717719
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985944630
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo removed a comment on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo removed a comment on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-984611553
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on a change in pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on a change in pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#discussion_r761637953
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/NamespaceOperation.java
##########
@@ -24,7 +24,6 @@
*/
public enum NamespaceOperation {
CREATE_TOPIC,
- GET_TOPIC,
Review comment:
You are welcome :)
Can you take a look at this PR and are there any other questions?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-988468677
> @yuruguo Please note that authZ is pluggable. Even if the default AuthorizationProvider does not use these operations, it exists plugins that are not role-based but token-based and they need the operations information to authorize. IMO, you should not deprecate the Operations which are not used in default AuthorizationProvider.
@KannarFr - I agree that we shouldn't just deprecate fields because they are not used by the `PulsarAuthorizationProvider`. It's not clear to me why we need fields that are not present in the rest of the code base, though. Which fields are you opposed to deprecating? Above, I mention some fields that I think we could deprecate. Perhaps I am missing context on some of those fields though, and we should keep them. If that is the case, it'd be great to add some comments to the associated fields to add documentation within the code base.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo removed a comment on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo removed a comment on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-986005255
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-984596471
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo removed a comment on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo removed a comment on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-984625862
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-984717719
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on a change in pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on a change in pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#discussion_r761637953
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/NamespaceOperation.java
##########
@@ -24,7 +24,6 @@
*/
public enum NamespaceOperation {
CREATE_TOPIC,
- GET_TOPIC,
Review comment:
You are welcome :)
Can you take a look at this PR and are there any other questions?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-984568763
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985092758
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo removed a comment on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo removed a comment on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985099563
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985949392
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo removed a comment on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo removed a comment on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985936983
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on a change in pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on a change in pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#discussion_r761966219
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java
##########
@@ -536,17 +536,32 @@ private void validatePoliciesReadOnlyAccess() {
case CLEAR_BACKLOG:
isAuthorizedFuture = allowConsumeOpsAsync(namespaceName, role, authData);
break;
+ case CREATE_TOPIC:
+ case DELETE_TOPIC:
+ case ADD_BUNDLE:
+ case GET_BUNDLE:
+ case DELETE_BUNDLE:
+ case GRANT_PERMISSION:
+ case GET_PERMISSION:
+ case REVOKE_PERMISSION:
+ return validateTenantAdminAccess(namespaceName.getTenant(), role, authData);
default:
- isAuthorizedFuture = CompletableFuture.completedFuture(false);
+ return FutureUtil.failedFuture(
+ new IllegalStateException("NamespaceOperation [" + operation.name() + "] is not supported."));
}
- CompletableFuture<Boolean> isTenantAdminFuture = validateTenantAdminAccess(namespaceName.getTenant(), role, authData);
- return isTenantAdminFuture.thenCombine(isAuthorizedFuture, (isTenantAdmin, isAuthorized) -> {
- if (log.isDebugEnabled()) {
- log.debug("Verify if role {} is allowed to {} to topic {}: isTenantAdmin={}, isAuthorized={}",
- role, operation, namespaceName, isTenantAdmin, isAuthorized);
- }
- return isTenantAdmin || isAuthorized;
- });
+
+ return validateTenantAdminAccess(namespaceName.getTenant(), role, authData)
+ .thenCompose(isSuperUserOrAdmin -> {
+ if (log.isDebugEnabled()) {
+ log.debug("Verify if role {} is allowed to {} to namespace {}: isSuperUserOrAdmin={}",
+ role, operation, namespaceName, isSuperUserOrAdmin);
+ }
+ if (isSuperUserOrAdmin) {
+ return CompletableFuture.completedFuture(true);
+ } else {
+ return isAuthorizedFuture;
Review comment:
Great sugesstion!
I have adjusted the order of permission judgment, included `PulsarAuthorizationProvider#allowTopicOperationAsync`
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo removed a comment on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo removed a comment on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985018851
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo removed a comment on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo removed a comment on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985130513
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo removed a comment on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo removed a comment on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985189156
@merlimat @codelipenghui @freeznet Can you help review this PR :)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#discussion_r761997548
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java
##########
@@ -526,27 +532,39 @@ private void validatePoliciesReadOnlyAccess() {
String role,
NamespaceOperation operation,
AuthenticationDataSource authData) {
- CompletableFuture<Boolean> isAuthorizedFuture;
- switch (operation) {
- case PACKAGES:
- isAuthorizedFuture = allowTheSpecifiedActionOpsAsync(namespaceName, role, authData, AuthAction.packages);
- break;
- case GET_TOPICS:
- case UNSUBSCRIBE:
- case CLEAR_BACKLOG:
- isAuthorizedFuture = allowConsumeOpsAsync(namespaceName, role, authData);
- break;
- default:
- isAuthorizedFuture = CompletableFuture.completedFuture(false);
- }
- CompletableFuture<Boolean> isTenantAdminFuture = validateTenantAdminAccess(namespaceName.getTenant(), role, authData);
- return isTenantAdminFuture.thenCombine(isAuthorizedFuture, (isTenantAdmin, isAuthorized) -> {
- if (log.isDebugEnabled()) {
- log.debug("Verify if role {} is allowed to {} to topic {}: isTenantAdmin={}, isAuthorized={}",
- role, operation, namespaceName, isTenantAdmin, isAuthorized);
- }
- return isTenantAdmin || isAuthorized;
- });
+ log.debug("Check allowNamespaceOperationAsync [{}] on [{}].", operation.name(), namespaceName.toString());
Review comment:
please avoid calling explicitly "toString"
also add "if log.isDebugEnabled()" guard
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on a change in pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on a change in pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#discussion_r762353763
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java
##########
@@ -183,6 +183,16 @@ default void initialize(ServiceConfiguration conf, PulsarResources pulsarResourc
CompletableFuture<Boolean> allowSinkOpsAsync(NamespaceName namespaceName, String role,
AuthenticationDataSource authenticationData);
+ /**
+ * Allow package operations with in this namespace
+ * @param namespaceName The namespace that the package operations can be executed in
+ * @param role The role to check
+ * @param authenticationData authentication data related to the role
+ * @return a boolean to determine whether authorized or not
+ */
+ CompletableFuture<Boolean> allowPackageOpsAsync(NamespaceName namespaceName, String role,
Review comment:
It is clearer to wrap the `packages` operation into proprietary methods, which is the same as `allowConsumeOpsAsync`, although `allowConsumeOpsAsync` also only be used in internally by the `PulsarAuthorizationProvider` and `MultiRolesTokenAuthorizationProvider`. I have no objection if you insist on your point of view :)
https://github.com/apache/pulsar/blob/2e963a30a112a715f8a97682d671a0e8eab6cb81/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java#L228-L231
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo removed a comment on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo removed a comment on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985944630
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-986005255
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-987263389
> @yuruguo - unfortunately, my PR #13133 created merge conflicts for this PR, so you'll need to rebase.
>
> Based on looking at this PR and at #6428, I think there are a few opportunities for cleanup, but I need to look at the API a little more before I have a fully formed opinion. For example, I think we might be able to add the `@Deprecated` annotation for several enum fields. Here are the ones that I am tentatively considering: `NamespaceOperation.GET_TOPIC`, `NamespaceOperation.ADD_BUNDLE`, `TopicOperation.ADD_BUNDLE_RANGE`, `TopicOperation.GET_BUNDLE_RANGE`, and `TopicOperation.DELETE_BUNDLE_RANGE`. These seem unnecessary, but I might be missing something. It'd probably be worth an email to the dev mailing list before we officially deprecate these fields.
>
> I also see that we have these unused fields: `TopicOperation.GRANT_PERMISSION`, `TopicOperation.GET_PERMISSION`, and `TopicOperation.REVOKE_PERMISSION`. Perhaps they should be used, though, if we want to support topic level granularity for permissions. The current implementation does not rely on these 3 fields because the `PersistentTopicsBase#internalGrantPermissionsOnTopic` implements logic that I think should reside in an `AuthorizationProvider`.
Nice!
`TenantOperation` and its enumeration are also useless because we only need to judge whether the `role` is a tenant administrator, so we may also need to take a look it.
https://github.com/apache/pulsar/blob/50ab411305fb616439b3313926b7cb9e85ccbec5/pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/TenantOperation.java#L22-L29
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-987766735
I have addressed comments.
Would @eolivelli @michaeljmarshall help review this PR again when you have a moment?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo edited a comment on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo edited a comment on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-987263389
> @yuruguo - unfortunately, my PR #13133 created merge conflicts for this PR, so you'll need to rebase.
>
> Based on looking at this PR and at #6428, I think there are a few opportunities for cleanup, but I need to look at the API a little more before I have a fully formed opinion. For example, I think we might be able to add the `@Deprecated` annotation for several enum fields. Here are the ones that I am tentatively considering: `NamespaceOperation.GET_TOPIC`, `NamespaceOperation.ADD_BUNDLE`, `TopicOperation.ADD_BUNDLE_RANGE`, `TopicOperation.GET_BUNDLE_RANGE`, and `TopicOperation.DELETE_BUNDLE_RANGE`. These seem unnecessary, but I might be missing something. It'd probably be worth an email to the dev mailing list before we officially deprecate these fields.
>
> I also see that we have these unused fields: `TopicOperation.GRANT_PERMISSION`, `TopicOperation.GET_PERMISSION`, and `TopicOperation.REVOKE_PERMISSION`. Perhaps they should be used, though, if we want to support topic level granularity for permissions. The current implementation does not rely on these 3 fields because the `PersistentTopicsBase#internalGrantPermissionsOnTopic` implements logic that I think should reside in an `AuthorizationProvider`.
Nice!
~~`TenantOperation` and its enumeration are also useless because we only need to judge whether the `role` is a tenant administrator, so we may also need to take a look it.~~ For more information, see @KannarFr 's [reply](https://github.com/apache/pulsar/pull/13090#issuecomment-987829239)
https://github.com/apache/pulsar/blob/50ab411305fb616439b3313926b7cb9e85ccbec5/pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/TenantOperation.java#L22-L29
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-984574152
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#discussion_r760992737
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/NamespaceOperation.java
##########
@@ -24,7 +24,6 @@
*/
public enum NamespaceOperation {
CREATE_TOPIC,
- GET_TOPIC,
Review comment:
is this a kind of breaking change ? why did we add this value ?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#discussion_r761077848
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/NamespaceOperation.java
##########
@@ -24,7 +24,6 @@
*/
public enum NamespaceOperation {
CREATE_TOPIC,
- GET_TOPIC,
Review comment:
thanks for your explanation
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985099563
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo removed a comment on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo removed a comment on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985173926
**Note**: The CI failure of `Unit / unit-tests` has nothing to do with this PR, it may be related to [13087](https://github.com/apache/pulsar/pull/13087) @lhotari
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985936983
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985130513
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985173926
**Note**: The CI failure of `Unit / unit-tests` has nothing to do with this PR, it may be related to [13043](https://github.com/apache/pulsar/pull/13043)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#discussion_r762118069
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/NamespaceOperation.java
##########
@@ -24,7 +24,6 @@
*/
public enum NamespaceOperation {
CREATE_TOPIC,
- GET_TOPIC,
Review comment:
Technically, this is a breaking change, and I don't think we should remove the value just yet. Even though it isn't used by the project, extensions of the `AuthorizationProvider` might reference it, and removing the value from the enum would break those implementations. If we think there is no use case for the value, perhaps we could somehow mark it as deprecated and eventually remove it.
@KannarFr - is there a place where the `GET_TOPIC` value was supposed to be used?
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java
##########
@@ -183,6 +183,16 @@ default void initialize(ServiceConfiguration conf, PulsarResources pulsarResourc
CompletableFuture<Boolean> allowSinkOpsAsync(NamespaceName namespaceName, String role,
AuthenticationDataSource authenticationData);
+ /**
+ * Allow package operations with in this namespace
+ * @param namespaceName The namespace that the package operations can be executed in
+ * @param role The role to check
+ * @param authenticationData authentication data related to the role
+ * @return a boolean to determine whether authorized or not
+ */
+ CompletableFuture<Boolean> allowPackageOpsAsync(NamespaceName namespaceName, String role,
Review comment:
@yuruguo - can you explain why we need this method in the interface? It is only used internally by the `PulsarAuthorizationProvider` and `MultiRolesTokenAuthorizationProvider`, so I don' think we need to add it to the public interface.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo removed a comment on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo removed a comment on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985224325
@merlimat @massakam @codelipenghui @freeznet Can you help review this PR?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#discussion_r761864076
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java
##########
@@ -536,17 +536,32 @@ private void validatePoliciesReadOnlyAccess() {
case CLEAR_BACKLOG:
isAuthorizedFuture = allowConsumeOpsAsync(namespaceName, role, authData);
break;
+ case CREATE_TOPIC:
+ case DELETE_TOPIC:
+ case ADD_BUNDLE:
+ case GET_BUNDLE:
+ case DELETE_BUNDLE:
+ case GRANT_PERMISSION:
+ case GET_PERMISSION:
+ case REVOKE_PERMISSION:
+ return validateTenantAdminAccess(namespaceName.getTenant(), role, authData);
default:
- isAuthorizedFuture = CompletableFuture.completedFuture(false);
+ return FutureUtil.failedFuture(
+ new IllegalStateException("NamespaceOperation [" + operation.name() + "] is not supported."));
}
- CompletableFuture<Boolean> isTenantAdminFuture = validateTenantAdminAccess(namespaceName.getTenant(), role, authData);
- return isTenantAdminFuture.thenCombine(isAuthorizedFuture, (isTenantAdmin, isAuthorized) -> {
- if (log.isDebugEnabled()) {
- log.debug("Verify if role {} is allowed to {} to topic {}: isTenantAdmin={}, isAuthorized={}",
- role, operation, namespaceName, isTenantAdmin, isAuthorized);
- }
- return isTenantAdmin || isAuthorized;
- });
+
+ return validateTenantAdminAccess(namespaceName.getTenant(), role, authData)
+ .thenCompose(isSuperUserOrAdmin -> {
+ if (log.isDebugEnabled()) {
+ log.debug("Verify if role {} is allowed to {} to namespace {}: isSuperUserOrAdmin={}",
+ role, operation, namespaceName, isSuperUserOrAdmin);
+ }
+ if (isSuperUserOrAdmin) {
+ return CompletableFuture.completedFuture(true);
+ } else {
+ return isAuthorizedFuture;
Review comment:
why are we still creating `isAuthorizedFuture` ?
if we are `SuperUserOrAdmin` we can skip evaluating the logic for `isAuthorizedFuture`
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo removed a comment on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo removed a comment on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985092758
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo edited a comment on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo edited a comment on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985173926
**Note**: The CI failure of `Unit / unit-tests` has nothing to do with this PR, it may be related to [13087](https://github.com/apache/pulsar/pull/13087) @lhotari
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo removed a comment on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo removed a comment on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985182230
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985182230
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-984556672
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-984625862
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] yuruguo commented on pull request #13090: [Authorization] Optimize the logic of allowing namespace operation
Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#issuecomment-985118187
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org