You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ta...@apache.org on 2018/04/10 16:09:29 UTC
[2/2] impala git commit: IMPALA-6817: Clean up Impala privilege model
IMPALA-6817: Clean up Impala privilege model
When we added support for multiple versions of Hadoop, the Impala
privilege model was copied from Sentry's Hive privilege model with
some modification. This patch aims to clean up that code.
Testing:
- Added new tests
- Ran all front-end tests
Cherry-picks: not for 2.x
Change-Id: I65f3f9b4d06f3b03bfa2f484737bb746ec598a6b
Reviewed-on: http://gerrit.cloudera.org:8080/9942
Reviewed-by: Alex Behm <al...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/4c1538ab
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/4c1538ab
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/4c1538ab
Branch: refs/heads/master
Commit: 4c1538ab106ec2097927e7586e5d736010706d6a
Parents: ae80979
Author: Fredy Wijaya <fw...@cloudera.com>
Authored: Thu Apr 5 22:16:51 2018 -0500
Committer: Impala Public Jenkins <im...@cloudera.com>
Committed: Tue Apr 10 08:28:26 2018 +0000
----------------------------------------------------------------------
.../authorization/ImpalaActionFactory.java | 66 ++++------
.../authorization/ImpalaPrivilegeModel.java | 6 +-
.../authorization/AuthorizationChecker.java | 11 +-
.../apache/impala/authorization/Privilege.java | 72 +++++-----
.../authorization/ImpalaActionFactoryTest.java | 132 +++++++++++++++++++
5 files changed, 200 insertions(+), 87 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/4c1538ab/fe/src/compat-minicluster-profile-3/java/org/apache/impala/authorization/ImpalaActionFactory.java
----------------------------------------------------------------------
diff --git a/fe/src/compat-minicluster-profile-3/java/org/apache/impala/authorization/ImpalaActionFactory.java b/fe/src/compat-minicluster-profile-3/java/org/apache/impala/authorization/ImpalaActionFactory.java
index da22386..c3ef004 100644
--- a/fe/src/compat-minicluster-profile-3/java/org/apache/impala/authorization/ImpalaActionFactory.java
+++ b/fe/src/compat-minicluster-profile-3/java/org/apache/impala/authorization/ImpalaActionFactory.java
@@ -16,64 +16,42 @@
*/
package org.apache.impala.authorization;
+import com.google.common.base.Preconditions;
+import org.apache.impala.authorization.Privilege.ImpalaAction;
import org.apache.sentry.core.common.BitFieldAction;
import org.apache.sentry.core.common.BitFieldActionFactory;
-import org.apache.sentry.core.model.db.AccessConstants;
+import java.util.ArrayList;
import java.util.List;
/**
- * Almost identical to HiveActionFactory, but extended to allow
- * for "refresh".
+ * An implementation of BitFieldActionFactory for Impala.
*/
public class ImpalaActionFactory extends BitFieldActionFactory {
-
- enum ActionType {
- SELECT(AccessConstants.SELECT, 1),
- INSERT(AccessConstants.INSERT, 2),
- ALTER(AccessConstants.ALTER, 4),
- CREATE(AccessConstants.CREATE, 8),
- DROP(AccessConstants.DROP, 16),
- INDEX(AccessConstants.INDEX, 32),
- LOCK(AccessConstants.LOCK, 64),
- // "refresh" isn't available in AccessConstants, so using an Impala constant.
- REFRESH(Privilege.SentryAction.REFRESH.name(), 128),
-
- // For the compatibility, ALL, ALL_STAR, SOME have the same binary value;
- // They have the different names which are "ALL", "*", "+"
- ALL(AccessConstants.ACTION_ALL, SELECT.getCode() | INSERT.getCode() | ALTER.getCode() | CREATE.getCode() |
- DROP.getCode() | INDEX.getCode() | LOCK.getCode() | REFRESH.getCode()),
- ALL_STAR(AccessConstants.ALL, ALL.getCode()),
- SOME(AccessConstants.SOME, ALL.getCode());
-
- final private String name;
- final private int code;
-
- ActionType(String name, int code) {
- this.name = name;
- this.code = code;
- }
-
- public int getCode() {
- return code;
- }
-
- public String getName() {
- return name;
- }
- }
-
+ @Override
public List<? extends BitFieldAction> getActionsByCode(int actionCode) {
- return null;
+ Preconditions.checkArgument(
+ actionCode >= 1 && actionCode <= ImpalaAction.ALL.getCode(),
+ String.format("Action code must between 1 and %d.", ImpalaAction.ALL.getCode()));
+
+ List<BitFieldAction> actions = new ArrayList<>();
+ for (ImpalaAction action : ImpalaAction.values()) {
+ if ((action.getCode() & actionCode) == action.getCode()) {
+ actions.add(action.getBitFieldAction());
+ }
+ }
+ return actions;
}
+ @Override
public BitFieldAction getActionByName(String name) {
- for (ActionType action : ActionType.values()) {
- if (action.name.equalsIgnoreCase(name)) {
- return new BitFieldAction(action.getName(), action.getCode());
+ Preconditions.checkNotNull(name);
+
+ for (ImpalaAction action : ImpalaAction.values()) {
+ if (action.getValue().equalsIgnoreCase(name)) {
+ return action.getBitFieldAction();
}
}
return null;
}
-
}
http://git-wip-us.apache.org/repos/asf/impala/blob/4c1538ab/fe/src/compat-minicluster-profile-3/java/org/apache/impala/authorization/ImpalaPrivilegeModel.java
----------------------------------------------------------------------
diff --git a/fe/src/compat-minicluster-profile-3/java/org/apache/impala/authorization/ImpalaPrivilegeModel.java b/fe/src/compat-minicluster-profile-3/java/org/apache/impala/authorization/ImpalaPrivilegeModel.java
index 0fd0a70..43a194e 100644
--- a/fe/src/compat-minicluster-profile-3/java/org/apache/impala/authorization/ImpalaPrivilegeModel.java
+++ b/fe/src/compat-minicluster-profile-3/java/org/apache/impala/authorization/ImpalaPrivilegeModel.java
@@ -25,12 +25,11 @@ import org.apache.sentry.core.common.Model;
/**
* Delegates to HivePrivilegeModel for getImplyMethodMap(), but
- * adds "refresh" to the BitFieldActionFactory.
+ * uses Impala's BitFieldActionFactory implementation.
*/
public class ImpalaPrivilegeModel implements Model {
public static final ImpalaPrivilegeModel INSTANCE = new ImpalaPrivilegeModel();
-
- private ImpalaActionFactory actionFactory = new ImpalaActionFactory();
+ private final ImpalaActionFactory actionFactory = new ImpalaActionFactory();
@Override
public Map<String, ImplyMethodType> getImplyMethodMap() {
@@ -41,5 +40,4 @@ public class ImpalaPrivilegeModel implements Model {
public BitFieldActionFactory getBitFieldActionFactory() {
return actionFactory;
}
-
}
http://git-wip-us.apache.org/repos/asf/impala/blob/4c1538ab/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java b/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
index 9fcf020..d1d906c 100644
--- a/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
+++ b/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
@@ -22,19 +22,14 @@ import java.util.EnumSet;
import java.util.List;
import java.util.Set;
-import org.apache.commons.lang.reflect.ConstructorUtils;
-import org.apache.impala.authorization.Privilege.SentryAction;
+import org.apache.impala.authorization.Privilege.ImpalaAction;
import org.apache.impala.catalog.AuthorizationException;
import org.apache.impala.catalog.AuthorizationPolicy;
import org.apache.impala.common.InternalException;
import org.apache.sentry.core.common.ActiveRoleSet;
import org.apache.sentry.core.common.Subject;
import org.apache.sentry.core.model.db.DBModelAuthorizable;
-import org.apache.sentry.provider.cache.SimpleCacheProviderBackend;
-import org.apache.sentry.provider.common.ProviderBackend;
-import org.apache.sentry.provider.common.ProviderBackendContext;
import org.apache.sentry.provider.common.ResourceAuthorizationProvider;
-import org.apache.sentry.provider.file.SimpleFileProviderBackend;
import com.google.common.base.Preconditions;
import com.google.common.collect.Lists;
@@ -145,7 +140,7 @@ public class AuthorizationChecker {
return true;
}
- EnumSet<SentryAction> actions = request.getPrivilege().getSentryActions();
+ EnumSet<ImpalaAction> actions = request.getPrivilege().getSentryActions();
List<DBModelAuthorizable> authorizeables = Lists.newArrayList(
server_.getHiveAuthorizeableHierarchy());
@@ -157,7 +152,7 @@ public class AuthorizationChecker {
// The Hive Access API does not currently provide a way to check if the user
// has any privileges on a given resource.
if (request.getPrivilege().getAnyOf()) {
- for (SentryAction action: actions) {
+ for (ImpalaAction action: actions) {
if (provider_.hasAccess(new Subject(user.getShortName()), authorizeables,
EnumSet.of(action), ActiveRoleSet.ALL)) {
return true;
http://git-wip-us.apache.org/repos/asf/impala/blob/4c1538ab/fe/src/main/java/org/apache/impala/authorization/Privilege.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/authorization/Privilege.java b/fe/src/main/java/org/apache/impala/authorization/Privilege.java
index 59cc8d3..7afe127 100644
--- a/fe/src/main/java/org/apache/impala/authorization/Privilege.java
+++ b/fe/src/main/java/org/apache/impala/authorization/Privilege.java
@@ -20,27 +20,28 @@ package org.apache.impala.authorization;
import java.util.EnumSet;
import org.apache.sentry.core.common.Action;
+import org.apache.sentry.core.common.BitFieldAction;
/**
- * Maps an Impala Privilege to one or more Sentry "Actions".
+ * Maps an Impala Privilege to one or more Impala "Actions".
*/
public enum Privilege {
- ALL(SentryAction.ALL, false),
- ALTER(SentryAction.ALTER, false),
- DROP(SentryAction.DROP, false),
- CREATE(SentryAction.CREATE, false),
- INSERT(SentryAction.INSERT, false),
- SELECT(SentryAction.SELECT, false),
- REFRESH(SentryAction.REFRESH, false),
+ ALL(ImpalaAction.ALL, false),
+ ALTER(ImpalaAction.ALTER, false),
+ DROP(ImpalaAction.DROP, false),
+ CREATE(ImpalaAction.CREATE, false),
+ INSERT(ImpalaAction.INSERT, false),
+ SELECT(ImpalaAction.SELECT, false),
+ REFRESH(ImpalaAction.REFRESH, false),
// Privileges required to view metadata on a server object.
- VIEW_METADATA(EnumSet.of(SentryAction.INSERT, SentryAction.SELECT,
- SentryAction.REFRESH), true),
+ VIEW_METADATA(EnumSet.of(ImpalaAction.INSERT, ImpalaAction.SELECT,
+ ImpalaAction.REFRESH), true),
// Special privilege that is used to determine if the user has any valid privileges
// on a target object.
- ANY(EnumSet.allOf(SentryAction.class), true),
+ ANY(EnumSet.allOf(ImpalaAction.class), true),
;
- private final EnumSet<SentryAction> actions;
+ private final EnumSet<ImpalaAction> actions;
// Determines whether to check if the user has ANY the privileges defined in the
// actions list or whether to check if the user has ALL of the privileges in the
@@ -50,39 +51,48 @@ public enum Privilege {
/**
* This enum provides a list of Sentry actions used in Impala.
*/
- public enum SentryAction implements Action {
- SELECT("select"),
- INSERT("insert"),
- REFRESH("refresh"),
- CREATE("create"),
- ALTER("alter"),
- DROP("drop"),
- ALL("*");
-
- private final String value;
-
- SentryAction(String value) {
- this.value = value;
- }
+ public enum ImpalaAction implements Action {
+ SELECT("select", 1),
+ INSERT("insert", 1 << 2),
+ ALTER("alter", 1 << 3),
+ CREATE("create", 1 << 4),
+ DROP("drop", 1 << 5),
+ REFRESH("refresh", 1 << 6),
+ ALL("*",
+ SELECT.getCode() |
+ INSERT.getCode() |
+ ALTER.getCode() |
+ CREATE.getCode() |
+ DROP.getCode() |
+ REFRESH.getCode());
+
+ private final BitFieldAction bitFieldAction_;
- public String getValue() {
- return value;
+ ImpalaAction(String value, int code) {
+ bitFieldAction_ = new BitFieldAction(value, code);
}
+
+ @Override
+ public String getValue() { return bitFieldAction_.getValue(); }
+
+ public int getCode() { return bitFieldAction_.getActionCode(); }
+
+ public BitFieldAction getBitFieldAction() { return bitFieldAction_; }
}
- private Privilege(EnumSet<SentryAction> actions, boolean anyOf) {
+ private Privilege(EnumSet<ImpalaAction> actions, boolean anyOf) {
this.actions = actions;
this.anyOf_ = anyOf;
}
- private Privilege(SentryAction action, boolean anyOf) {
+ private Privilege(ImpalaAction action, boolean anyOf) {
this(EnumSet.of(action), anyOf);
}
/*
* Returns the set of Sentry Access Actions mapping to this Privilege.
*/
- public EnumSet<SentryAction> getSentryActions() {
+ public EnumSet<ImpalaAction> getSentryActions() {
return actions;
}
http://git-wip-us.apache.org/repos/asf/impala/blob/4c1538ab/fe/src/test/java/org/apache/impala/authorization/ImpalaActionFactoryTest.java
----------------------------------------------------------------------
diff --git a/fe/src/test/java/org/apache/impala/authorization/ImpalaActionFactoryTest.java b/fe/src/test/java/org/apache/impala/authorization/ImpalaActionFactoryTest.java
new file mode 100644
index 0000000..bd39839
--- /dev/null
+++ b/fe/src/test/java/org/apache/impala/authorization/ImpalaActionFactoryTest.java
@@ -0,0 +1,132 @@
+// 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.impala.authorization;
+
+import com.google.common.collect.Lists;
+import org.apache.impala.authorization.Privilege.ImpalaAction;
+import org.apache.sentry.core.common.BitFieldAction;
+import org.junit.Test;
+
+import java.util.List;
+import java.util.Random;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.fail;
+
+public class ImpalaActionFactoryTest {
+ @Test
+ public void testGetActionsByCode() {
+ ImpalaActionFactory factory = new ImpalaActionFactory();
+
+ List<? extends BitFieldAction> actual = factory.getActionsByCode(
+ ImpalaAction.SELECT.getCode() |
+ ImpalaAction.INSERT.getCode() |
+ ImpalaAction.CREATE.getCode());
+ List<ImpalaAction> expected = Lists.newArrayList(
+ ImpalaAction.SELECT,
+ ImpalaAction.INSERT,
+ ImpalaAction.CREATE);
+ assertBitFieldActions(expected, actual);
+
+ actual = factory.getActionsByCode(
+ ImpalaAction.SELECT.getCode() |
+ ImpalaAction.INSERT.getCode() |
+ ImpalaAction.ALTER.getCode() |
+ ImpalaAction.CREATE.getCode() |
+ ImpalaAction.DROP.getCode() |
+ ImpalaAction.REFRESH.getCode());
+ expected = Lists.newArrayList(
+ ImpalaAction.SELECT,
+ ImpalaAction.INSERT,
+ ImpalaAction.ALTER,
+ ImpalaAction.CREATE,
+ ImpalaAction.DROP,
+ ImpalaAction.REFRESH,
+ ImpalaAction.ALL);
+ assertBitFieldActions(expected, actual);
+
+ actual = factory.getActionsByCode(ImpalaAction.ALL.getCode());
+ expected = Lists.newArrayList(
+ ImpalaAction.SELECT,
+ ImpalaAction.INSERT,
+ ImpalaAction.ALTER,
+ ImpalaAction.CREATE,
+ ImpalaAction.DROP,
+ ImpalaAction.REFRESH,
+ ImpalaAction.ALL);
+ assertBitFieldActions(expected, actual);
+
+ try {
+ factory.getActionsByCode(Integer.MAX_VALUE);
+ fail("IllegalArgumentException should be thrown.");
+ } catch (IllegalArgumentException e) {
+ assertEquals(String.format("Action code must between 1 and %d.",
+ ImpalaAction.ALL.getCode()), e.getMessage());
+ }
+
+ try {
+ factory.getActionsByCode(Integer.MIN_VALUE);
+ fail("IllegalArgumentException should be thrown.");
+ } catch (IllegalArgumentException e) {
+ assertEquals(String.format("Action code must between 1 and %d.",
+ ImpalaAction.ALL.getCode()), e.getMessage());
+ }
+ }
+
+ private static void assertBitFieldActions(List<ImpalaAction> expected,
+ List<? extends BitFieldAction> actual) {
+ assertEquals(expected.size(), actual.size());
+ for (int i = 0; i < actual.size(); i++) {
+ assertEquals(expected.get(i).getValue(), actual.get(i).getValue());
+ assertEquals(expected.get(i).getCode(), actual.get(i).getActionCode());
+ }
+ }
+
+ @Test
+ public void testGetActionByName() {
+ ImpalaActionFactory impala = new ImpalaActionFactory();
+
+ for (ImpalaAction action : ImpalaAction.values()) {
+ testGetActionByName(impala, action, action.getValue());
+ }
+ assertNull(impala.getActionByName("foo"));
+ }
+
+ private static void testGetActionByName(ImpalaActionFactory impala,
+ ImpalaAction expected, String name) {
+ assertEquals(toBitFieldAction(expected),
+ impala.getActionByName(name.toUpperCase()));
+ assertEquals(toBitFieldAction(expected),
+ impala.getActionByName(name.toLowerCase()));
+ assertEquals(toBitFieldAction(expected),
+ impala.getActionByName(randomizeCaseSensitivity(name)));
+ }
+
+ private static String randomizeCaseSensitivity(String str) {
+ char[] chars = str.toCharArray();
+ Random random = new Random(System.currentTimeMillis());
+ for (int i = 0; i < chars.length; i++) {
+ chars[i] = (random.nextBoolean()) ? Character.toUpperCase(chars[i]) : chars[i];
+ }
+ return new String(chars);
+ }
+
+ private static BitFieldAction toBitFieldAction(ImpalaAction action) {
+ return new BitFieldAction(action.getValue(), action.getCode());
+ }
+}
\ No newline at end of file