Repository: incubator-sentry Updated Branches: refs/heads/master a79e39a53 -> aab78731a
SENTRY-408: The URI permission should support more filesystem prefixes (Colin Ma via Lenni Kuff) Project: http://git-wip-us.apache.org/repos/asf/incubator-sentry/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-sentry/commit/aab78731 Tree: http://git-wip-us.apache.org/repos/asf/incubator-sentry/tree/aab78731 Diff: http://git-wip-us.apache.org/repos/asf/incubator-sentry/diff/aab78731 Branch: refs/heads/master Commit: aab78731a7a7e8de1ed5344bef94152ac83d6207 Parents: a79e39a Author: Lenni Kuff <[email protected]> Authored: Thu Nov 13 09:38:03 2014 -0800 Committer: Lenni Kuff <[email protected]> Committed: Thu Nov 13 09:38:03 2014 -0800 ---------------------------------------------------------------------- .../binding/hive/HiveAuthzBindingHook.java | 2 +- sentry-core/sentry-core-common/pom.xml | 5 + .../sentry/core/common/utils/PathUtils.java | 118 +++++++++---------- .../sentry/core/common/utils/TestPathUtils.java | 42 +++++++ 4 files changed, 103 insertions(+), 64 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/aab78731/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java ---------------------------------------------------------------------- diff --git a/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java b/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java index 095209a..1e59671 100644 --- a/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java +++ b/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java @@ -290,7 +290,7 @@ public class HiveAuthzBindingHook extends AbstractSemanticAnalyzerHook { try { HiveConf conf = SessionState.get().getConf(); String warehouseDir = conf.getVar(ConfVars.METASTOREWAREHOUSE); - return new AccessURI(PathUtils.parseDFSURI(warehouseDir, uri, isLocal)); + return new AccessURI(PathUtils.parseURI(warehouseDir, uri, isLocal)); } catch (Exception e) { throw new SemanticException("Error parsing URI " + uri + ": " + e.getMessage(), e); http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/aab78731/sentry-core/sentry-core-common/pom.xml ---------------------------------------------------------------------- diff --git a/sentry-core/sentry-core-common/pom.xml b/sentry-core/sentry-core-common/pom.xml index 5c0fe43..feff030 100644 --- a/sentry-core/sentry-core-common/pom.xml +++ b/sentry-core/sentry-core-common/pom.xml @@ -57,6 +57,11 @@ limitations under the License. <artifactId>junit</artifactId> <scope>test</scope> </dependency> + <dependency> + <groupId>org.apache.hadoop</groupId> + <artifactId>hadoop-common</artifactId> + <scope>provided</scope> + </dependency> </dependencies> <build> http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/aab78731/sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java ---------------------------------------------------------------------- diff --git a/sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java b/sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java index 1cdbdb8..73f91ee 100644 --- a/sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java +++ b/sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java @@ -20,21 +20,24 @@ import java.io.File; import java.net.URI; import java.net.URISyntaxException; +import org.apache.commons.lang.StringUtils; import org.apache.commons.lang.text.StrSubstitutor; +import org.apache.hadoop.fs.Path; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import com.google.common.base.Strings; public class PathUtils { - private static final Logger LOGGER = LoggerFactory - .getLogger(PathUtils.class); + private static final Logger LOGGER = LoggerFactory.getLogger(PathUtils.class); + private static String LOCAL_FILE_SCHEMA = "file"; + private static String AUTHORITY_PREFIX = "://"; + /** * URI is a a special case. For URI's, /a implies /a/b. * Therefore the test is "/a/b".startsWith("/a"); */ - public static boolean impliesURI(URI privilegeURI, URI requestURI) - throws URISyntaxException { + public static boolean impliesURI(URI privilegeURI, URI requestURI) throws URISyntaxException { if (privilegeURI.getPath() == null || requestURI.getPath() == null) { return false; } @@ -51,9 +54,10 @@ public class PathUtils { // authorities (nullable) are equal String requestPath = ensureEndsWithSeparator(requestURI.getPath()).replace("//", "/"); String privilegePath = ensureEndsWithSeparator(privilegeURI.getPath()).replace("//", "/"); - if (requestURI.getPath().equals(requestURI.normalize().getPath()) && - requestPath.startsWith(privilegePath) && - Strings.nullToEmpty(privilegeURI.getAuthority()).equals(Strings.nullToEmpty(requestURI.getAuthority()))) { + if (requestURI.getPath().equals(requestURI.normalize().getPath()) + && requestPath.startsWith(privilegePath) + && Strings.nullToEmpty(privilegeURI.getAuthority()).equals( + Strings.nullToEmpty(requestURI.getAuthority()))) { return true; } return false; @@ -61,16 +65,16 @@ public class PathUtils { public static boolean impliesURI(String privilege, String request) { try { - URI privilegeURI = new URI(new StrSubstitutor(System.getProperties()).replace(privilege)); - URI requestURI = new URI(request); - if(privilegeURI.getScheme() == null || privilegeURI.getPath() == null) { - LOGGER.warn("Privilege URI " + request + " is not valid. Either no scheme or no path."); - return false; - } - if(requestURI.getScheme() == null || requestURI.getPath() == null) { - LOGGER.warn("Request URI " + request + " is not valid. Either no scheme or no path."); - return false; - } + URI privilegeURI = new URI(new StrSubstitutor(System.getProperties()).replace(privilege)); + URI requestURI = new URI(request); + if (privilegeURI.getScheme() == null || privilegeURI.getPath() == null) { + LOGGER.warn("Privilege URI " + request + " is not valid. Either no scheme or no path."); + return false; + } + if (requestURI.getScheme() == null || requestURI.getPath() == null) { + LOGGER.warn("Request URI " + request + " is not valid. Either no scheme or no path."); + return false; + } return PathUtils.impliesURI(privilegeURI, requestURI); } catch (URISyntaxException e) { LOGGER.warn("Request URI " + request + " is not a URI", e); @@ -92,45 +96,39 @@ public class PathUtils { return path + File.separator; } - public static String parseDFSURI(String warehouseDir, String uri) - throws URISyntaxException { - return parseDFSURI(warehouseDir, uri, false); + public static String parseDFSURI(String warehouseDir, String uri) throws URISyntaxException { + return parseURI(warehouseDir, uri, false); } /** - * Parse a URI which should be on HDFS in the normal case but can be on a local - * file system in the testing case. In either case it should be on the same fs - * as the warehouse directory. + * Parse a URI which can be HDFS, S3, SWIFT, WEBHDFS,etc. In either case it + * should be on the same fs as the warehouse directory. */ - public static String parseDFSURI(String warehouseDir, String uri, boolean isLocal) + public static String parseURI(String warehouseDir, String uri, boolean isLocal) throws URISyntaxException { - if ((uri.startsWith("file://") || uri.startsWith("hdfs://"))) { - return uri; - } else { - if (uri.startsWith("file:")) { - uri = uri.replace("file:", "file://"); - } if (uri.startsWith("hdfs:")) { - uri = uri.replace("hdfs:", "hdfs://"); - } else if (uri.startsWith("/")) { - if (warehouseDir.startsWith("hdfs:")) { - URI warehouse = toDFSURI(warehouseDir); - uri = warehouse.getScheme() + "://" + warehouse.getAuthority() + uri; - } else if (warehouseDir.startsWith("file:")) { - uri = "file://" + uri; - } else { - if (isLocal) { - uri = "file://" + uri; - } else { - // TODO fix this logic. I don't see why we would want to add hdfs:// - // to a URI at this point in time since no namenode is specified - // and warehouseDir appear to just be a path starting with / ? - // I think in the isLocal = false case we might want to throw - uri = "hdfs://" + uri; - } - } + Path warehouseDirPath = new Path(warehouseDir); + Path uriPath = new Path(uri); + + if (uriPath.isAbsolute()) { + // Merge warehouseDir and uri only when there is no scheme and authority + // in uri. + if (uriPath.isAbsoluteAndSchemeAuthorityNull()) { + uriPath = uriPath.makeQualified(warehouseDirPath.toUri(), warehouseDirPath); } - return uri; + String uriScheme = uriPath.toUri().getScheme(); + String uriAuthority = uriPath.toUri().getAuthority(); + + if (StringUtils.isEmpty(uriScheme) || isLocal) { + uriScheme = LOCAL_FILE_SCHEMA; + } + + uriPath = new Path(uriScheme + AUTHORITY_PREFIX + StringUtils.trimToEmpty(uriAuthority) + + Path.getPathWithoutSchemeAndAuthority(uriPath)); + } else { + // don't support relative path + throw new IllegalArgumentException("Invalid URI " + uri + "."); } + return uriPath.toUri().toString(); } /** @@ -138,21 +136,15 @@ public class PathUtils { */ public static String parseLocalURI(String uri) throws URISyntaxException { - if (uri.startsWith("file://")) { - return uri; - } else if (uri.startsWith("file:")) { - return uri.replace("file:", "file://"); - } else if (uri.startsWith("/")) { - return "file://" + uri; + Path uriPath = new Path(uri); + if (uriPath.isAbsolute()) { + uriPath = new Path(LOCAL_FILE_SCHEMA + AUTHORITY_PREFIX + + StringUtils.trimToEmpty(uriPath.toUri().getAuthority()) + + Path.getPathWithoutSchemeAndAuthority(uriPath)); + } else { + throw new IllegalArgumentException("Parse URI does not work on relative URI: " + uri); } - throw new IllegalStateException("Parse URI does not work on relative URI: " + uri); + return uriPath.toUri().toString(); } - private static URI toDFSURI(String s) throws URISyntaxException { - URI uri = new URI(s); - if(uri.getScheme() == null || uri.getAuthority() == null) { - throw new IllegalArgumentException("Invalid URI " + s + ". No scheme or authority."); - } - return uri; - } } http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/aab78731/sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/utils/TestPathUtils.java ---------------------------------------------------------------------- diff --git a/sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/utils/TestPathUtils.java b/sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/utils/TestPathUtils.java index d30305b..09fc91b 100644 --- a/sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/utils/TestPathUtils.java +++ b/sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/utils/TestPathUtils.java @@ -19,6 +19,7 @@ package org.apache.sentry.core.common.utils; import static junit.framework.Assert.assertEquals; import static junit.framework.Assert.assertFalse; import static junit.framework.Assert.assertTrue; +import static org.junit.Assert.fail; import java.net.URI; @@ -43,6 +44,23 @@ public class TestPathUtils { // warehouse hdfs, path hdfs assertEquals("hdfs://namenode:8020/tmp/hive-user", PathUtils. parseDFSURI("hdfs://namenode:8020/user/hive/warehouse", "hdfs://namenode:8020/tmp/hive-user")); + try { + PathUtils.parseDFSURI("hdfs://namenode:8020/user/hive/warehouse", "tmp/hive-user"); + fail("IllegalArgumentException should be thrown"); + } catch (IllegalArgumentException ue) { + } + + // warehouse swift, path / + assertEquals("swift://namenode:8020/tmp/hive-user", + PathUtils.parseDFSURI("swift://namenode:8020/user/hive/warehouse", "/tmp/hive-user")); + // warehouse swift, path swift + assertEquals("swift://namenode:8020/tmp/hive-user", PathUtils.parseDFSURI( + "swift://namenode:8020/user/hive/warehouse", "swift://namenode:8020/tmp/hive-user")); + try { + PathUtils.parseDFSURI("swift://namenode:8020/user/hive/warehouse", "tmp/hive-user"); + fail("IllegalArgumentException should be thrown"); + } catch (IllegalArgumentException ue) { + } // warehouse file:///, path / assertEquals("file:///tmp/hive-user", PathUtils. @@ -53,6 +71,11 @@ public class TestPathUtils { // warehouse file:///, path file:/// assertEquals("file:///tmp/hive-user", PathUtils. parseDFSURI("file:///tmp/hive-warehouse", "file:///tmp/hive-user")); + try { + PathUtils.parseDFSURI("file:///hive-warehouse", "tmp/hive-user"); + fail("IllegalArgumentException should be thrown"); + } catch (IllegalArgumentException ue) { + } // warehouse file:/, path / assertEquals("file:///tmp/hive-user", PathUtils. @@ -63,6 +86,23 @@ public class TestPathUtils { // warehouse file:/, path file:/// assertEquals("file:///tmp/hive-user", PathUtils. parseDFSURI("file:/tmp/hive-warehouse", "file:///tmp/hive-user")); + + // for local test case + assertEquals("file:///tmp/hive-user", + PathUtils.parseURI("testLocal:///tmp/hive-warehouse", "/tmp/hive-user", true)); + assertEquals("file://localhost:9999/tmp/hive-user", PathUtils.parseURI( + "file://localhost:9999/tmp/hive-warehouse", "file://localhost:9999/tmp/hive-user", true)); + assertEquals("file://localhost:9999/tmp/hive-user", PathUtils.parseURI( + "file:///tmp/hive-warehouse", "file://localhost:9999/tmp/hive-user", true)); + try { + PathUtils.parseURI("testLocal:///tmp/hive-warehouse", "tmp/hive-user", true); + fail("IllegalStateException should be thrown"); + } catch (IllegalArgumentException ue) { + } + + // warehouse /, path / + assertEquals("file:///tmp/hive-user", + PathUtils.parseDFSURI("/tmp/hive-warehouse", "/tmp/hive-user")); } @Test @@ -73,5 +113,7 @@ public class TestPathUtils { parseLocalURI("file:/tmp/hive-user")); assertEquals("file:///tmp/hive-user", PathUtils. parseLocalURI("file:///tmp/hive-user")); + assertEquals("file://localhost:9999/tmp/hive-user", + PathUtils.parseLocalURI("file://localhost:9999/tmp/hive-user")); } }
