[GitHub] [hadoop] xiaoyuyao commented on a change in pull request #1829: HDFS-14743. Enhance INodeAttributeProvider/ AccessControlEnforcer Interface in HDFS to support Authorization of mkdir, rm, rmdi
xiaoyuyao commented on a change in pull request #1829: HDFS-14743. Enhance
INodeAttributeProvider/ AccessControlEnforcer Interface in HDFS to support
Authorization of mkdir, rm, rmdir, copy, move etc...
URL: https://github.com/apache/hadoop/pull/1829#discussion_r391187769
##
File path:
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAuthorizationContext.java
##
@@ -0,0 +1,167 @@
+/**
+ * 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.hadoop.hdfs.server.namenode;
+
+import org.apache.hadoop.ipc.CallerContext;
+import org.apache.hadoop.security.UserGroupInformation;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.io.IOException;
+
+import static junit.framework.TestCase.assertEquals;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+public class TestAuthorizationContext {
+
+ private String fsOwner = "hdfs";
+ private String superGroup = "hdfs";
+ private UserGroupInformation ugi = UserGroupInformation.
+ createUserForTesting(fsOwner, new String[] {superGroup});
+
+ private INodeAttributes[] emptyINodeAttributes = new INodeAttributes[] {};
+ private INodesInPath iip = mock(INodesInPath.class);
+ private int snapshotId = 0;
+ private INode[] inodes = new INode[] {};
+ private byte[][] components = new byte[][] {};
+ private String path = "";
+ private int ancestorIndex = inodes.length - 2;
+
+ @Before
+ public void setUp() throws IOException {
+when(iip.getPathSnapshotId()).thenReturn(snapshotId);
+when(iip.getINodesArray()).thenReturn(inodes);
+when(iip.getPathComponents()).thenReturn(components);
+when(iip.getPath()).thenReturn(path);
+ }
+
+ @Test
+ public void testBuilder() {
+String opType = "test";
+CallerContext.setCurrent(new CallerContext.Builder(
+"TestAuthorizationContext").build());
+
+INodeAttributeProvider.AuthorizationContext.Builder builder =
+new INodeAttributeProvider.AuthorizationContext.Builder();
+builder.fsOwner(fsOwner).
+supergroup(superGroup).
+callerUgi(ugi).
+inodeAttrs(emptyINodeAttributes).
+inodes(inodes).
+pathByNameArr(components).
+snapshotId(snapshotId).
+path(path).
+ancestorIndex(ancestorIndex).
+doCheckOwner(true).
+ancestorAccess(null).
+parentAccess(null).
+access(null).
+subAccess(null).
+ignoreEmptyDir(true).
+operationName(opType).
+callerContext(CallerContext.getCurrent());
+
+INodeAttributeProvider.AuthorizationContext authzContext = builder.build();
+assertEquals(authzContext.getFsOwner(), fsOwner);
+assertEquals(authzContext.getSupergroup(), superGroup);
+assertEquals(authzContext.getCallerUgi(), ugi);
+assertEquals(authzContext.getInodeAttrs(), emptyINodeAttributes);
+assertEquals(authzContext.getInodes(), inodes);
+assertEquals(authzContext.getPathByNameArr(), components);
+assertEquals(authzContext.getSnapshotId(), snapshotId);
+assertEquals(authzContext.getPath(), path);
+assertEquals(authzContext.getAncestorIndex(), ancestorIndex);
+assertEquals(authzContext.getOperationName(), opType);
+assertEquals(authzContext.getCallerContext(), CallerContext.getCurrent());
+ }
+
+ @Test
+ public void testLegacyAPI() throws IOException {
+INodeAttributeProvider.AccessControlEnforcer
+mockEnforcer =
mock(INodeAttributeProvider.AccessControlEnforcer.class);
+INodeAttributeProvider mockINodeAttributeProvider =
+mock(INodeAttributeProvider.class);
+when(mockINodeAttributeProvider.getExternalAccessControlEnforcer(any())).
+thenReturn(mockEnforcer);
+
+FSPermissionChecker checker = new FSPermissionChecker(
+fsOwner, superGroup, ugi, mockINodeAttributeProvider);
Review comment:
NIT: do we have a test case when the attributeProvider=null?
This is an automated message from the Apache Git Service.
To respond to the message, please log on
[GitHub] [hadoop] xiaoyuyao commented on a change in pull request #1829: HDFS-14743. Enhance INodeAttributeProvider/ AccessControlEnforcer Interface in HDFS to support Authorization of mkdir, rm, rmdi
xiaoyuyao commented on a change in pull request #1829: HDFS-14743. Enhance INodeAttributeProvider/ AccessControlEnforcer Interface in HDFS to support Authorization of mkdir, rm, rmdir, copy, move etc... URL: https://github.com/apache/hadoop/pull/1829#discussion_r391179551 ## File path: hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeAttributeProvider.java ## @@ -68,6 +391,16 @@ public abstract void checkPermission(String fsOwner, String supergroup, boolean ignoreEmptyDir) throws AccessControlException; +/** + * Checks permission on a file system object. Has to throw an Exception + * if the filesystem object is not accessessible by the calling Ugi. Review comment: NIT: typo: accessessible 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] xiaoyuyao commented on a change in pull request #1829: HDFS-14743. Enhance INodeAttributeProvider/ AccessControlEnforcer Interface in HDFS to support Authorization of mkdir, rm, rmdi
xiaoyuyao commented on a change in pull request #1829: HDFS-14743. Enhance
INodeAttributeProvider/ AccessControlEnforcer Interface in HDFS to support
Authorization of mkdir, rm, rmdir, copy, move etc...
URL: https://github.com/apache/hadoop/pull/1829#discussion_r391173121
##
File path:
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
##
@@ -1982,6 +1982,7 @@ void setPermission(String src, FsPermission permission)
throws IOException {
FileStatus auditStat;
checkOperation(OperationCategory.WRITE);
final FSPermissionChecker pc = getPermissionChecker();
+FSPermissionChecker.setOperationType(operationName);
Review comment:
There are other places that need to be patched with setOperationType After
HDFS-7416 refactor, not all permission check is done in FSN.
Here is the list of missed ones:
FSDirSymlinkOp#createSymlinkInt()
NameNodeAdapter#getFileInfo()
NamenodeFsck#getBlockLocations()
FSNDNCache#addCacheDirective/removeCacheDirective/modifyCacheDirective/listCacheDirectives/listCachePools
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] xiaoyuyao commented on a change in pull request #1829: HDFS-14743. Enhance INodeAttributeProvider/ AccessControlEnforcer Interface in HDFS to support Authorization of mkdir, rm, rmdi
xiaoyuyao commented on a change in pull request #1829: HDFS-14743. Enhance
INodeAttributeProvider/ AccessControlEnforcer Interface in HDFS to support
Authorization of mkdir, rm, rmdir, copy, move etc...
URL: https://github.com/apache/hadoop/pull/1829#discussion_r377978605
##
File path:
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeAttributeProvider.java
##
@@ -17,19 +17,227 @@
*/
package org.apache.hadoop.hdfs.server.namenode;
+import com.google.common.annotations.VisibleForTesting;
import org.apache.commons.lang3.StringUtils;
import org.apache.hadoop.classification.InterfaceAudience;
import org.apache.hadoop.classification.InterfaceStability;
import org.apache.hadoop.fs.Path;
import org.apache.hadoop.fs.permission.FsAction;
import org.apache.hadoop.hdfs.DFSUtil;
+import org.apache.hadoop.ipc.CallerContext;
import org.apache.hadoop.security.AccessControlException;
import org.apache.hadoop.security.UserGroupInformation;
@InterfaceAudience.Public
@InterfaceStability.Unstable
public abstract class INodeAttributeProvider {
+ public static class AuthorizationContext {
+public String fsOwner;
+public String supergroup;
+public UserGroupInformation callerUgi;
+public INodeAttributes[] inodeAttrs;
+public INode[] inodes;
+public byte[][] pathByNameArr;
+public int snapshotId;
+public String path;
+public int ancestorIndex;
+public boolean doCheckOwner;
+public FsAction ancestorAccess;
+public FsAction parentAccess;
+public FsAction access;
+public FsAction subAccess;
+public boolean ignoreEmptyDir;
+public String operationName;
+public CallerContext callerContext;
+
+public static class Builder {
+ public String fsOwner;
+ public String supergroup;
+ public UserGroupInformation callerUgi;
+ public INodeAttributes[] inodeAttrs;
+ public INode[] inodes;
+ public byte[][] pathByNameArr;
+ public int snapshotId;
+ public String path;
+ public int ancestorIndex;
+ public boolean doCheckOwner;
+ public FsAction ancestorAccess;
+ public FsAction parentAccess;
+ public FsAction access;
+ public FsAction subAccess;
+ public boolean ignoreEmptyDir;
+ public String operationName;
+ public CallerContext callerContext;
+
+ public AuthorizationContext build() {
+return new AuthorizationContext(this);
+ }
+
+ public Builder fsOwner(String val) {
+this.fsOwner = val;
+return this;
+ }
+
+ public Builder supergroup(String val) {
+this.supergroup = val;
+return this;
+ }
+
+ public Builder callerUgi(UserGroupInformation val) {
+this.callerUgi = val;
+return this;
+ }
+
+ public Builder inodeAttrs(INodeAttributes[] val) {
+this.inodeAttrs = val;
+return this;
+ }
+
+ public Builder inodes(INode[] val) {
+this.inodes = val;
+return this;
+ }
+
+ public Builder pathByNameArr(byte[][] val) {
+this.pathByNameArr = val;
+return this;
+ }
+
+ public Builder snapshotId(int val) {
+this.snapshotId = val;
+return this;
+ }
+
+ public Builder path(String val) {
+this.path = val;
+return this;
+ }
+
+ public Builder ancestorIndex(int val) {
+this.ancestorIndex = val;
+return this;
+ }
+
+ public Builder doCheckOwner(boolean val) {
+this.doCheckOwner = val;
+return this;
+ }
+
+ public Builder ancestorAccess(FsAction val) {
+this.ancestorAccess = val;
+return this;
+ }
+
+ public Builder parentAccess(FsAction val) {
+this.parentAccess = val;
+return this;
+ }
+
+ public Builder access(FsAction val) {
+this.access = val;
+return this;
+ }
+
+ public Builder subAccess(FsAction val) {
+this.subAccess = val;
+return this;
+ }
+
+ public Builder ignoreEmptyDir(boolean val) {
+this.ignoreEmptyDir = val;
+return this;
+ }
+
+ public Builder operationName(String val) {
+this.operationName = val;
+return this;
+ }
+
+ public Builder callerContext(CallerContext val) {
+this.callerContext = val;
+return this;
+ }
+}
+
+public AuthorizationContext(
+String fsOwner,
+String supergroup,
+UserGroupInformation callerUgi,
+INodeAttributes[] inodeAttrs,
+INode[] inodes,
+byte[][] pathByNameArr,
+int snapshotId,
+String path,
+int ancestorIndex,
+boolean doCheckOwner,
+FsAction ancestorAccess,
+FsAction parentAccess,
+FsAction access,
+FsAction subAccess,
+boolean ignoreEmptyDir) {
+ this.fsOwner = fsOwner;
+ this.supergroup =
[GitHub] [hadoop] xiaoyuyao commented on a change in pull request #1829: HDFS-14743. Enhance INodeAttributeProvider/ AccessControlEnforcer Interface in HDFS to support Authorization of mkdir, rm, rmdi
xiaoyuyao commented on a change in pull request #1829: HDFS-14743. Enhance
INodeAttributeProvider/ AccessControlEnforcer Interface in HDFS to support
Authorization of mkdir, rm, rmdir, copy, move etc...
URL: https://github.com/apache/hadoop/pull/1829#discussion_r377977917
##
File path:
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeAttributeProvider.java
##
@@ -17,19 +17,227 @@
*/
package org.apache.hadoop.hdfs.server.namenode;
+import com.google.common.annotations.VisibleForTesting;
import org.apache.commons.lang3.StringUtils;
import org.apache.hadoop.classification.InterfaceAudience;
import org.apache.hadoop.classification.InterfaceStability;
import org.apache.hadoop.fs.Path;
import org.apache.hadoop.fs.permission.FsAction;
import org.apache.hadoop.hdfs.DFSUtil;
+import org.apache.hadoop.ipc.CallerContext;
import org.apache.hadoop.security.AccessControlException;
import org.apache.hadoop.security.UserGroupInformation;
@InterfaceAudience.Public
@InterfaceStability.Unstable
public abstract class INodeAttributeProvider {
+ public static class AuthorizationContext {
+public String fsOwner;
+public String supergroup;
+public UserGroupInformation callerUgi;
+public INodeAttributes[] inodeAttrs;
+public INode[] inodes;
+public byte[][] pathByNameArr;
+public int snapshotId;
+public String path;
+public int ancestorIndex;
+public boolean doCheckOwner;
+public FsAction ancestorAccess;
+public FsAction parentAccess;
+public FsAction access;
+public FsAction subAccess;
+public boolean ignoreEmptyDir;
+public String operationName;
+public CallerContext callerContext;
+
+public static class Builder {
+ public String fsOwner;
+ public String supergroup;
+ public UserGroupInformation callerUgi;
+ public INodeAttributes[] inodeAttrs;
+ public INode[] inodes;
+ public byte[][] pathByNameArr;
+ public int snapshotId;
+ public String path;
+ public int ancestorIndex;
+ public boolean doCheckOwner;
+ public FsAction ancestorAccess;
+ public FsAction parentAccess;
+ public FsAction access;
+ public FsAction subAccess;
+ public boolean ignoreEmptyDir;
+ public String operationName;
+ public CallerContext callerContext;
+
+ public AuthorizationContext build() {
+return new AuthorizationContext(this);
+ }
+
+ public Builder fsOwner(String val) {
+this.fsOwner = val;
+return this;
+ }
+
+ public Builder supergroup(String val) {
+this.supergroup = val;
+return this;
+ }
+
+ public Builder callerUgi(UserGroupInformation val) {
+this.callerUgi = val;
+return this;
+ }
+
+ public Builder inodeAttrs(INodeAttributes[] val) {
+this.inodeAttrs = val;
+return this;
+ }
+
+ public Builder inodes(INode[] val) {
+this.inodes = val;
+return this;
+ }
+
+ public Builder pathByNameArr(byte[][] val) {
+this.pathByNameArr = val;
+return this;
+ }
+
+ public Builder snapshotId(int val) {
+this.snapshotId = val;
+return this;
+ }
+
+ public Builder path(String val) {
+this.path = val;
+return this;
+ }
+
+ public Builder ancestorIndex(int val) {
+this.ancestorIndex = val;
+return this;
+ }
+
+ public Builder doCheckOwner(boolean val) {
+this.doCheckOwner = val;
+return this;
+ }
+
+ public Builder ancestorAccess(FsAction val) {
+this.ancestorAccess = val;
+return this;
+ }
+
+ public Builder parentAccess(FsAction val) {
+this.parentAccess = val;
+return this;
+ }
+
+ public Builder access(FsAction val) {
+this.access = val;
+return this;
+ }
+
+ public Builder subAccess(FsAction val) {
+this.subAccess = val;
+return this;
+ }
+
+ public Builder ignoreEmptyDir(boolean val) {
+this.ignoreEmptyDir = val;
+return this;
+ }
+
+ public Builder operationName(String val) {
+this.operationName = val;
+return this;
+ }
+
+ public Builder callerContext(CallerContext val) {
+this.callerContext = val;
+return this;
+ }
+}
+
+public AuthorizationContext(
+String fsOwner,
+String supergroup,
+UserGroupInformation callerUgi,
+INodeAttributes[] inodeAttrs,
+INode[] inodes,
+byte[][] pathByNameArr,
+int snapshotId,
+String path,
+int ancestorIndex,
+boolean doCheckOwner,
+FsAction ancestorAccess,
+FsAction parentAccess,
+FsAction access,
+FsAction subAccess,
+boolean ignoreEmptyDir) {
+ this.fsOwner = fsOwner;
+ this.supergroup =
[GitHub] [hadoop] xiaoyuyao commented on a change in pull request #1829: HDFS-14743. Enhance INodeAttributeProvider/ AccessControlEnforcer Interface in HDFS to support Authorization of mkdir, rm, rmdi
xiaoyuyao commented on a change in pull request #1829: HDFS-14743. Enhance INodeAttributeProvider/ AccessControlEnforcer Interface in HDFS to support Authorization of mkdir, rm, rmdir, copy, move etc... URL: https://github.com/apache/hadoop/pull/1829#discussion_r377977497 ## File path: hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeAttributeProvider.java ## @@ -68,6 +277,8 @@ public abstract void checkPermission(String fsOwner, String supergroup, boolean ignoreEmptyDir) throws AccessControlException; +void checkPermissionWithContext(AuthorizationContext authzContext) Review comment: NIT: can you add javadoc for the new public method? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
