xunliu commented on code in PR #7086: URL: https://github.com/apache/gravitino/pull/7086#discussion_r2062802726
########## server-common/src/main/resources/jcasbin_model.conf: ########## @@ -0,0 +1,50 @@ +; Licensed to the Apache Software Foundation (ASF) under one +; or more contributor license agreements. See the NOTICE file +; distributed with this work for additional information +; regarding copyright ownership. The ASF licenses this file +; to you under the Apache License, Version 2.0 (the +; "License"); you may not use this file except in compliance +; with the License. You may obtain a copy of the License at +; +; http://www.apache.org/licenses/LICENSE-2.0 +; +; Unless required by applicable law or agreed to in writing, +; software distributed under the License is distributed on an +; "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +; KIND, either express or implied. See the License for the +; specific language governing permissions and limitations +; under the License. + +; The model file of jcasbin defines a permission model used to describe the permissions +; of users, groups, and roles for a single metadata type. + +[request_definition] +r = sub,metalakeId,metadataType,metadataId, act Review Comment: Can we add a space in every word? ########## server-common/src/main/resources/jcasbin_model.conf: ########## @@ -0,0 +1,50 @@ +; Licensed to the Apache Software Foundation (ASF) under one +; or more contributor license agreements. See the NOTICE file +; distributed with this work for additional information +; regarding copyright ownership. The ASF licenses this file +; to you under the Apache License, Version 2.0 (the +; "License"); you may not use this file except in compliance +; with the License. You may obtain a copy of the License at +; +; http://www.apache.org/licenses/LICENSE-2.0 +; +; Unless required by applicable law or agreed to in writing, +; software distributed under the License is distributed on an +; "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +; KIND, either express or implied. See the License for the +; specific language governing permissions and limitations +; under the License. + +; The model file of jcasbin defines a permission model used to describe the permissions +; of users, groups, and roles for a single metadata type. + +[request_definition] +r = sub,metalakeId,metadataType,metadataId, act + +; "r" represents the parameters passed when making a request to jCasbin. +; "sub" represents a role, user, or groupCode. +; "metalakeId" represents the metalake to which a role, user, or group belongs. +; "metadataType" represents the type of metadata. +; "metadataId" represents the id of metadata +; "act" represents the privilege that needs to be authorized or whether it is an OWNER. + +[policy_definition] +p = sub,metalakeId,metadataType,metadataId, act, eft + +; "p" represents a permission policy. +; "eft" stands for "effect" which can be either "allow" or "deny". + +[role_definition] +g = _, _ + +; "g" represents the membership or ownership relationship of users, groups, or roles. Review Comment: Can we move these comments before the `[role_definition]`? ########## server-common/src/main/resources/jcasbin_model.conf: ########## @@ -0,0 +1,50 @@ +; Licensed to the Apache Software Foundation (ASF) under one +; or more contributor license agreements. See the NOTICE file +; distributed with this work for additional information +; regarding copyright ownership. The ASF licenses this file +; to you under the Apache License, Version 2.0 (the +; "License"); you may not use this file except in compliance +; with the License. You may obtain a copy of the License at +; +; http://www.apache.org/licenses/LICENSE-2.0 +; +; Unless required by applicable law or agreed to in writing, +; software distributed under the License is distributed on an +; "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +; KIND, either express or implied. See the License for the +; specific language governing permissions and limitations +; under the License. + +; The model file of jcasbin defines a permission model used to describe the permissions +; of users, groups, and roles for a single metadata type. + +[request_definition] +r = sub,metalakeId,metadataType,metadataId, act + +; "r" represents the parameters passed when making a request to jCasbin. +; "sub" represents a role, user, or groupCode. +; "metalakeId" represents the metalake to which a role, user, or group belongs. +; "metadataType" represents the type of metadata. +; "metadataId" represents the id of metadata +; "act" represents the privilege that needs to be authorized or whether it is an OWNER. + +[policy_definition] +p = sub,metalakeId,metadataType,metadataId, act, eft + +; "p" represents a permission policy. +; "eft" stands for "effect" which can be either "allow" or "deny". + +[role_definition] +g = _, _ + +; "g" represents the membership or ownership relationship of users, groups, or roles. + +[policy_effect] +e = some(where (p.eft == allow)) && !some(where (p.eft == deny)) + +; "e" represents the effect of the "eft" Review Comment: I think we can add detailed comments here, maybe just like: 1. `some(where (p.eft == allow))`: What mean is this code? 2. `!some(where (p.eft == deny))`: What mean is this code? and other places. ########## server-common/src/test/java/org/apache/gravitino/server/authorization/TestJcasbinModel.java: ########## @@ -0,0 +1,284 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.gravitino.server.authorization; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.nio.charset.StandardCharsets; +import org.apache.commons.io.IOUtils; +import org.casbin.jcasbin.main.Enforcer; +import org.casbin.jcasbin.model.Model; +import org.casbin.jcasbin.persist.file_adapter.FileAdapter; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +/** Test for jcasbin model. */ +public class TestJcasbinModel { + + private static Enforcer enforcer; + + /** mock jcasbin policy */ + private static final String JCASBIN_POLICY = + "p,role1,metalake1,METALAKE,metalake1,OWNER,allow\n" + + "\n" + + "p,role2,metalake1,CATALOG,catalog1,OWNER,allow\n" + + "\n" + + "p,role3,metalake1,SCHEMA,schema1,OWNER,allow\n" + + "\n" + + "p,role4,metalake1,TABLE,table1,OWNER,allow\n" + + "\n" + + "p,role5,metalake1,METALAKE,metalake1,SELECT_TABLE,allow\n" + + "p,role5,metalake1,METALAKE,metalake1,USE_CATALOG,allow\n" + + "p,role5,metalake1,TABLE,table1,SELECT_TABLE,allow\n" + + "\n" + + "g,user1,role5\n" + + "\n" + + "g,group1,role5\n" + + "\n" + + "g,user2,group1\n" + + "\n" + + "g,user3,role1\n" + + "g,user3,group1"; + + @BeforeAll + public static void init() throws IOException { + try (InputStream modelStream = + TestJcasbinModel.class.getResourceAsStream("/jcasbin_model.conf"); + InputStream policyInputStream = + new ByteArrayInputStream(JCASBIN_POLICY.getBytes(StandardCharsets.UTF_8))) { + Assertions.assertNotNull(modelStream); + String string = IOUtils.toString(modelStream, StandardCharsets.UTF_8); + Model model = new Model(); + model.loadModelFromText(string); + FileAdapter fileAdapter = new FileAdapter(policyInputStream); + enforcer = new Enforcer(model, fileAdapter); + } + } + + @Test + public void testMetalakeOwner() { Review Comment: Please add comments in every test cases, to introduce what permission to check with jCasbin ########## server-common/src/test/java/org/apache/gravitino/server/authorization/TestJcasbinModel.java: ########## @@ -0,0 +1,284 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.gravitino.server.authorization; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.nio.charset.StandardCharsets; +import org.apache.commons.io.IOUtils; +import org.casbin.jcasbin.main.Enforcer; +import org.casbin.jcasbin.model.Model; +import org.casbin.jcasbin.persist.file_adapter.FileAdapter; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +/** Test for jcasbin model. */ +public class TestJcasbinModel { + + private static Enforcer enforcer; + + /** mock jcasbin policy */ + private static final String JCASBIN_POLICY = + "p,role1,metalake1,METALAKE,metalake1,OWNER,allow\n" + + "\n" + + "p,role2,metalake1,CATALOG,catalog1,OWNER,allow\n" + + "\n" + + "p,role3,metalake1,SCHEMA,schema1,OWNER,allow\n" + + "\n" + + "p,role4,metalake1,TABLE,table1,OWNER,allow\n" + + "\n" + + "p,role5,metalake1,METALAKE,metalake1,SELECT_TABLE,allow\n" + + "p,role5,metalake1,METALAKE,metalake1,USE_CATALOG,allow\n" + + "p,role5,metalake1,TABLE,table1,SELECT_TABLE,allow\n" + + "\n" + + "g,user1,role5\n" + + "\n" + + "g,group1,role5\n" + + "\n" + + "g,user2,group1\n" + + "\n" + + "g,user3,role1\n" + + "g,user3,group1"; Review Comment: Can we move this into a `resource/jcasbin-data-xxx` file? I thought it will be clearly ########## server-common/src/test/java/org/apache/gravitino/server/authorization/TestJcasbinModel.java: ########## @@ -0,0 +1,284 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.gravitino.server.authorization; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.nio.charset.StandardCharsets; +import org.apache.commons.io.IOUtils; +import org.casbin.jcasbin.main.Enforcer; +import org.casbin.jcasbin.model.Model; +import org.casbin.jcasbin.persist.file_adapter.FileAdapter; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +/** Test for jcasbin model. */ +public class TestJcasbinModel { + + private static Enforcer enforcer; + + /** mock jcasbin policy */ + private static final String JCASBIN_POLICY = + "p,role1,metalake1,METALAKE,metalake1,OWNER,allow\n" + + "\n" + + "p,role2,metalake1,CATALOG,catalog1,OWNER,allow\n" + + "\n" + + "p,role3,metalake1,SCHEMA,schema1,OWNER,allow\n" + + "\n" + + "p,role4,metalake1,TABLE,table1,OWNER,allow\n" + + "\n" + + "p,role5,metalake1,METALAKE,metalake1,SELECT_TABLE,allow\n" + + "p,role5,metalake1,METALAKE,metalake1,USE_CATALOG,allow\n" + + "p,role5,metalake1,TABLE,table1,SELECT_TABLE,allow\n" + + "\n" + + "g,user1,role5\n" + + "\n" + + "g,group1,role5\n" + + "\n" + + "g,user2,group1\n" + + "\n" + + "g,user3,role1\n" + + "g,user3,group1"; + + @BeforeAll + public static void init() throws IOException { + try (InputStream modelStream = + TestJcasbinModel.class.getResourceAsStream("/jcasbin_model.conf"); + InputStream policyInputStream = + new ByteArrayInputStream(JCASBIN_POLICY.getBytes(StandardCharsets.UTF_8))) { + Assertions.assertNotNull(modelStream); + String string = IOUtils.toString(modelStream, StandardCharsets.UTF_8); + Model model = new Model(); + model.loadModelFromText(string); + FileAdapter fileAdapter = new FileAdapter(policyInputStream); + enforcer = new Enforcer(model, fileAdapter); + } + } + + @Test + public void testMetalakeOwner() { + Assertions.assertTrue(enforcer.enforce("role1", "metalake1", "METALAKE", "metalake1", "OWNER")); + Assertions.assertTrue( + enforcer.enforce("role1", "metalake1", "METALAKE", "metalake1", "USE_CATALOG")); + Assertions.assertFalse( + enforcer.enforce("role1", "metalake2", "METALAKE", "metalake1", "OWNER")); + Assertions.assertFalse( + enforcer.enforce("role1", "metalake2", "METALAKE", "metalake1", "USE_CATALOG")); + Assertions.assertFalse( + enforcer.enforce("role1", "metalake1", "METALAKE", "metalake2", "OWNER")); + Assertions.assertFalse( + enforcer.enforce("role1", "metalake1", "METALAKE", "metalake2", "USE_CATALOG")); Review Comment: I think maybe we need use `Privilege.Name.USE_CATALOG.toString()` instead "USE_CATALOG"?? and use `MetadataObject.Type. METALAKE.toString()`?? or other `toXXX()` function. I thought it will be clear? ########## server-common/src/main/resources/jcasbin_model.conf: ########## @@ -0,0 +1,50 @@ +; Licensed to the Apache Software Foundation (ASF) under one +; or more contributor license agreements. See the NOTICE file +; distributed with this work for additional information +; regarding copyright ownership. The ASF licenses this file +; to you under the Apache License, Version 2.0 (the +; "License"); you may not use this file except in compliance +; with the License. You may obtain a copy of the License at +; +; http://www.apache.org/licenses/LICENSE-2.0 +; +; Unless required by applicable law or agreed to in writing, +; software distributed under the License is distributed on an +; "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +; KIND, either express or implied. See the License for the +; specific language governing permissions and limitations +; under the License. + +; The model file of jcasbin defines a permission model used to describe the permissions +; of users, groups, and roles for a single metadata type. + +[request_definition] +r = sub,metalakeId,metadataType,metadataId, act + +; "r" represents the parameters passed when making a request to jCasbin. +; "sub" represents a role, user, or groupCode. +; "metalakeId" represents the metalake to which a role, user, or group belongs. +; "metadataType" represents the type of metadata. +; "metadataId" represents the id of metadata +; "act" represents the privilege that needs to be authorized or whether it is an OWNER. + +[policy_definition] +p = sub,metalakeId,metadataType,metadataId, act, eft Review Comment: Can we add a space in every word? -- 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]
