Skip to content

Commit 2a5838f

Browse files
jerqijerqi
authored
[#5934] fix(auth): Avoid other catalogs' privileges are pushed down (#5935)
### What changes were proposed in this pull request? Avoid other catalogs' privileges are pushed down. For example, if a role has two catalogs. One catalog has select table, the other catalog has create table. The plugin will make the role can create and select table at the same time. ### Why are the changes needed? Fix: #5934 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Add a UT
1 parent 908e994 commit 2a5838f

File tree

4 files changed

+200
-73
lines changed

4 files changed

+200
-73
lines changed

‎core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java

+82-36
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,12 @@
1818
*/
1919
package org.apache.gravitino.authorization;
2020

21+
import com.google.common.collect.Lists;
2122
import com.google.common.collect.Sets;
2223
import java.util.Collection;
2324
import java.util.List;
2425
import java.util.Set;
26+
import java.util.function.BiConsumer;
2527
import java.util.function.Consumer;
2628
import org.apache.gravitino.Catalog;
2729
import org.apache.gravitino.Entity;
@@ -39,6 +41,7 @@
3941
import org.apache.gravitino.exceptions.NoSuchCatalogException;
4042
import org.apache.gravitino.exceptions.NoSuchMetadataObjectException;
4143
import org.apache.gravitino.exceptions.NoSuchUserException;
44+
import org.apache.gravitino.meta.RoleEntity;
4245
import org.apache.gravitino.utils.MetadataObjectUtil;
4346
import org.apache.gravitino.utils.NameIdentifierUtil;
4447

@@ -144,8 +147,8 @@ public static void checkRoleNamespace(Namespace namespace) {
144147
public static void callAuthorizationPluginForSecurableObjects(
145148
String metalake,
146149
List<SecurableObject> securableObjects,
147-
Set<String> catalogsAlreadySet,
148-
Consumer<AuthorizationPlugin> consumer) {
150+
BiConsumer<AuthorizationPlugin, String> consumer) {
151+
Set<String> catalogsAlreadySet = Sets.newHashSet();
149152
CatalogManager catalogManager = GravitinoEnv.getInstance().catalogManager();
150153
for (SecurableObject securableObject : securableObjects) {
151154
if (needApplyAuthorizationPluginAllCatalogs(securableObject)) {
@@ -245,40 +248,6 @@ public static void checkPrivilege(
245248
}
246249
}
247250

248-
private static void checkCatalogType(
249-
NameIdentifier catalogIdent, Catalog.Type type, Privilege privilege) {
250-
Catalog catalog = GravitinoEnv.getInstance().catalogDispatcher().loadCatalog(catalogIdent);
251-
if (catalog.type() != type) {
252-
throw new IllegalPrivilegeException(
253-
"Catalog %s type %s doesn't support privilege %s",
254-
catalogIdent, catalog.type(), privilege);
255-
}
256-
}
257-
258-
private static boolean needApplyAuthorizationPluginAllCatalogs(MetadataObject.Type type) {
259-
return type == MetadataObject.Type.METALAKE;
260-
}
261-
262-
private static boolean needApplyAuthorization(MetadataObject.Type type) {
263-
return type != MetadataObject.Type.ROLE && type != MetadataObject.Type.METALAKE;
264-
}
265-
266-
private static void callAuthorizationPluginImpl(
267-
Consumer<AuthorizationPlugin> consumer, Catalog catalog) {
268-
269-
if (catalog instanceof BaseCatalog) {
270-
BaseCatalog baseCatalog = (BaseCatalog) catalog;
271-
if (baseCatalog.getAuthorizationPlugin() != null) {
272-
consumer.accept(baseCatalog.getAuthorizationPlugin());
273-
}
274-
} else {
275-
throw new IllegalArgumentException(
276-
String.format(
277-
"Catalog %s is not a BaseCatalog, we don't support authorization plugin for it",
278-
catalog.type()));
279-
}
280-
}
281-
282251
public static void authorizationPluginRemovePrivileges(
283252
NameIdentifier ident, Entity.EntityType type) {
284253
// If we enable authorization, we should remove the privileges about the entity in the
@@ -313,4 +282,81 @@ public static void authorizationPluginRenamePrivileges(
313282
});
314283
}
315284
}
285+
286+
public static Role filterSecurableObjects(
287+
RoleEntity role, String metalakeName, String catalogName) {
288+
List<SecurableObject> securableObjects = role.securableObjects();
289+
List<SecurableObject> filteredSecurableObjects = Lists.newArrayList();
290+
for (SecurableObject securableObject : securableObjects) {
291+
NameIdentifier identifier = MetadataObjectUtil.toEntityIdent(metalakeName, securableObject);
292+
if (securableObject.type() == MetadataObject.Type.METALAKE) {
293+
filteredSecurableObjects.add(securableObject);
294+
} else {
295+
NameIdentifier catalogIdent = NameIdentifierUtil.getCatalogIdentifier(identifier);
296+
297+
if (catalogIdent.name().equals(catalogName)) {
298+
filteredSecurableObjects.add(securableObject);
299+
}
300+
}
301+
}
302+
303+
return RoleEntity.builder()
304+
.withId(role.id())
305+
.withName(role.name())
306+
.withAuditInfo(role.auditInfo())
307+
.withNamespace(role.namespace())
308+
.withSecurableObjects(filteredSecurableObjects)
309+
.withProperties(role.properties())
310+
.build();
311+
}
312+
313+
private static boolean needApplyAuthorizationPluginAllCatalogs(MetadataObject.Type type) {
314+
return type == MetadataObject.Type.METALAKE;
315+
}
316+
317+
private static boolean needApplyAuthorization(MetadataObject.Type type) {
318+
return type != MetadataObject.Type.ROLE && type != MetadataObject.Type.METALAKE;
319+
}
320+
321+
private static void callAuthorizationPluginImpl(
322+
BiConsumer<AuthorizationPlugin, String> consumer, Catalog catalog) {
323+
324+
if (catalog instanceof BaseCatalog) {
325+
BaseCatalog baseCatalog = (BaseCatalog) catalog;
326+
if (baseCatalog.getAuthorizationPlugin() != null) {
327+
consumer.accept(baseCatalog.getAuthorizationPlugin(), catalog.name());
328+
}
329+
} else {
330+
throw new IllegalArgumentException(
331+
String.format(
332+
"Catalog %s is not a BaseCatalog, we don't support authorization plugin for it",
333+
catalog.type()));
334+
}
335+
}
336+
337+
private static void callAuthorizationPluginImpl(
338+
Consumer<AuthorizationPlugin> consumer, Catalog catalog) {
339+
340+
if (catalog instanceof BaseCatalog) {
341+
BaseCatalog baseCatalog = (BaseCatalog) catalog;
342+
if (baseCatalog.getAuthorizationPlugin() != null) {
343+
consumer.accept(baseCatalog.getAuthorizationPlugin());
344+
}
345+
} else {
346+
throw new IllegalArgumentException(
347+
String.format(
348+
"Catalog %s is not a BaseCatalog, we don't support authorization plugin for it",
349+
catalog.type()));
350+
}
351+
}
352+
353+
private static void checkCatalogType(
354+
NameIdentifier catalogIdent, Catalog.Type type, Privilege privilege) {
355+
Catalog catalog = GravitinoEnv.getInstance().catalogDispatcher().loadCatalog(catalogIdent);
356+
if (catalog.type() != type) {
357+
throw new IllegalPrivilegeException(
358+
"Catalog %s type %s doesn't support privilege %s",
359+
catalogIdent, catalog.type(), privilege);
360+
}
361+
}
316362
}

‎core/src/main/java/org/apache/gravitino/authorization/PermissionManager.java

+51-32
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import static org.apache.gravitino.authorization.AuthorizationUtils.GROUP_DOES_NOT_EXIST_MSG;
2222
import static org.apache.gravitino.authorization.AuthorizationUtils.ROLE_DOES_NOT_EXIST_MSG;
2323
import static org.apache.gravitino.authorization.AuthorizationUtils.USER_DOES_NOT_EXIST_MSG;
24+
import static org.apache.gravitino.authorization.AuthorizationUtils.filterSecurableObjects;
2425

2526
import com.google.common.collect.Lists;
2627
import java.io.IOException;
@@ -115,17 +116,22 @@ User grantRolesToUser(String metalake, List<String> roles, String user) {
115116
.build();
116117
});
117118

118-
Set<String> catalogs = Sets.newHashSet();
119+
List<SecurableObject> securableObjects = Lists.newArrayList();
120+
119121
for (Role grantedRole : roleEntitiesToGrant) {
120-
AuthorizationUtils.callAuthorizationPluginForSecurableObjects(
121-
metalake,
122-
grantedRole.securableObjects(),
123-
catalogs,
124-
authorizationPlugin ->
125-
authorizationPlugin.onGrantedRolesToUser(
126-
Lists.newArrayList(roleEntitiesToGrant), updatedUser));
122+
securableObjects.addAll(grantedRole.securableObjects());
127123
}
128124

125+
AuthorizationUtils.callAuthorizationPluginForSecurableObjects(
126+
metalake,
127+
securableObjects,
128+
(authorizationPlugin, catalogName) ->
129+
authorizationPlugin.onGrantedRolesToUser(
130+
roleEntitiesToGrant.stream()
131+
.map(roleEntity -> filterSecurableObjects(roleEntity, metalake, catalogName))
132+
.collect(Collectors.toList()),
133+
updatedUser));
134+
129135
return updatedUser;
130136
} catch (NoSuchEntityException nse) {
131137
LOG.warn("Failed to grant, user {} does not exist in the metalake {}", user, metalake, nse);
@@ -196,17 +202,22 @@ Group grantRolesToGroup(String metalake, List<String> roles, String group) {
196202
.build();
197203
});
198204

199-
Set<String> catalogs = Sets.newHashSet();
205+
List<SecurableObject> securableObjects = Lists.newArrayList();
206+
200207
for (Role grantedRole : roleEntitiesToGrant) {
201-
AuthorizationUtils.callAuthorizationPluginForSecurableObjects(
202-
metalake,
203-
grantedRole.securableObjects(),
204-
catalogs,
205-
authorizationPlugin ->
206-
authorizationPlugin.onGrantedRolesToGroup(
207-
Lists.newArrayList(roleEntitiesToGrant), updatedGroup));
208+
securableObjects.addAll(grantedRole.securableObjects());
208209
}
209210

211+
AuthorizationUtils.callAuthorizationPluginForSecurableObjects(
212+
metalake,
213+
securableObjects,
214+
(authorizationPlugin, catalogName) ->
215+
authorizationPlugin.onGrantedRolesToGroup(
216+
roleEntitiesToGrant.stream()
217+
.map(roleEntity -> filterSecurableObjects(roleEntity, metalake, catalogName))
218+
.collect(Collectors.toList()),
219+
updatedGroup));
220+
210221
return updatedGroup;
211222
} catch (NoSuchEntityException nse) {
212223
LOG.warn("Failed to grant, group {} does not exist in the metalake {}", group, metalake, nse);
@@ -276,17 +287,21 @@ Group revokeRolesFromGroup(String metalake, List<String> roles, String group) {
276287
.build();
277288
});
278289

279-
Set<String> catalogs = Sets.newHashSet();
290+
List<SecurableObject> securableObjects = Lists.newArrayList();
280291
for (Role grantedRole : roleEntitiesToRevoke) {
281-
AuthorizationUtils.callAuthorizationPluginForSecurableObjects(
282-
metalake,
283-
grantedRole.securableObjects(),
284-
catalogs,
285-
authorizationPlugin ->
286-
authorizationPlugin.onRevokedRolesFromGroup(
287-
Lists.newArrayList(roleEntitiesToRevoke), updatedGroup));
292+
securableObjects.addAll(grantedRole.securableObjects());
288293
}
289294

295+
AuthorizationUtils.callAuthorizationPluginForSecurableObjects(
296+
metalake,
297+
securableObjects,
298+
(authorizationPlugin, catalogName) ->
299+
authorizationPlugin.onRevokedRolesFromGroup(
300+
roleEntitiesToRevoke.stream()
301+
.map(roleEntity -> filterSecurableObjects(roleEntity, metalake, catalogName))
302+
.collect(Collectors.toList()),
303+
updatedGroup));
304+
290305
return updatedGroup;
291306

292307
} catch (NoSuchEntityException nse) {
@@ -358,17 +373,21 @@ User revokeRolesFromUser(String metalake, List<String> roles, String user) {
358373
.build();
359374
});
360375

361-
Set<String> catalogs = Sets.newHashSet();
376+
List<SecurableObject> securableObjects = Lists.newArrayList();
362377
for (Role grantedRole : roleEntitiesToRevoke) {
363-
AuthorizationUtils.callAuthorizationPluginForSecurableObjects(
364-
metalake,
365-
grantedRole.securableObjects(),
366-
catalogs,
367-
authorizationPlugin ->
368-
authorizationPlugin.onRevokedRolesFromUser(
369-
Lists.newArrayList(roleEntitiesToRevoke), updatedUser));
378+
securableObjects.addAll(grantedRole.securableObjects());
370379
}
371380

381+
AuthorizationUtils.callAuthorizationPluginForSecurableObjects(
382+
metalake,
383+
securableObjects,
384+
(authorizationPlugin, catalogName) ->
385+
authorizationPlugin.onRevokedRolesFromUser(
386+
roleEntitiesToRevoke.stream()
387+
.map(roleEntity -> filterSecurableObjects(roleEntity, metalake, catalogName))
388+
.collect(Collectors.toList()),
389+
updatedUser));
390+
372391
return updatedUser;
373392
} catch (NoSuchEntityException nse) {
374393
LOG.warn("Failed to revoke, user {} does not exist in the metalake {}", user, metalake, nse);

‎core/src/main/java/org/apache/gravitino/authorization/RoleManager.java

+6-5
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121

2222
import static org.apache.gravitino.metalake.MetalakeManager.checkMetalake;
2323

24-
import com.google.common.collect.Sets;
2524
import java.io.IOException;
2625
import java.time.Instant;
2726
import java.util.List;
@@ -87,8 +86,9 @@ RoleEntity createRole(
8786
AuthorizationUtils.callAuthorizationPluginForSecurableObjects(
8887
metalake,
8988
roleEntity.securableObjects(),
90-
Sets.newHashSet(),
91-
authorizationPlugin -> authorizationPlugin.onRoleCreated(roleEntity));
89+
(authorizationPlugin, catalogName) ->
90+
authorizationPlugin.onRoleCreated(
91+
AuthorizationUtils.filterSecurableObjects(roleEntity, metalake, catalogName)));
9292

9393
return roleEntity;
9494
} catch (EntityAlreadyExistsException e) {
@@ -122,8 +122,9 @@ boolean deleteRole(String metalake, String role) {
122122
AuthorizationUtils.callAuthorizationPluginForSecurableObjects(
123123
metalake,
124124
roleEntity.securableObjects(),
125-
Sets.newHashSet(),
126-
authorizationPlugin -> authorizationPlugin.onRoleDeleted(roleEntity));
125+
(authorizationPlugin, catalogName) ->
126+
authorizationPlugin.onRoleDeleted(
127+
AuthorizationUtils.filterSecurableObjects(roleEntity, metalake, catalogName)));
127128
} catch (NoSuchEntityException nse) {
128129
// ignore, because the role may have been deleted.
129130
}

‎core/src/test/java/org/apache/gravitino/authorization/TestAuthorizationUtils.java

+61
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,14 @@
1818
*/
1919
package org.apache.gravitino.authorization;
2020

21+
import com.google.common.collect.Lists;
22+
import java.util.List;
2123
import org.apache.gravitino.NameIdentifier;
2224
import org.apache.gravitino.Namespace;
2325
import org.apache.gravitino.exceptions.IllegalNameIdentifierException;
2426
import org.apache.gravitino.exceptions.IllegalNamespaceException;
27+
import org.apache.gravitino.meta.AuditInfo;
28+
import org.apache.gravitino.meta.RoleEntity;
2529
import org.junit.jupiter.api.Assertions;
2630
import org.junit.jupiter.api.Test;
2731

@@ -149,4 +153,61 @@ void testCheckNamespace() {
149153
IllegalNamespaceException.class,
150154
() -> AuthorizationUtils.checkRoleNamespace(Namespace.of("a", "b", "c", "d")));
151155
}
156+
157+
@Test
158+
void testFilteredSecurableObjects() {
159+
160+
List<SecurableObject> securableObjects = Lists.newArrayList();
161+
162+
SecurableObject metalakeObject =
163+
SecurableObjects.ofMetalake("metalake", Lists.newArrayList(Privileges.SelectTable.allow()));
164+
securableObjects.add(metalakeObject);
165+
166+
SecurableObject catalog1Object =
167+
SecurableObjects.ofCatalog("catalog1", Lists.newArrayList(Privileges.SelectTable.allow()));
168+
securableObjects.add(catalog1Object);
169+
170+
SecurableObject catalog2Object =
171+
SecurableObjects.ofCatalog("catalog2", Lists.newArrayList(Privileges.SelectTable.allow()));
172+
securableObjects.add(catalog2Object);
173+
174+
SecurableObject schema1Object =
175+
SecurableObjects.ofSchema(
176+
catalog1Object, "schema1", Lists.newArrayList(Privileges.SelectTable.allow()));
177+
SecurableObject table1Object =
178+
SecurableObjects.ofTable(
179+
schema1Object, "table1", Lists.newArrayList(Privileges.SelectTable.allow()));
180+
securableObjects.add(table1Object);
181+
securableObjects.add(schema1Object);
182+
183+
SecurableObject schema2Object =
184+
SecurableObjects.ofSchema(
185+
catalog2Object, "schema2", Lists.newArrayList(Privileges.SelectTable.allow()));
186+
SecurableObject table2Object =
187+
SecurableObjects.ofTable(
188+
schema2Object, "table2", Lists.newArrayList(Privileges.SelectTable.allow()));
189+
securableObjects.add(table2Object);
190+
securableObjects.add(schema2Object);
191+
192+
RoleEntity role =
193+
RoleEntity.builder()
194+
.withId(1L)
195+
.withName("role")
196+
.withSecurableObjects(securableObjects)
197+
.withAuditInfo(AuditInfo.EMPTY)
198+
.build();
199+
Role filteredRole = AuthorizationUtils.filterSecurableObjects(role, "metalake", "catalog1");
200+
Assertions.assertEquals(4, filteredRole.securableObjects().size());
201+
Assertions.assertTrue(filteredRole.securableObjects().contains(metalakeObject));
202+
Assertions.assertTrue(filteredRole.securableObjects().contains(catalog1Object));
203+
Assertions.assertTrue(filteredRole.securableObjects().contains(schema1Object));
204+
Assertions.assertTrue(filteredRole.securableObjects().contains(table1Object));
205+
206+
filteredRole = AuthorizationUtils.filterSecurableObjects(role, "metalake", "catalog2");
207+
Assertions.assertEquals(4, filteredRole.securableObjects().size());
208+
Assertions.assertTrue(filteredRole.securableObjects().contains(metalakeObject));
209+
Assertions.assertTrue(filteredRole.securableObjects().contains(catalog2Object));
210+
Assertions.assertTrue(filteredRole.securableObjects().contains(schema2Object));
211+
Assertions.assertTrue(filteredRole.securableObjects().contains(table2Object));
212+
}
152213
}

0 commit comments

Comments
 (0)