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 2022/05/13 07:48:00 UTC
[GitHub] [pulsar] lhotari commented on a diff in pull request #15576: [PIP][Authorization] Make Implicit Subscription Permission Configurable
lhotari commented on code in PR #15576:
URL: https://github.com/apache/pulsar/pull/15576#discussion_r872088658
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v1/Namespaces.java:
##########
@@ -314,6 +314,49 @@ public void revokePermissionOnSubscription(@PathParam("property") String propert
internalRevokePermissionsOnSubscription(subscription, role);
}
+ @PUT
+ @Path("/{property}/{cluster}/{namespace}/implicitPermissionOnSubscription")
+ @ApiOperation(hidden = true, value = "Allow a consumer's role to have implicit permission to consume from a"
+ + " subscription.")
+ @ApiResponses(value = {@ApiResponse(code = 403, message = "Don't have admin permission"),
+ @ApiResponse(code = 404, message = "Property or cluster or namespace doesn't exist"),
+ @ApiResponse(code = 409, message = "Concurrent modification"),
+ @ApiResponse(code = 501, message = "Authorization is not enabled")})
+ public void grantImplicitPermissionOnSubscription(
+ @PathParam("property") String property, @PathParam("cluster") String cluster,
+ @PathParam("namespace") String namespace) {
+ validateNamespaceName(property, cluster, namespace);
+ internalSetImplicitPermissionOnSubscription(true);
+ }
+
+ @DELETE
+ @Path("/{property}/{cluster}/{namespace}/implicitPermissionOnSubscription")
+ @ApiOperation(hidden = true, value = "Require a consumer's role to have explicit permission to consume from a"
+ + " subscription.")
+ @ApiResponses(value = {@ApiResponse(code = 403, message = "Don't have admin permission"),
+ @ApiResponse(code = 404, message = "Property or cluster or namespace doesn't exist"),
+ @ApiResponse(code = 409, message = "Concurrent modification"),
+ @ApiResponse(code = 501, message = "Authorization is not enabled")})
+ public void revokeImplicitPermissionOnSubscription(
+ @PathParam("property") String property, @PathParam("cluster") String cluster,
+ @PathParam("namespace") String namespace) {
+ validateNamespaceName(property, cluster, namespace);
+ internalSetImplicitPermissionOnSubscription(false);
+ }
+
+ @GET
+ @Path("/{property}/{cluster}/{namespace}/implicitPermissionOnSubscription")
+ @ApiOperation(value = "Get permission on subscription required for namespace.")
+ @ApiResponses(value = {@ApiResponse(code = 403, message = "Don't have admin permission"),
+ @ApiResponse(code = 404, message = "Property or cluster or namespace doesn't exist"),
+ @ApiResponse(code = 409, message = "Namespace is not empty")})
+ public boolean getImplicitPermissionOnSubscription(@PathParam("property") String property,
+ @PathParam("cluster") String cluster,
+ @PathParam("namespace") String namespace) {
+ validateNamespaceName(property, cluster, namespace);
+ return getImplicitPermissionOnSubscription();
+ }
+
Review Comment:
Does this have to be added to the v1 API at all?
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/Namespaces.java:
##########
@@ -267,6 +267,48 @@ public void revokePermissionOnSubscription(@PathParam("property") String propert
internalRevokePermissionsOnSubscription(subscription, role);
}
+ @PUT
Review Comment:
```suggestion
@POST
```
Other API methods to grant permissions are using POST.
##########
pulsar-common/src/main/java/org/apache/pulsar/client/admin/internal/data/AuthPoliciesImpl.java:
##########
@@ -42,6 +42,10 @@ public final class AuthPoliciesImpl implements AuthPolicies {
@JsonProperty("subscription_auth_roles")
private Map<String, Set<String>> subscriptionAuthentication = new TreeMap<>();
+ // Default value is set in the builder
+ @JsonProperty(value = "implicit_subscription_auth")
+ private boolean implicitSubscriptionAuth;
Review Comment:
Good points Enrico. Yes this should be addressed.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -698,6 +698,24 @@ protected void internalGrantPermissionOnNamespace(String role, Set<AuthAction> a
}
}
+ protected boolean getImplicitPermissionOnSubscription() {
+ validateNamespaceOperation(namespaceName, NamespaceOperation.GET_PERMISSION);
+ Policies policies = getNamespacePolicies(namespaceName);
+ return policies.auth_policies.isImplicitSubscriptionAuth();
+ }
+
+ protected void internalSetImplicitPermissionOnSubscription(boolean isImplicitPermissionOnSubscription) {
+ if (isImplicitPermissionOnSubscription) {
+ validateNamespaceOperation(namespaceName, NamespaceOperation.GRANT_PERMISSION);
+ } else {
+ validateNamespaceOperation(namespaceName, NamespaceOperation.REVOKE_PERMISSION);
+ }
+ validatePoliciesReadOnlyAccess();
+ updatePolicies(namespaceName, policies -> {
+ policies.auth_policies.setImplicitSubscriptionAuth(isImplicitPermissionOnSubscription);
+ return policies;
+ });
+ }
Review Comment:
It might be worth considering PIP-149/#14365 here and use the async style to implement the API.
To clarify it: the server side implementation should by async, but the client API can contain both sync and async API methods.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/Namespaces.java:
##########
@@ -267,6 +267,48 @@ public void revokePermissionOnSubscription(@PathParam("property") String propert
internalRevokePermissionsOnSubscription(subscription, role);
}
+ @PUT
+ @Path("/{property}/{namespace}/implicitPermissionOnSubscription")
+ @ApiOperation(hidden = true, value = "Allow a consumer's role to have implicit permission to consume from a"
+ + " subscription.")
+ @ApiResponses(value = {@ApiResponse(code = 403, message = "Don't have admin permission"),
+ @ApiResponse(code = 404, message = "Property or cluster or namespace doesn't exist"),
+ @ApiResponse(code = 409, message = "Concurrent modification"),
+ @ApiResponse(code = 501, message = "Authorization is not enabled")})
+ public void grantImplicitPermissionOnSubscription(
+ @PathParam("property") String property,
+ @PathParam("namespace") String namespace) {
+ validateNamespaceName(property, namespace);
+ internalSetImplicitPermissionOnSubscription(true);
Review Comment:
It might be worth considering PIP-149/#14365 here and use the async style to implement the API.
To clarify it: the server side implementation should by async, but the client API can contain both sync and async API methods.
--
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