laserninja commented on code in PR #12867:
URL: https://github.com/apache/gravitino/pull/12867#discussion_r4214604435
##########
core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java:
##########
@@ -473,6 +501,65 @@ public static Role filterSecurableObjects(
.build();
}
+ // Compare the connector-visible states before choosing the callback and its
payload. A
+ // Semantic Model-only change must neither contact a connector nor leak into
its role snapshot.
+ static void notifyRolePrivilegesUpdated(
+ String metalake,
+ RoleEntity role,
+ @Nullable SecurableObject before,
+ @Nullable SecurableObject after) {
+ SecurableObject previous = connectorSecurableObject(before);
+ SecurableObject next = connectorSecurableObject(after);
+ if (previous == null && next == null) {
+ return;
+ }
+ RoleChange change;
+ if (previous == null) {
+ change = RoleChange.addSecurableObject(role.name(), next);
+ } else if (next == null) {
+ change = RoleChange.removeSecurableObject(role.name(), previous);
+ } else {
+ if
(Sets.newHashSet(previous.privileges()).equals(Sets.newHashSet(next.privileges())))
{
+ return;
+ }
+ change = RoleChange.updateSecurableObject(role.name(), previous, next);
+ }
+ // Include the previous privileges when selecting catalogs, so removing
the last inherited
+ // connector privilege still reaches the plugin that received it.
+ SecurableObject target = next == null ? previous : next;
+ Set<Privilege> affectedPrivileges = Sets.newHashSet(target.privileges());
+ if (previous != null) {
+ affectedPrivileges.addAll(previous.privileges());
+ }
+ callAuthorizationPluginForSecurableObjects(
+ metalake,
+ List.of(
+ SecurableObjects.parse(
+ target.fullName(), target.type(),
Lists.newArrayList(affectedPrivileges))),
+ (plugin, catalog) ->
+ plugin.onRoleUpdated(filterSecurableObjects(role, metalake,
catalog), change));
+ }
+
+ @Nullable
+ private static SecurableObject connectorSecurableObject(@Nullable
SecurableObject object) {
+ if (object == null) {
+ return null;
+ }
+ List<Privilege> privileges = connectorPrivileges(object);
+ return privileges.isEmpty()
+ ? null
+ : SecurableObjects.parse(object.fullName(), object.type(), privileges);
+ }
+
+ private static List<Privilege> connectorPrivileges(SecurableObject object) {
Review Comment:
The existing `SEMANTIC_MODEL_PRIVILEGES` constant already contains the three
semantic-model privileges, and `connectorPrivileges` filters against that set.
It is also reused for catalog-type validation. We can clarify the
connector-filtering intent in the naming or a comment without duplicating the
privilege list.
##########
api/src/main/java/org/apache/gravitino/authorization/Privilege.java:
##########
@@ -171,7 +171,13 @@ enum Name {
* credential secrets in the {@code getSecrets} result. Alone it does not
authorize {@code
* getSecrets}. Does not authorize {@code getCredentials}.
*/
- INCLUDE_CREDENTIAL_SECRETS(0L, 1L << 38);
+ INCLUDE_CREDENTIAL_SECRETS(0L, 1L << 38),
+ /** The privilege to create a semantic model. */
+ CREATE_SEMANTIC_MODEL(0L, 1L << 39),
+ /** The privilege to discover a semantic model and load its definition. */
+ SELECT_SEMANTIC_MODEL(0L, 1L << 40),
Review Comment:
Agreed, `USE_SEMANTIC_MODEL` better describes the intended capability and is
consistent with `USE_MODEL`. `SELECT_SEMANTIC_MODEL` follows the original
design, but its scope is discovering models and loading their definitions; it
does not grant access to the underlying source data. I support a coordinated
rename in a follow-up, keeping the existing privilege bit and updating the API,
authorization expressions, OpenAPI, docs, and tests together. We should also
check compatibility for clients using the current name.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
