Repository: sentry Updated Branches: refs/heads/master 938b2c2ba -> 60a05f618
SENTRY-1609: DelegateSentryStore is subject to JDQL injection (Alex Kolbasov, Review by: Vamsee Yarlagadda and kalyan kumar kalvagadda) The fix re-implements getGroupsByRoles() using SentryStore methods instead of directly issuing queries. Project: http://git-wip-us.apache.org/repos/asf/sentry/repo Commit: http://git-wip-us.apache.org/repos/asf/sentry/commit/60a05f61 Tree: http://git-wip-us.apache.org/repos/asf/sentry/tree/60a05f61 Diff: http://git-wip-us.apache.org/repos/asf/sentry/diff/60a05f61 Branch: refs/heads/master Commit: 60a05f618cd192c564a519f1437173d6dd60749a Parents: 938b2c2 Author: Alexander Kolbasov <[email protected]> Authored: Wed Feb 8 12:23:35 2017 -0800 Committer: Alexander Kolbasov <[email protected]> Committed: Wed Feb 8 12:25:35 2017 -0800 ---------------------------------------------------------------------- .../service/persistent/DelegateSentryStore.java | 51 ++++++++------------ .../db/service/persistent/SentryStore.java | 2 +- 2 files changed, 22 insertions(+), 31 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/sentry/blob/60a05f61/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java ---------------------------------------------------------------------- diff --git a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java index f65c83f..27dbfca 100644 --- a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java +++ b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java @@ -18,12 +18,10 @@ package org.apache.sentry.provider.db.generic.service.persistent; import javax.jdo.PersistenceManager; -import javax.jdo.Query; import java.util.Arrays; import java.util.Collections; import java.util.HashSet; -import java.util.LinkedList; import java.util.List; import java.util.Set; @@ -45,7 +43,6 @@ import org.apache.sentry.provider.db.service.thrift.TSentryRole; import org.apache.sentry.service.thrift.ServiceConstants.ServerConfig; import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.Joiner; import com.google.common.base.Preconditions; import com.google.common.base.Strings; import com.google.common.collect.ImmutableSet; @@ -256,37 +253,31 @@ public class DelegateSentryStore implements SentryStoreLayer { @Override public Set<String> getGroupsByRoles(final String component, final Set<String> roles) throws Exception { + // In all calls roles contain exactly one group if (roles.isEmpty()) { return Collections.emptySet(); } - return delegate.getTransactionManager().executeTransaction( - new TransactionBlock<Set<String>>() { - public Set<String> execute(PersistenceManager pm) throws Exception { - //get groups by roles - final Set<String> trimmedRoles = SentryStore.toTrimedLower(roles); - - Query query = pm.newQuery(MSentryGroup.class); - StringBuilder filters = new StringBuilder(); - query.declareVariables("MSentryRole role"); - List<String> rolesFiler = new LinkedList<>(); - for (String role : trimmedRoles) { - rolesFiler.add("role.roleName == \"" + role + "\" "); - } - filters.append("roles.contains(role) " + "&& (" + Joiner.on(" || ").join(rolesFiler) + ")"); - query.setFilter(filters.toString()); - @SuppressWarnings("unchecked") - List<MSentryGroup> groups = (List<MSentryGroup>)query.execute(); - if (groups.isEmpty()) { - return Collections.emptySet(); - } - Set<String> groupNames = new HashSet<>(groups.size()); - for (MSentryGroup group : groups) { - groupNames.add(group.getGroupName()); - } - return groupNames; - } - }); + // Collect resulting group names in a set + Set<String> groupNames = new HashSet<>(); + for (String role : roles) { + MSentryRole sentryRole = null; + try { + sentryRole = delegate.getMSentryRoleByName(role); + } + catch (SentryNoSuchObjectException e) { + // Role disappeared - not a big deal, just ognore it + continue; + } + // Collect all group names for this role. + // Since we use a set, a group can appear multiple times and will only + // show up once in a set + for (MSentryGroup group : sentryRole.getGroups()) { + groupNames.add(group.getGroupName()); + } + } + + return groupNames; } @Override http://git-wip-us.apache.org/repos/asf/sentry/blob/60a05f61/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java ---------------------------------------------------------------------- diff --git a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java index 2e63d05..7b926a5 100644 --- a/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java +++ b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java @@ -939,7 +939,7 @@ public class SentryStore { } @VisibleForTesting - MSentryRole getMSentryRoleByName(final String roleName) throws Exception { + public MSentryRole getMSentryRoleByName(final String roleName) throws Exception { return tm.executeTransaction( new TransactionBlock<MSentryRole>() { public MSentryRole execute(PersistenceManager pm) throws Exception {
