You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sentry.apache.org by sh...@apache.org on 2014/03/12 21:50:58 UTC

git commit: SENTRY-132 - Implement APIs that grant/revoke roles to groups (Brock via Shreepadma)

Repository: incubator-sentry
Updated Branches:
  refs/heads/db_policy_store a7df761dd -> 066f993e5


SENTRY-132 - Implement APIs that grant/revoke roles to groups (Brock via Shreepadma)


Project: http://git-wip-us.apache.org/repos/asf/incubator-sentry/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-sentry/commit/066f993e
Tree: http://git-wip-us.apache.org/repos/asf/incubator-sentry/tree/066f993e
Diff: http://git-wip-us.apache.org/repos/asf/incubator-sentry/diff/066f993e

Branch: refs/heads/db_policy_store
Commit: 066f993e57d23ecfcaade818b133d82e1a6b3f2f
Parents: a7df761
Author: Shreepadma Venugopalan <sh...@apache.org>
Authored: Wed Mar 12 13:50:22 2014 -0700
Committer: Shreepadma Venugopalan <sh...@apache.org>
Committed: Wed Mar 12 13:50:22 2014 -0700

----------------------------------------------------------------------
 .../provider/db/service/model/MSentryGroup.java |  70 ++++++++-
 .../db/service/model/MSentryPrivilege.java      | 120 +++++++++++++--
 .../provider/db/service/model/MSentryRole.java  |  62 +++++++-
 .../db/service/persistent/SentryStore.java      | 112 ++++++++++++--
 .../thrift/SentryPolicyStoreProcessor.java      |  10 +-
 .../main/resources/sentry_policy_service.thrift |   8 +-
 .../db/service/persistent/TestSentryStore.java  | 145 +++++++++++++++++++
 7 files changed, 487 insertions(+), 40 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/066f993e/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGroup.java
----------------------------------------------------------------------
diff --git a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGroup.java b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGroup.java
index 15982a3..b5de36e 100644
--- a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGroup.java
+++ b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGroup.java
@@ -22,17 +22,21 @@ import java.util.Set;
 
 import javax.jdo.annotations.PersistenceCapable;
 
+/**
+ * Database backed Sentry Group. Any changes to this object
+ * require re-running the maven build so DN an re-enhance.
+ */
 @PersistenceCapable
 public class MSentryGroup {
 
-  String groupName;
+  private String groupName;
   // set of roles granted to this group
-  Set<MSentryRole> roles;
-  long createTime;
-  String grantorPrincipal;
+  private Set<MSentryRole> roles;
+  private long createTime;
+  private String grantorPrincipal;
 
-  MSentryGroup(String groupName, long createTime, String grantorPrincipal,
-               Set<MSentryRole> roles) {
+  public MSentryGroup(String groupName, long createTime, String grantorPrincipal,
+      Set<MSentryRole> roles) {
     this.setGroupName(groupName);
     this.createTime = createTime;
     this.grantorPrincipal = grantorPrincipal;
@@ -71,7 +75,57 @@ public class MSentryGroup {
     this.groupName = groupName;
   }
 
-  public void appendRoles(Set<MSentryRole> roles) {
-    this.roles.addAll(roles);
+  public void appendRole(MSentryRole role) {
+    if (roles.add(role)) {
+      role.appendGroup(this);
+    }
+  }
+
+  public void removeRole(MSentryRole role) {
+    if (roles.remove(role)) {
+      role.removeGroup(this);
+    }
+  }
+
+  @Override
+  public String toString() {
+    return "MSentryGroup [groupName=" + groupName + ", roles=[...]"
+        + ", createTime=" + createTime + ", grantorPrincipal="
+        + grantorPrincipal + "]";
+  }
+
+  @Override
+  public int hashCode() {
+    final int prime = 31;
+    int result = 1;
+    result = prime * result + (int) (createTime ^ (createTime >>> 32));
+    result = prime * result
+        + ((grantorPrincipal == null) ? 0 : grantorPrincipal.hashCode());
+    result = prime * result + ((groupName == null) ? 0 : groupName.hashCode());
+    return result;
+  }
+
+  @Override
+  public boolean equals(Object obj) {
+    if (this == obj)
+      return true;
+    if (obj == null)
+      return false;
+    if (getClass() != obj.getClass())
+      return false;
+    MSentryGroup other = (MSentryGroup) obj;
+    if (createTime != other.createTime)
+      return false;
+    if (grantorPrincipal == null) {
+      if (other.grantorPrincipal != null)
+        return false;
+    } else if (!grantorPrincipal.equals(other.grantorPrincipal))
+      return false;
+    if (groupName == null) {
+      if (other.groupName != null)
+        return false;
+    } else if (!groupName.equals(other.groupName))
+      return false;
+    return true;
   }
 }
\ No newline at end of file

http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/066f993e/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java
----------------------------------------------------------------------
diff --git a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java
index 9642689..7215435 100644
--- a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java
+++ b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java
@@ -24,28 +24,32 @@ import java.util.Set;
 
 import javax.jdo.annotations.PersistenceCapable;
 
+/**
+ * Database backed Sentry Privilege. Any changes to this object
+ * require re-running the maven build so DN an re-enhance.
+ */
 @PersistenceCapable
 public class MSentryPrivilege {
 
-  String privilegeScope;
-  String privilegeName;
-  String serverName;
-  String dbName;
-  String tableName;
-  String URI;
-  String action;
+  private String privilegeScope;
+  private String privilegeName;
+  private String serverName;
+  private String dbName;
+  private String tableName;
+  private String URI;
+  private String action;
   // roles this privilege is a part of
-  Set<MSentryRole> roles;
-  long createTime;
-  String grantorPrincipal;
+  private Set<MSentryRole> roles;
+  private long createTime;
+  private String grantorPrincipal;
 
   public MSentryPrivilege() {
     this.roles = new HashSet<MSentryRole>();
   }
 
   public MSentryPrivilege(String privilegeName, String privilegeScope,
-                          String serverName, String dbName, String tableName, String URI,
-                          String action) {
+      String serverName, String dbName, String tableName, String URI,
+      String action) {
     this.privilegeName = privilegeName;
     this.privilegeScope = privilegeScope;
     this.serverName = serverName;
@@ -148,4 +152,96 @@ public class MSentryPrivilege {
       }
     }
   }
+
+  public void removeRole(String roleName) {
+    for (MSentryRole role: roles) {
+      if (role.getRoleName().equalsIgnoreCase(roleName)) {
+        roles.remove(role);
+        return;
+      }
+    }
+  }
+
+  @Override
+  public String toString() {
+    return "MSentryPrivilege [privilegeScope=" + privilegeScope
+        + ", privilegeName=" + privilegeName + ", serverName=" + serverName
+        + ", dbName=" + dbName + ", tableName=" + tableName + ", URI=" + URI
+        + ", action=" + action + ", roles=[...]" + ", createTime="
+        + createTime + ", grantorPrincipal=" + grantorPrincipal + "]";
+  }
+
+  @Override
+  public int hashCode() {
+    final int prime = 31;
+    int result = 1;
+    result = prime * result + ((URI == null) ? 0 : URI.hashCode());
+    result = prime * result + ((action == null) ? 0 : action.hashCode());
+    result = prime * result + (int) (createTime ^ (createTime >>> 32));
+    result = prime * result + ((dbName == null) ? 0 : dbName.hashCode());
+    result = prime * result
+        + ((grantorPrincipal == null) ? 0 : grantorPrincipal.hashCode());
+    result = prime * result
+        + ((privilegeName == null) ? 0 : privilegeName.hashCode());
+    result = prime * result
+        + ((privilegeScope == null) ? 0 : privilegeScope.hashCode());
+    result = prime * result
+        + ((serverName == null) ? 0 : serverName.hashCode());
+    result = prime * result + ((tableName == null) ? 0 : tableName.hashCode());
+    return result;
+  }
+
+  @Override
+  public boolean equals(Object obj) {
+    if (this == obj)
+      return true;
+    if (obj == null)
+      return false;
+    if (getClass() != obj.getClass())
+      return false;
+    MSentryPrivilege other = (MSentryPrivilege) obj;
+    if (URI == null) {
+      if (other.URI != null)
+        return false;
+    } else if (!URI.equals(other.URI))
+      return false;
+    if (action == null) {
+      if (other.action != null)
+        return false;
+    } else if (!action.equals(other.action))
+      return false;
+    if (createTime != other.createTime)
+      return false;
+    if (dbName == null) {
+      if (other.dbName != null)
+        return false;
+    } else if (!dbName.equals(other.dbName))
+      return false;
+    if (grantorPrincipal == null) {
+      if (other.grantorPrincipal != null)
+        return false;
+    } else if (!grantorPrincipal.equals(other.grantorPrincipal))
+      return false;
+    if (privilegeName == null) {
+      if (other.privilegeName != null)
+        return false;
+    } else if (!privilegeName.equals(other.privilegeName))
+      return false;
+    if (privilegeScope == null) {
+      if (other.privilegeScope != null)
+        return false;
+    } else if (!privilegeScope.equals(other.privilegeScope))
+      return false;
+    if (serverName == null) {
+      if (other.serverName != null)
+        return false;
+    } else if (!serverName.equals(other.serverName))
+      return false;
+    if (tableName == null) {
+      if (other.tableName != null)
+        return false;
+    } else if (!tableName.equals(other.tableName))
+      return false;
+    return true;
+  }
 }
\ No newline at end of file

http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/066f993e/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java
----------------------------------------------------------------------
diff --git a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java
index 9559c57..16be80b 100644
--- a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java
+++ b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java
@@ -26,6 +26,10 @@ import javax.jdo.annotations.PersistenceCapable;
 
 import org.apache.sentry.provider.db.service.persistent.SentryNoSuchObjectException;
 
+/**
+ * Database backed Sentry Role. Any changes to this object
+ * require re-running the maven build so DN an re-enhance.
+ */
 @PersistenceCapable
 public class MSentryRole {
 
@@ -42,7 +46,7 @@ public class MSentryRole {
   }
 
   MSentryRole(String roleName, long createTime, String grantorPrincipal,
-              Set<MSentryPrivilege> privileges, Set<MSentryGroup> groups) {
+      Set<MSentryPrivilege> privileges, Set<MSentryGroup> groups) {
     this.roleName = roleName;
     this.createTime = createTime;
     this.grantorPrincipal = grantorPrincipal;
@@ -115,7 +119,61 @@ public class MSentryRole {
     this.groups.addAll(groups);
   }
 
+  public void appendGroup(MSentryGroup group) {
+    if (groups.add(group)) {
+      group.appendRole(this);
+    }
+  }
+
+  public void removeGroup(MSentryGroup group) {
+    if (groups.remove(group)) {
+      group.removeRole(this);
+    }
+  }
+
   public void removePrivileges() {
     this.privileges.clear();
   }
-}
\ No newline at end of file
+
+  @Override
+  public String toString() {
+    return "MSentryRole [roleName=" + roleName + ", privileges=[..]"
+        + ", groups=[...]" + ", createTime=" + createTime
+        + ", grantorPrincipal=" + grantorPrincipal + "]";
+  }
+
+  @Override
+  public int hashCode() {
+    final int prime = 31;
+    int result = 1;
+    result = prime * result + (int) (createTime ^ (createTime >>> 32));
+    result = prime * result
+        + ((grantorPrincipal == null) ? 0 : grantorPrincipal.hashCode());
+    result = prime * result + ((roleName == null) ? 0 : roleName.hashCode());
+    return result;
+  }
+
+  @Override
+  public boolean equals(Object obj) {
+    if (this == obj)
+      return true;
+    if (obj == null)
+      return false;
+    if (getClass() != obj.getClass())
+      return false;
+    MSentryRole other = (MSentryRole) obj;
+    if (createTime != other.createTime)
+      return false;
+    if (grantorPrincipal == null) {
+      if (other.grantorPrincipal != null)
+        return false;
+    } else if (!grantorPrincipal.equals(other.grantorPrincipal))
+      return false;
+    if (roleName == null) {
+      if (other.roleName != null)
+        return false;
+    } else if (!roleName.equals(other.roleName))
+      return false;
+    return true;
+  }
+}

http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/066f993e/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
----------------------------------------------------------------------
diff --git a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
index 5df6657..f1e502a 100644
--- a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
+++ b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
@@ -19,6 +19,7 @@
 package org.apache.sentry.provider.db.service.persistent;
 
 import java.util.HashSet;
+import java.util.List;
 import java.util.Properties;
 import java.util.Set;
 import java.util.UUID;
@@ -29,15 +30,21 @@ import javax.jdo.PersistenceManagerFactory;
 import javax.jdo.Query;
 import javax.jdo.Transaction;
 
+import org.apache.sentry.provider.db.service.model.MSentryGroup;
 import org.apache.sentry.provider.db.service.model.MSentryPrivilege;
 import org.apache.sentry.provider.db.service.model.MSentryRole;
+import org.apache.sentry.provider.db.service.thrift.TSentryGroup;
 import org.apache.sentry.provider.db.service.thrift.TSentryPrivilege;
 import org.apache.sentry.provider.db.service.thrift.TSentryRole;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Sets;
 
 public class SentryStore {
   private static final UUID SERVER_UUID = UUID.randomUUID();
+  static final String DEFAULT_DATA_DIR = "sentry_policy_db";
   /**
    * Commit order sequence id. This is used by notification handlers
    * to know the order in which events where committed to the database.
@@ -48,13 +55,19 @@ public class SentryStore {
   private long commitSequenceId;
   private final Properties prop;
   private final PersistenceManagerFactory pmf;
+  private final String databaseName;
 
-  public SentryStore () {
+  public SentryStore(String dataDir) {
     commitSequenceId = 0;
+    databaseName = (dataDir = dataDir.trim()).isEmpty() ? DEFAULT_DATA_DIR : dataDir;
     prop = getDataSourceProperties();
     pmf = JDOHelper.getPersistenceManagerFactory(prop);
   }
 
+  public SentryStore() {
+    this("");
+  }
+
   public synchronized void stop() {
     if (pmf != null) {
       pmf.close();
@@ -89,7 +102,7 @@ public class SentryStore {
     prop.setProperty("javax.jdo.option.ConnectionPassword", "Sentry");
     prop.setProperty("javax.jdo.option.Multithreaded", "true");
     prop.setProperty("javax.jdo.option.ConnectionURL",
-                     "jdbc:derby:;databaseName=sentry_policy_db;create=true");
+                     "jdbc:derby:;databaseName=" + databaseName + ";create=true");
     return prop;
   }
 
@@ -142,7 +155,7 @@ public class SentryStore {
   }
 
   private void rollbackTransaction(PersistenceManager pm) {
-    if (pm == null) {
+    if (pm == null || pm.isClosed()) {
       return;
     }
     Transaction currentTransaction = pm.currentTransaction();
@@ -200,6 +213,7 @@ public class SentryStore {
         MSentryPrivilege mPrivilege = convertToMSentryPrivilege(privilege);
         // add privilege and role objects to each other. needed by datanucleus to model
         // m:n relationships correctly through a join table.
+        mPrivilege.appendRole(mRole);
         mRole.appendPrivilege(mPrivilege);
         pm.makePersistent(mRole);
         pm.makePersistent(mPrivilege);
@@ -280,19 +294,88 @@ public class SentryStore {
     }
   }
 
-  public CommitContext alterSentryRoleAddGroups()
+  public CommitContext alterSentryRoleAddGroups(String grantorPrincipal,
+      String roleName, Set<TSentryGroup> groupNames)
   throws SentryNoSuchObjectException {
-    // TODO implement
-    throw new RuntimeException("TODO");
+    boolean rollbackTransaction = true;
+    PersistenceManager pm = null;
+    try {
+      pm = openTransaction();
+      Query query = pm.newQuery(MSentryRole.class);
+      query.setFilter("this.roleName == t");
+      query.declareParameters("java.lang.String t");
+      query.setUnique(true);
+      MSentryRole role = (MSentryRole) query.execute(roleName);
+      if (role == null) {
+        throw new SentryNoSuchObjectException("Role: " + roleName);
+      } else {
+        query = pm.newQuery(MSentryGroup.class);
+        query.setFilter("this.groupName == t");
+        query.declareParameters("java.lang.String t");
+        query.setUnique(true);
+        List<MSentryGroup> groups = Lists.newArrayList();
+        for (TSentryGroup tGroup : groupNames) {
+          MSentryGroup group = (MSentryGroup) query.execute(tGroup.getGroupName());
+          if (group == null) {
+            group = new MSentryGroup(tGroup.getGroupName(), System.currentTimeMillis(),
+                grantorPrincipal, Sets.newHashSet(role));
+          }
+          group.appendRole(role);
+          groups.add(group);
+        }
+        pm.makePersistentAll(groups);
+        CommitContext commit = commitUpdateTransaction(pm);
+        rollbackTransaction = false;
+        return commit;
+      }
+    } finally {
+      if (rollbackTransaction) {
+        rollbackTransaction(pm);
+      }
+    }
   }
 
-  public CommitContext alterSentryRoleDeleteGroups()
+  public CommitContext alterSentryRoleDeleteGroups(String roleName,
+      Set<TSentryGroup> groupNames)
   throws SentryNoSuchObjectException {
-    // TODO implement
-    throw new RuntimeException("TODO");
+    boolean rollbackTransaction = true;
+    PersistenceManager pm = null;
+    try {
+      pm = openTransaction();
+      Query query = pm.newQuery(MSentryRole.class);
+      query.setFilter("this.roleName == t");
+      query.declareParameters("java.lang.String t");
+      query.setUnique(true);
+      MSentryRole role = (MSentryRole) query.execute(roleName);
+      if (role == null) {
+        throw new SentryNoSuchObjectException("Role: " + roleName);
+      } else {
+        query = pm.newQuery(MSentryGroup.class);
+        query.setFilter("this.groupName == t");
+        query.declareParameters("java.lang.String t");
+        query.setUnique(true);
+        List<MSentryGroup> groups = Lists.newArrayList();
+        for (TSentryGroup tGroup : groupNames) {
+          MSentryGroup group = (MSentryGroup) query.execute(tGroup.getGroupName());
+          if (group != null) {
+            group.removeRole(role);
+            groups.add(group);
+          }
+        }
+        pm.makePersistentAll(groups);
+        CommitContext commit = commitUpdateTransaction(pm);
+        rollbackTransaction = false;
+        return commit;
+      }
+    } finally {
+      if (rollbackTransaction) {
+        rollbackTransaction(pm);
+      }
+    }
   }
 
-  public TSentryRole getSentryRoleByName(String roleName)
+  @VisibleForTesting
+  MSentryRole getMSentryRoleByName(String roleName)
   throws SentryNoSuchObjectException {
     boolean rollbackTransaction = true;
     PersistenceManager pm = null;
@@ -311,7 +394,7 @@ public class SentryStore {
       }
       rollbackTransaction = false;
       commitTransaction(pm);
-      return convertToSentryRole(sentryRole);
+      return sentryRole;
     } finally {
       if (rollbackTransaction) {
         rollbackTransaction(pm);
@@ -319,6 +402,11 @@ public class SentryStore {
     }
   }
 
+  public TSentryRole getSentryRoleByName(String roleName)
+  throws SentryNoSuchObjectException {
+    return convertToSentryRole(getMSentryRoleByName(roleName));
+  }
+
   private MSentryRole convertToMSentryRole(TSentryRole role) {
     MSentryRole mRole = new MSentryRole();
     mRole.setCreateTime(role.getCreateTime());
@@ -370,4 +458,4 @@ public class SentryStore {
     mSentryPrivilege.setPrivilegeName(privilege.getPrivilegeName());
     return mSentryPrivilege;
   }
-}
\ No newline at end of file
+}

http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/066f993e/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
----------------------------------------------------------------------
diff --git a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
index 78e0a87..ff4817f 100644
--- a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
+++ b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
@@ -40,6 +40,7 @@ import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Splitter;
 import com.google.common.collect.Lists;
+import com.google.common.collect.Sets;
 
 @SuppressWarnings("unused")
 public class SentryPolicyStoreProcessor implements SentryPolicyService.Iface {
@@ -100,7 +101,8 @@ public class SentryPolicyStoreProcessor implements SentryPolicyService.Iface {
   }
 
   //TODO:Validate privilege scope?
-  private String constructPrivilegeName(TSentryPrivilege privilege) throws SentryInvalidInputException {
+  @VisibleForTesting
+  public static String constructPrivilegeName(TSentryPrivilege privilege) throws SentryInvalidInputException {
     StringBuilder privilegeName = new StringBuilder();
     String serverName = privilege.getServerName();
     String dbName = privilege.getDbName();
@@ -219,6 +221,7 @@ public class SentryPolicyStoreProcessor implements SentryPolicyService.Iface {
       LOGGER.error(msg, e);
       response.setStatus(Status.RuntimeError(msg, e));
     }
+
     return response;
   }
 
@@ -249,7 +252,8 @@ public class SentryPolicyStoreProcessor implements SentryPolicyService.Iface {
     TAlterSentryRoleAddGroupsRequest request) throws TException {
     TAlterSentryRoleAddGroupsResponse response = new TAlterSentryRoleAddGroupsResponse();
     try {
-      CommitContext commitContext = sentryStore.alterSentryRoleAddGroups();
+      CommitContext commitContext = sentryStore.alterSentryRoleAddGroups(request.getUserName(),
+          request.getRoleName(), request.getGroups());
       response.setStatus(Status.OK());
       notificationHandlerInvoker.alter_sentry_role_add_groups(commitContext,
           request, response);
@@ -271,7 +275,7 @@ public class SentryPolicyStoreProcessor implements SentryPolicyService.Iface {
     // TODO implement
     TAlterSentryRoleDeleteGroupsResponse response = new TAlterSentryRoleDeleteGroupsResponse();
     try {
-      CommitContext commitContext = sentryStore.alterSentryRoleDeleteGroups();
+      CommitContext commitContext = sentryStore.alterSentryRoleDeleteGroups(null, null);
       response.setStatus(Status.OK());
       notificationHandlerInvoker.alter_sentry_role_delete_groups(commitContext,
           request, response);

http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/066f993e/sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift
----------------------------------------------------------------------
diff --git a/sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift b/sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift
index d6e05b7..7c54290 100644
--- a/sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift
+++ b/sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift
@@ -30,8 +30,8 @@ namespace php sentry.provider.db.service.thrift
 namespace cpp Apache.Sentry.Provider.Db.Service.Thrift
 
 struct TSentryPrivilege {
-1: required string privilegeScope,
-2: optional string privilegeName,
+1: required string privilegeScope, # Valid values are SERVER, DATABASE, TABLE
+2: optional string privilegeName, # Generated on server side
 3: required string serverName,
 4: optional string dbName,
 5: optional string tableName,
@@ -43,6 +43,8 @@ struct TSentryPrivilege {
 
 struct TSentryRole {
 1: required string roleName,
+# TODO privs should not be part of Sentry role as
+# they are created when a grant is executed
 2: required set<TSentryPrivilege> privileges,
 3: required i64 createTime,
 4: required string grantorPrincipal
@@ -136,4 +138,4 @@ service SentryPolicyService
 
   TListSentryRolesResponse list_sentry_roles_by_group(1:TListSentryRolesRequest request)
   TListSentryRolesResponse list_sentry_roles_by_role_name(1:TListSentryRolesRequest request) 
-}
\ No newline at end of file
+}

http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/066f993e/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
----------------------------------------------------------------------
diff --git a/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
new file mode 100644
index 0000000..be3d078
--- /dev/null
+++ b/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
@@ -0,0 +1,145 @@
+/**
+ * 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.sentry.provider.db.service.persistent;
+
+import static junit.framework.Assert.assertEquals;
+import static junit.framework.Assert.fail;
+
+import java.io.File;
+import java.util.Collections;
+import java.util.Set;
+
+import org.apache.commons.io.FileUtils;
+import org.apache.sentry.provider.db.service.model.MSentryPrivilege;
+import org.apache.sentry.provider.db.service.model.MSentryRole;
+import org.apache.sentry.provider.db.service.thrift.SentryPolicyStoreProcessor;
+import org.apache.sentry.provider.db.service.thrift.TSentryGroup;
+import org.apache.sentry.provider.db.service.thrift.TSentryPrivilege;
+import org.apache.sentry.provider.db.service.thrift.TSentryRole;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import com.google.common.collect.Iterables;
+import com.google.common.collect.Sets;
+import com.google.common.io.Files;
+
+public class TestSentryStore {
+
+  private static File dataDir;
+  private static SentryStore sentryStore;
+
+  @BeforeClass
+  public static void setup() throws Exception {
+    dataDir = new File(Files.createTempDir(), SentryStore.DEFAULT_DATA_DIR);
+    sentryStore = new SentryStore(dataDir.getPath());
+  }
+
+  @AfterClass
+  public static void teardown() {
+    if (sentryStore != null) {
+      sentryStore.stop();
+    }
+    if (dataDir != null) {
+      FileUtils.deleteQuietly(dataDir);
+    }
+  }
+
+  private static CommitContext createRole(String r, String g) throws Exception {
+    TSentryRole role = new TSentryRole();
+    role.setGrantorPrincipal(g);
+    role.setRoleName(r);
+    return sentryStore.createSentryRole(role);
+  }
+
+
+  @Test
+  public void testCreateDuplicateRole() throws Exception {
+    String roleName = "test-dup-role";
+    String grantor = "g1";
+    createRole(roleName, grantor);
+    try {
+      createRole(roleName, grantor);
+      fail("Expected SentryAlreadyExistsException");
+    } catch(SentryAlreadyExistsException e) {
+      // expected
+    }
+  }
+
+  @Test
+  public void testCreateDropRole() throws Exception {
+    String roleName = "test-drop-role";
+    String grantor = "g1";
+    long seqId = createRole(roleName, grantor).getSequenceId();
+    assertEquals(seqId + 1, sentryStore.dropSentryRole(roleName).getSequenceId());
+  }
+
+  @Test(expected = SentryNoSuchObjectException.class)
+  public void testAddDeleteGroupsNonExistantRole()
+      throws Exception {
+    String roleName = "non-existant-role";
+    String grantor = "g1";
+    Set<TSentryGroup> groups = Sets.newHashSet();
+    sentryStore.alterSentryRoleAddGroups(grantor, roleName, groups);
+  }
+
+  @Test
+  public void testAddDeleteGroups() throws Exception {
+    String roleName = "test-groups";
+    String grantor = "g1";
+    long seqId = createRole(roleName, grantor).getSequenceId();
+    Set<TSentryGroup> groups = Sets.newHashSet();
+    TSentryGroup group = new TSentryGroup();
+    group.setGroupName("test-groups-g1");
+    groups.add(group);
+    group = new TSentryGroup();
+    group.setGroupName("test-groups-g2");
+    groups.add(group);
+    assertEquals(seqId + 1, sentryStore.alterSentryRoleAddGroups(grantor,
+        roleName, groups).getSequenceId());
+    assertEquals(seqId + 2, sentryStore.alterSentryRoleDeleteGroups(roleName, groups)
+        .getSequenceId());
+    MSentryRole role = sentryStore.getMSentryRoleByName(roleName);
+    assertEquals(Collections.emptySet(), role.getGroups());
+  }
+
+  @Test
+  public void testGrantRevokePrivilege() throws Exception {
+    String roleName = "test-privilege";
+    String grantor = "g1";
+    long seqId = createRole(roleName, grantor).getSequenceId();
+    TSentryPrivilege privilege = new TSentryPrivilege();
+    privilege.setPrivilegeScope("TABLE");
+    privilege.setServerName("server1");
+    privilege.setDbName("db1");
+    privilege.setTableName("tbl1");
+    privilege.setAction("SELECT");
+    privilege.setGrantorPrincipal(grantor);
+    privilege.setCreateTime(System.currentTimeMillis());
+    privilege.setPrivilegeName(SentryPolicyStoreProcessor.constructPrivilegeName(privilege));
+    assertEquals(seqId + 1, sentryStore.alterSentryRoleGrantPrivilege(roleName, privilege)
+        .getSequenceId());
+    MSentryRole role = sentryStore.getMSentryRoleByName(roleName);
+    Set<MSentryPrivilege> privileges = role.getPrivileges();
+    assertEquals(privileges.toString(), 1, privileges.size());
+    assertEquals(privilege.getPrivilegeName(), Iterables.get(privileges, 0).getPrivilegeName());
+    assertEquals(seqId + 2, sentryStore.alterSentryRoleRevokePrivilege(roleName, privilege.getPrivilegeName())
+        .getSequenceId());
+  }
+}