[Authorization] Optimize the logic of allowing namespace operation

[yuruguo]

Motivation

Currently, pulsar supports 13 NamespaceOperation, so there should not be more than 13 in method PulsarAuthorizationProvider#allowNamespaceOperationAsync. Otherwise, the undefined NamespaceOperation can also be passed by super-user or role with admin privileges.

pulsar/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java

Lines 525 to 550 in 7adfffd

	public CompletableFuture<Boolean> allowNamespaceOperationAsync(NamespaceName namespaceName,
	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;
	});
	}

The purpose of this PR is to optimize the logic of PulsarAuthorizationProvider#allowNamespaceOperationAsync so that it only covers the NamespaceOperation supported by pulsar.

Documentation

• no-need-doc

[nicoloboschi]

LGTM

[eolivelli]

I left one last comment, then I am +1

@michaeljmarshall can you please take a look ?

[michaeljmarshall]

Currently, pulsar supports 12 NamespaceOperation, so there should not be more than 12 in method PulsarAuthorizationProvider#allowNamespaceOperationAsync
Otherwise, the undefined NamespaceOperation can also be passed by super-user or role with admin privileges.

@yuruguo - can you explain the risk more? The NamespaceOperation is a serverside abstraction. I think might be missing something.

[michaeljmarshall]

@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.

[yuruguo]

@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

pulsar/pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/TenantOperation.java

Lines 22 to 29 in 50ab411

	/**
	* Tenant authorization operations.
	*/
	public enum TenantOperation {
	CREATE_NAMESPACE,
	DELETE_NAMESPACE,
	LIST_NAMESPACES,
	}

[eolivelli]

+1

[KannarFr]

@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.

[michaeljmarshall]

@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.

[michaeljmarshall]

LGTM

[BewareMyPower]

Add the release/2.8.4 label since it's relied by #15501.