You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cassandra.apache.org by al...@apache.org on 2015/03/11 10:44:52 UTC

cassandra git commit: Make syntax for Role options consistent with other statements

Repository: cassandra
Updated Branches:
  refs/heads/trunk 814bd325e -> 2f3fd416f


Make syntax for Role options consistent with other statements

patch by Sam Tunnicliffe; reviewed by Aleksey Yeschenko for
CASSANDRA-8850


Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo
Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/2f3fd416
Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/2f3fd416
Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/2f3fd416

Branch: refs/heads/trunk
Commit: 2f3fd416fa3b2d254a479f6339993713efdbcc16
Parents: 814bd32
Author: Sam Tunnicliffe <sa...@beobal.com>
Authored: Wed Mar 11 02:43:42 2015 -0700
Committer: Aleksey Yeschenko <al...@apache.org>
Committed: Wed Mar 11 02:43:42 2015 -0700

----------------------------------------------------------------------
 CHANGES.txt                                     |   2 +-
 pylib/cqlshlib/cql3handling.py                  |  11 +-
 .../cassandra/auth/CassandraRoleManager.java    |  56 +++--
 .../org/apache/cassandra/auth/IRoleManager.java |   4 +-
 .../org/apache/cassandra/auth/RoleOptions.java  | 153 +++++++++++++
 src/java/org/apache/cassandra/cql3/Cql.g        |  62 +++--
 .../org/apache/cassandra/cql3/RoleOptions.java  |  62 -----
 .../cql3/statements/AlterRoleStatement.java     |  11 +-
 .../cql3/statements/CreateRoleStatement.java    |  18 +-
 .../apache/cassandra/auth/RoleOptionsTest.java  | 227 +++++++++++++++++++
 .../org/apache/cassandra/cql3/CQLTester.java    |  28 ++-
 .../apache/cassandra/cql3/RoleSyntaxTest.java   |  51 +++++
 12 files changed, 535 insertions(+), 150 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/cassandra/blob/2f3fd416/CHANGES.txt
----------------------------------------------------------------------
diff --git a/CHANGES.txt b/CHANGES.txt
index d6ba737..0148925 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -1,4 +1,5 @@
 3.0
+ * Add role based access control (CASSANDRA-7653, 8650, 7216, 8760, 8849, 8761, 8850)
  * Record client ip address in tracing sessions (CASSANDRA-8162)
  * Indicate partition key columns in response metadata for prepared
    statements (CASSANDRA-7660)
@@ -7,7 +8,6 @@
  * Optimise (Time)?UUIDType Comparisons (CASSANDRA-8730)
  * Make CRC32Ex into a separate maven dependency (CASSANDRA-8836)
  * Use preloaded jemalloc w/ Unsafe (CASSANDRA-8714)
- * Add role based access control (CASSANDRA-7653, 8650, 7216, 8760, 8849, 8761)
  * Avoid accessing partitioner through StorageProxy (CASSANDRA-8244, 8268)
  * Upgrade Metrics library and remove depricated metrics (CASSANDRA-5657)
  * Serializing Row cache alternative, fully off heap (CASSANDRA-7438)

http://git-wip-us.apache.org/repos/asf/cassandra/blob/2f3fd416/pylib/cqlshlib/cql3handling.py
----------------------------------------------------------------------
diff --git a/pylib/cqlshlib/cql3handling.py b/pylib/cqlshlib/cql3handling.py
index 0eb7a3b..592daad 100644
--- a/pylib/cqlshlib/cql3handling.py
+++ b/pylib/cqlshlib/cql3handling.py
@@ -1181,17 +1181,16 @@ syntax_rules += r'''
 
 <createRoleStatement> ::= "CREATE" "ROLE" <rolename>
                               ( "WITH" <roleProperty> ("AND" <roleProperty>)*)?
-                              ( "SUPERUSER" | "NOSUPERUSER" )?
-                              ( "LOGIN" | "NOLOGIN" )?
                         ;
 
 <alterRoleStatement> ::= "ALTER" "ROLE" <rolename>
                               ( "WITH" <roleProperty> ("AND" <roleProperty>)*)?
-                              ( "SUPERUSER" | "NOSUPERUSER" )?
-                              ( "LOGIN" | "NOLOGIN" )?
                        ;
-<roleProperty> ::= "PASSWORD" "="? <stringLiteral>
-                 | "OPTIONS" "="? <mapLiteral>
+
+<roleProperty> ::= "PASSWORD" "=" <stringLiteral>
+                 | "OPTIONS" "=" <mapLiteral>
+                 | "SUPERUSER" "=" <boolean>
+                 | "LOGIN" "=" <boolean>
                  ;
 
 <dropRoleStatement> ::= "DROP" "ROLE" <rolename>

http://git-wip-us.apache.org/repos/asf/cassandra/blob/2f3fd416/src/java/org/apache/cassandra/auth/CassandraRoleManager.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/auth/CassandraRoleManager.java b/src/java/org/apache/cassandra/auth/CassandraRoleManager.java
index 36c57d4..05a4fc8 100644
--- a/src/java/org/apache/cassandra/auth/CassandraRoleManager.java
+++ b/src/java/org/apache/cassandra/auth/CassandraRoleManager.java
@@ -115,11 +115,11 @@ public class CassandraRoleManager implements IRoleManager
     public CassandraRoleManager()
     {
         supportedOptions = DatabaseDescriptor.getAuthenticator().getClass() == PasswordAuthenticator.class
-                           ? ImmutableSet.of(Option.LOGIN, Option.SUPERUSER, Option.PASSWORD)
-                           : ImmutableSet.of(Option.LOGIN, Option.SUPERUSER);
+                         ? ImmutableSet.of(Option.LOGIN, Option.SUPERUSER, Option.PASSWORD)
+                         : ImmutableSet.of(Option.LOGIN, Option.SUPERUSER);
         alterableOptions = DatabaseDescriptor.getAuthenticator().getClass().equals(PasswordAuthenticator.class)
-                           ? ImmutableSet.of(Option.PASSWORD)
-                           : ImmutableSet.<Option>of();
+                         ? ImmutableSet.of(Option.PASSWORD)
+                         : ImmutableSet.<Option>of();
     }
 
     public void setup()
@@ -135,8 +135,8 @@ public class CassandraRoleManager implements IRoleManager
         if (Schema.instance.getCFMetaData(AuthKeyspace.NAME, "users") != null)
         {
              legacySelectUserStatement = (SelectStatement) prepare("SELECT * FROM %s.%s WHERE name = ?",
-                                                             AuthKeyspace.NAME,
-                                                             LEGACY_USERS_TABLE);
+                                                                   AuthKeyspace.NAME,
+                                                                   LEGACY_USERS_TABLE);
             scheduleSetupTask(new Runnable()
             {
                 public void run()
@@ -167,23 +167,23 @@ public class CassandraRoleManager implements IRoleManager
         return alterableOptions;
     }
 
-    public void createRole(AuthenticatedUser performer, RoleResource role, Map<Option, Object> options)
+    public void createRole(AuthenticatedUser performer, RoleResource role, RoleOptions options)
     throws RequestValidationException, RequestExecutionException
     {
-        String insertCql = options.containsKey(Option.PASSWORD)
-                            ? String.format("INSERT INTO %s.%s (role, is_superuser, can_login, salted_hash) VALUES ('%s', %s, %s, '%s')",
-                                            AuthKeyspace.NAME,
-                                            AuthKeyspace.ROLES,
-                                            escape(role.getRoleName()),
-                                            options.get(Option.SUPERUSER),
-                                            options.get(Option.LOGIN),
-                                            escape(hashpw(options.get(Option.PASSWORD).toString())))
-                            : String.format("INSERT INTO %s.%s (role, is_superuser, can_login) VALUES ('%s', %s, %s)",
-                                            AuthKeyspace.NAME,
-                                            AuthKeyspace.ROLES,
-                                            escape(role.getRoleName()),
-                                            options.get(Option.SUPERUSER),
-                                            options.get(Option.LOGIN));
+        String insertCql = options.getPassword().isPresent()
+                         ? String.format("INSERT INTO %s.%s (role, is_superuser, can_login, salted_hash) VALUES ('%s', %s, %s, '%s')",
+                                         AuthKeyspace.NAME,
+                                         AuthKeyspace.ROLES,
+                                         escape(role.getRoleName()),
+                                         options.getSuperuser().get(),
+                                         options.getLogin().get(),
+                                         escape(hashpw(options.getPassword().get())))
+                         : String.format("INSERT INTO %s.%s (role, is_superuser, can_login) VALUES ('%s', %s, %s)",
+                                         AuthKeyspace.NAME,
+                                         AuthKeyspace.ROLES,
+                                         escape(role.getRoleName()),
+                                         options.getSuperuser().get(),
+                                         options.getLogin().get());
         process(insertCql, consistencyForRole(role.getRoleName()));
     }
 
@@ -197,14 +197,12 @@ public class CassandraRoleManager implements IRoleManager
         removeAllMembers(role.getRoleName());
     }
 
-    public void alterRole(AuthenticatedUser performer, RoleResource role, Map<Option, Object> options)
-    throws RequestValidationException, RequestExecutionException
+    public void alterRole(AuthenticatedUser performer, RoleResource role, RoleOptions options)
     {
         // Unlike most of the other data access methods here, this does not use a
         // prepared statement in order to allow the set of assignments to be variable.
-        String assignments = Joiner.on(',')
-                                   .join(Iterables.filter(optionsToAssignments(options),
-                                                          Predicates.notNull()));
+        String assignments = Joiner.on(',').join(Iterables.filter(optionsToAssignments(options.getOptions()),
+                                                                  Predicates.notNull()));
         if (!Strings.isNullOrEmpty(assignments))
         {
             QueryProcessor.process(String.format("UPDATE %s.%s SET %s WHERE role = '%s'",
@@ -372,9 +370,9 @@ public class CassandraRoleManager implements IRoleManager
                                                                 ConsistencyLevel.QUORUM);
                 for (UntypedResultSet.Row row : users)
                 {
-                    Map<Option, Object> options = new HashMap<>();
-                    options.put(Option.SUPERUSER, row.getBoolean("super"));
-                    options.put(Option.LOGIN, true);
+                    RoleOptions options = new RoleOptions();
+                    options.setOption(Option.SUPERUSER, row.getBoolean("super"));
+                    options.setOption(Option.LOGIN, true);
                     createRole(null, RoleResource.role(row.getString("name")), options);
                 }
                 logger.info("Completed conversion of legacy users");

http://git-wip-us.apache.org/repos/asf/cassandra/blob/2f3fd416/src/java/org/apache/cassandra/auth/IRoleManager.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/auth/IRoleManager.java b/src/java/org/apache/cassandra/auth/IRoleManager.java
index 5205dad..a8befde 100644
--- a/src/java/org/apache/cassandra/auth/IRoleManager.java
+++ b/src/java/org/apache/cassandra/auth/IRoleManager.java
@@ -65,7 +65,7 @@ public interface IRoleManager
      * @throws RequestValidationException
      * @throws RequestExecutionException
      */
-    void createRole(AuthenticatedUser performer, RoleResource role, Map<Option, Object> options)
+    void createRole(AuthenticatedUser performer, RoleResource role, RoleOptions options)
     throws RequestValidationException, RequestExecutionException;
 
     /**
@@ -91,7 +91,7 @@ public interface IRoleManager
      * @throws RequestValidationException
      * @throws RequestExecutionException
      */
-    void alterRole(AuthenticatedUser performer, RoleResource role, Map<Option, Object> options)
+    void alterRole(AuthenticatedUser performer, RoleResource role, RoleOptions options)
     throws RequestValidationException, RequestExecutionException;
 
     /**

http://git-wip-us.apache.org/repos/asf/cassandra/blob/2f3fd416/src/java/org/apache/cassandra/auth/RoleOptions.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/auth/RoleOptions.java b/src/java/org/apache/cassandra/auth/RoleOptions.java
new file mode 100644
index 0000000..9609ff3
--- /dev/null
+++ b/src/java/org/apache/cassandra/auth/RoleOptions.java
@@ -0,0 +1,153 @@
+/*
+ * 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.cassandra.auth;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import com.google.common.base.Optional;
+
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.exceptions.InvalidRequestException;
+import org.apache.cassandra.exceptions.SyntaxException;
+import org.apache.cassandra.utils.FBUtilities;
+
+public class RoleOptions
+{
+    private final Map<IRoleManager.Option, Object> options = new HashMap<>();
+
+    /**
+     * Set a value for a specific option.
+     * Throws SyntaxException if the same option is set multiple times
+     * @param option
+     * @param value
+     */
+    public void setOption(IRoleManager.Option option, Object value)
+    {
+        if (options.containsKey(option))
+            throw new SyntaxException(String.format("Multiple definition for property '%s'", option.name()));
+        options.put(option, value);
+    }
+
+    /**
+     * Return true if there are no options with values set, false otherwise
+     * @return whether any options have values set or not
+     */
+    public boolean isEmpty()
+    {
+        return options.isEmpty();
+    }
+
+    /**
+     * Return a map of all the options which have been set
+     * @return all options with values
+     */
+    public Map<IRoleManager.Option, Object> getOptions()
+    {
+        return options;
+    }
+
+    /**
+     * Return a boolean value of the superuser option
+     * @return superuser option value
+     */
+    public Optional<Boolean> getSuperuser()
+    {
+        return Optional.fromNullable((Boolean)options.get(IRoleManager.Option.SUPERUSER));
+    }
+
+    /**
+     * Return a boolean value of the login option
+     * @return login option value
+     */
+    public Optional<Boolean> getLogin()
+    {
+        return Optional.fromNullable((Boolean)options.get(IRoleManager.Option.LOGIN));
+    }
+
+    /**
+     * Return the string value of the password option
+     * @return password option value
+     */
+    public Optional<String> getPassword()
+    {
+        return Optional.fromNullable((String)options.get(IRoleManager.Option.PASSWORD));
+    }
+
+    /**
+     * Return a Map<String, String> representing custom options
+     * It is the responsiblity of IRoleManager implementations which support
+     * IRoleManager.Option.OPTION to handle type checking and conversion of these
+     * values, if present
+     * @return map of custom options
+     */
+    @SuppressWarnings("unchecked")
+    public Optional<Map<String, String>> getCustomOptions()
+    {
+        return Optional.fromNullable((Map<String, String>)options.get(IRoleManager.Option.OPTIONS));
+    }
+
+    /**
+     * Validate the contents of the options in two ways:
+     * - Ensure that only a subset of the options supported by the configured IRoleManager are set
+     * - Validate the type of any option values present.
+     * Should either condition fail, then InvalidRequestException is thrown. This method is called
+     * during validation of CQL statements, so the IRE results in a error response to the client.
+     *
+     * @throws InvalidRequestException if any options which are not supported by the configured IRoleManager
+     *     are set or if any option value is of an incorrect type.
+     */
+    public void validate()
+    {
+        for (Map.Entry<IRoleManager.Option, Object> option : options.entrySet())
+        {
+            if (!DatabaseDescriptor.getRoleManager().supportedOptions().contains(option.getKey()))
+                throw new InvalidRequestException(String.format("%s doesn't support %s",
+                                                                DatabaseDescriptor.getRoleManager().getClass().getName(),
+                                                                option.getKey()));
+            switch (option.getKey())
+            {
+                case LOGIN:
+                case SUPERUSER:
+                    if (!(option.getValue() instanceof Boolean))
+                        throw new InvalidRequestException(String.format("Invalid value for property '%s'. " +
+                                                                        "It must be a boolean",
+                                                                        option.getKey()));
+                    break;
+                case PASSWORD:
+                    if (!(option.getValue() instanceof String))
+                        throw new InvalidRequestException(String.format("Invalid value for property '%s'. " +
+                                                                        "It must be a string",
+                                                                        option.getKey()));
+                    break;
+                case OPTIONS:
+                    if (!(option.getValue() instanceof Map))
+                        throw new InvalidRequestException(String.format("Invalid value for property '%s'. " +
+                                                                        "It must be a map",
+                                                                        option.getKey()));
+                    break;
+
+            }
+        }
+    }
+
+    public String toString()
+    {
+        return FBUtilities.toString(options);
+    }
+}

http://git-wip-us.apache.org/repos/asf/cassandra/blob/2f3fd416/src/java/org/apache/cassandra/cql3/Cql.g
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/cql3/Cql.g b/src/java/org/apache/cassandra/cql3/Cql.g
index d565258..2ca6b5d 100644
--- a/src/java/org/apache/cassandra/cql3/Cql.g
+++ b/src/java/org/apache/cassandra/cql3/Cql.g
@@ -914,15 +914,15 @@ roleResource  returns [RoleResource res]
 createUserStatement returns [CreateRoleStatement stmt]
     @init {
         RoleOptions opts = new RoleOptions();
-        opts.put(IRoleManager.Option.LOGIN.name(), true);
+        opts.setOption(IRoleManager.Option.LOGIN, true);
         boolean superuser = false;
         boolean ifNotExists = false;
         RoleName name = new RoleName();
     }
     : K_CREATE K_USER (K_IF K_NOT K_EXISTS { ifNotExists = true; })? u=username { name.setName($u.text, false); }
-      ( K_WITH roleOptions[opts] )?
+      ( K_WITH userPassword[opts] )?
       ( K_SUPERUSER { superuser = true; } | K_NOSUPERUSER { superuser = false; } )?
-      { opts.put(IRoleManager.Option.SUPERUSER.name(), superuser);
+      { opts.setOption(IRoleManager.Option.SUPERUSER, superuser);
         $stmt = new CreateRoleStatement(name, opts, ifNotExists); }
     ;
 
@@ -935,9 +935,9 @@ alterUserStatement returns [AlterRoleStatement stmt]
         RoleName name = new RoleName();
     }
     : K_ALTER K_USER u=username { name.setName($u.text, false); }
-      ( K_WITH roleOptions[opts] )?
-      ( K_SUPERUSER { opts.put(IRoleManager.Option.SUPERUSER.name(), true); }
-        | K_NOSUPERUSER { opts.put(IRoleManager.Option.SUPERUSER.name(), false); } ) ?
+      ( K_WITH userPassword[opts] )?
+      ( K_SUPERUSER { opts.setOption(IRoleManager.Option.SUPERUSER, true); }
+        | K_NOSUPERUSER { opts.setOption(IRoleManager.Option.SUPERUSER, false); } ) ?
       {  $stmt = new AlterRoleStatement(name, opts); }
     ;
 
@@ -960,26 +960,43 @@ listUsersStatement returns [ListRolesStatement stmt]
     ;
 
 /**
- * CREATE ROLE [IF NOT EXISTS] <rolename> [WITH PASSWORD <password>] [SUPERUSER|NOSUPERUSER] [LOGIN|NOLOGIN]
+ * CREATE ROLE [IF NOT EXISTS] <rolename> [ [WITH] option [ [AND] option ]* ]
+ *
+ * where option can be:
+ *  PASSWORD = '<password>'
+ *  SUPERUSER = (true|false)
+ *  LOGIN = (true|false)
+ *  OPTIONS = { 'k1':'v1', 'k2':'v2'}
  */
 createRoleStatement returns [CreateRoleStatement stmt]
     @init {
         RoleOptions opts = new RoleOptions();
-        boolean superuser = false;
-        boolean login = false;
         boolean ifNotExists = false;
     }
     : K_CREATE K_ROLE (K_IF K_NOT K_EXISTS { ifNotExists = true; })? name=userOrRoleName
       ( K_WITH roleOptions[opts] )?
-      ( K_SUPERUSER { superuser = true; } | K_NOSUPERUSER { superuser = false; } )?
-      ( K_LOGIN { login = true; } | K_NOLOGIN { login = false; } )?
-      { opts.put(IRoleManager.Option.SUPERUSER.name(), superuser);
-        opts.put(IRoleManager.Option.LOGIN.name(), login);
-        $stmt = new CreateRoleStatement(name, opts, ifNotExists); }
+      {
+        // set defaults if they weren't explictly supplied
+        if (!opts.getLogin().isPresent())
+        {
+            opts.setOption(IRoleManager.Option.LOGIN, false);
+        }
+        if (!opts.getSuperuser().isPresent())
+        {
+            opts.setOption(IRoleManager.Option.SUPERUSER, false);
+        }
+        $stmt = new CreateRoleStatement(name, opts, ifNotExists);
+      }
     ;
 
 /**
- * ALTER ROLE <rolename> [WITH PASSWORD <password>] [SUPERUSER|NOSUPERUSER]
+ * ALTER ROLE <rolename> [ [WITH] option [ [AND] option ]* ]
+ *
+ * where option can be:
+ *  PASSWORD = '<password>'
+ *  SUPERUSER = (true|false)
+ *  LOGIN = (true|false)
+ *  OPTIONS = { 'k1':'v1', 'k2':'v2'}
  */
 alterRoleStatement returns [AlterRoleStatement stmt]
     @init {
@@ -987,10 +1004,6 @@ alterRoleStatement returns [AlterRoleStatement stmt]
     }
     : K_ALTER K_ROLE name=userOrRoleName
       ( K_WITH roleOptions[opts] )?
-      ( K_SUPERUSER { opts.put(IRoleManager.Option.SUPERUSER.name(), true); }
-        | K_NOSUPERUSER { opts.put(IRoleManager.Option.SUPERUSER.name(), false); } ) ?
-      ( K_LOGIN { opts.put(IRoleManager.Option.LOGIN.name(), true); }
-        | K_NOLOGIN { opts.put(IRoleManager.Option.LOGIN.name(), false); } )?
       {  $stmt = new AlterRoleStatement(name, opts); }
     ;
 
@@ -1024,8 +1037,15 @@ roleOptions[RoleOptions opts]
     ;
 
 roleOption[RoleOptions opts]
-    :  k=K_PASSWORD '='? v=STRING_LITERAL { opts.put($k.text, $v.text); }
-    |  k=K_OPTIONS '='? m=mapLiteral { opts.put(IRoleManager.Option.OPTIONS.name(), convertPropertyMap(m)); }
+    :  K_PASSWORD '=' v=STRING_LITERAL { opts.setOption(IRoleManager.Option.PASSWORD, $v.text); }
+    |  K_OPTIONS '=' m=mapLiteral { opts.setOption(IRoleManager.Option.OPTIONS, convertPropertyMap(m)); }
+    |  K_SUPERUSER '=' b=BOOLEAN { opts.setOption(IRoleManager.Option.SUPERUSER, Boolean.valueOf($b.text)); }
+    |  K_LOGIN '=' b=BOOLEAN { opts.setOption(IRoleManager.Option.LOGIN, Boolean.valueOf($b.text)); }
+    ;
+
+// for backwards compatibility in CREATE/ALTER USER, this has no '='
+userPassword[RoleOptions opts]
+    :  K_PASSWORD v=STRING_LITERAL { opts.setOption(IRoleManager.Option.PASSWORD, $v.text); }
     ;
 
 /** DEFINITIONS **/

http://git-wip-us.apache.org/repos/asf/cassandra/blob/2f3fd416/src/java/org/apache/cassandra/cql3/RoleOptions.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/cql3/RoleOptions.java b/src/java/org/apache/cassandra/cql3/RoleOptions.java
deleted file mode 100644
index 89f37dd..0000000
--- a/src/java/org/apache/cassandra/cql3/RoleOptions.java
+++ /dev/null
@@ -1,62 +0,0 @@
-/*
- * 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.cassandra.cql3;
-
-import java.util.HashMap;
-import java.util.Map;
-
-import org.apache.cassandra.auth.IRoleManager;
-import org.apache.cassandra.config.DatabaseDescriptor;
-import org.apache.cassandra.exceptions.InvalidRequestException;
-import org.apache.cassandra.utils.FBUtilities;
-
-public class RoleOptions
-{
-    private final Map<IRoleManager.Option, Object> options = new HashMap<>();
-
-    public void put(String name, Object value)
-    {
-        options.put(IRoleManager.Option.valueOf(name.toUpperCase()), value);
-    }
-
-    public boolean isEmpty()
-    {
-        return options.isEmpty();
-    }
-
-    public Map<IRoleManager.Option, Object> getOptions()
-    {
-        return options;
-    }
-
-    public void validate() throws InvalidRequestException
-    {
-        for (IRoleManager.Option option : options.keySet())
-        {
-            if (!DatabaseDescriptor.getRoleManager().supportedOptions().contains(option))
-                throw new InvalidRequestException(String.format("%s doesn't support %s",
-                                                                DatabaseDescriptor.getRoleManager().getClass().getName(),
-                                                                option));
-        }
-    }
-
-    public String toString()
-    {
-        return FBUtilities.toString(options);
-    }
-}

http://git-wip-us.apache.org/repos/asf/cassandra/blob/2f3fd416/src/java/org/apache/cassandra/cql3/statements/AlterRoleStatement.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/cql3/statements/AlterRoleStatement.java b/src/java/org/apache/cassandra/cql3/statements/AlterRoleStatement.java
index 494ab19..6134741 100644
--- a/src/java/org/apache/cassandra/cql3/statements/AlterRoleStatement.java
+++ b/src/java/org/apache/cassandra/cql3/statements/AlterRoleStatement.java
@@ -17,13 +17,10 @@
  */
 package org.apache.cassandra.cql3.statements;
 
-import org.apache.cassandra.auth.AuthenticatedUser;
+import org.apache.cassandra.auth.*;
 import org.apache.cassandra.auth.IRoleManager.Option;
-import org.apache.cassandra.auth.Permission;
-import org.apache.cassandra.auth.RoleResource;
 import org.apache.cassandra.config.DatabaseDescriptor;
 import org.apache.cassandra.cql3.RoleName;
-import org.apache.cassandra.cql3.RoleOptions;
 import org.apache.cassandra.exceptions.*;
 import org.apache.cassandra.service.ClientState;
 import org.apache.cassandra.transport.messages.ResultMessage;
@@ -57,11 +54,11 @@ public class AlterRoleStatement extends AuthenticationStatement
         AuthenticatedUser user = state.getUser();
         boolean isSuper = user.isSuper();
 
-        if (opts.getOptions().containsKey(Option.SUPERUSER) && user.getRoles().contains(role))
+        if (opts.getSuperuser().isPresent() && user.getRoles().contains(role))
             throw new UnauthorizedException("You aren't allowed to alter your own superuser " +
                                             "status or that of a role granted to you");
 
-        if (opts.getOptions().containsKey(Option.SUPERUSER) && !isSuper)
+        if (opts.getSuperuser().isPresent() && !isSuper)
             throw new UnauthorizedException("Only superusers are allowed to alter superuser status");
 
         // superusers can do whatever else they like
@@ -87,7 +84,7 @@ public class AlterRoleStatement extends AuthenticationStatement
     public ResultMessage execute(ClientState state) throws RequestValidationException, RequestExecutionException
     {
         if (!opts.isEmpty())
-            DatabaseDescriptor.getRoleManager().alterRole(state.getUser(), role, opts.getOptions());
+            DatabaseDescriptor.getRoleManager().alterRole(state.getUser(), role, opts);
         return null;
     }
 }

http://git-wip-us.apache.org/repos/asf/cassandra/blob/2f3fd416/src/java/org/apache/cassandra/cql3/statements/CreateRoleStatement.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/cql3/statements/CreateRoleStatement.java b/src/java/org/apache/cassandra/cql3/statements/CreateRoleStatement.java
index 347d20a..9be4c89 100644
--- a/src/java/org/apache/cassandra/cql3/statements/CreateRoleStatement.java
+++ b/src/java/org/apache/cassandra/cql3/statements/CreateRoleStatement.java
@@ -17,13 +17,9 @@
  */
 package org.apache.cassandra.cql3.statements;
 
-import org.apache.cassandra.auth.AuthenticatedUser;
-import org.apache.cassandra.auth.IRoleManager.Option;
-import org.apache.cassandra.auth.Permission;
-import org.apache.cassandra.auth.RoleResource;
+import org.apache.cassandra.auth.*;
 import org.apache.cassandra.config.DatabaseDescriptor;
 import org.apache.cassandra.cql3.RoleName;
-import org.apache.cassandra.cql3.RoleOptions;
 import org.apache.cassandra.exceptions.*;
 import org.apache.cassandra.service.ClientState;
 import org.apache.cassandra.transport.messages.ResultMessage;
@@ -44,9 +40,9 @@ public class CreateRoleStatement extends AuthenticationStatement
     public void checkAccess(ClientState state) throws UnauthorizedException
     {
         super.checkPermission(state, Permission.CREATE, RoleResource.root());
-        if (opts.getOptions().containsKey(Option.SUPERUSER))
+        if (opts.getSuperuser().isPresent())
         {
-            if ((Boolean)opts.getOptions().get(Option.SUPERUSER) && !state.getUser().isSuper())
+            if (opts.getSuperuser().get() && !state.getUser().isSuper())
                 throw new UnauthorizedException("Only superusers can create a role with superuser status");
         }
     }
@@ -63,12 +59,6 @@ public class CreateRoleStatement extends AuthenticationStatement
 
         if (!ifNotExists && DatabaseDescriptor.getRoleManager().isExistingRole(role))
             throw new InvalidRequestException(String.format("%s already exists", role.getRoleName()));
-
-        for (Option option : opts.getOptions().keySet())
-        {
-            if (!DatabaseDescriptor.getRoleManager().supportedOptions().contains(option))
-                throw new UnauthorizedException(String.format("You aren't allowed to alter %s", option));
-        }
     }
 
     public ResultMessage execute(ClientState state) throws RequestExecutionException, RequestValidationException
@@ -77,7 +67,7 @@ public class CreateRoleStatement extends AuthenticationStatement
         if (ifNotExists && DatabaseDescriptor.getRoleManager().isExistingRole(role))
             return null;
 
-        DatabaseDescriptor.getRoleManager().createRole(state.getUser(), role, opts.getOptions());
+        DatabaseDescriptor.getRoleManager().createRole(state.getUser(), role, opts);
         grantPermissionsToCreator(state);
         return null;
     }

http://git-wip-us.apache.org/repos/asf/cassandra/blob/2f3fd416/test/unit/org/apache/cassandra/auth/RoleOptionsTest.java
----------------------------------------------------------------------
diff --git a/test/unit/org/apache/cassandra/auth/RoleOptionsTest.java b/test/unit/org/apache/cassandra/auth/RoleOptionsTest.java
new file mode 100644
index 0000000..71f0c97
--- /dev/null
+++ b/test/unit/org/apache/cassandra/auth/RoleOptionsTest.java
@@ -0,0 +1,227 @@
+/*
+ * 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.cassandra.auth;
+
+import java.lang.reflect.Field;
+import java.util.*;
+
+import com.google.common.collect.ImmutableSet;
+import org.junit.Test;
+
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.exceptions.*;
+import org.apache.cassandra.utils.FBUtilities;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+public class RoleOptionsTest
+{
+    @Test
+    public void validateValueTypes()
+    {
+        setupRoleManager(getRoleManager(IRoleManager.Option.values()));
+
+        RoleOptions opts = new RoleOptions();
+        opts.setOption(IRoleManager.Option.LOGIN, "test");
+        assertInvalidOptions(opts, "Invalid value for property 'LOGIN'. It must be a boolean");
+
+        opts = new RoleOptions();
+        opts.setOption(IRoleManager.Option.PASSWORD, 99);
+        assertInvalidOptions(opts, "Invalid value for property 'PASSWORD'. It must be a string");
+
+        opts = new RoleOptions();
+        opts.setOption(IRoleManager.Option.SUPERUSER, new HashSet<>());
+        assertInvalidOptions(opts, "Invalid value for property 'SUPERUSER'. It must be a boolean");
+
+        opts = new RoleOptions();
+        opts.setOption(IRoleManager.Option.OPTIONS, false);
+        assertInvalidOptions(opts, "Invalid value for property 'OPTIONS'. It must be a map");
+
+        opts = new RoleOptions();
+        opts.setOption(IRoleManager.Option.LOGIN, true);
+        opts.setOption(IRoleManager.Option.SUPERUSER, false);
+        opts.setOption(IRoleManager.Option.PASSWORD, "test");
+        opts.setOption(IRoleManager.Option.OPTIONS, Collections.singletonMap("key", "value"));
+        opts.validate();
+    }
+
+    @Test
+    public void rejectUnsupportedOptions()
+    {
+        // Our hypothetical IRoleManager only supports the LOGIN option
+        IRoleManager roleManager = getRoleManager(IRoleManager.Option.LOGIN);
+        setupRoleManager(roleManager);
+        RoleOptions opts = new RoleOptions();
+        opts.setOption(IRoleManager.Option.PASSWORD, "test");
+        assertInvalidOptions(opts, String.format("%s doesn't support PASSWORD", roleManager.getClass().getName()));
+    }
+
+    @Test
+    public void rejectSettingSameOptionMultipleTimes()
+    {
+        RoleOptions opts = new RoleOptions();
+        opts.setOption(IRoleManager.Option.LOGIN, true);
+        try
+        {
+            opts.setOption(IRoleManager.Option.LOGIN, false);
+        }
+        catch (SyntaxException e)
+        {
+            assertEquals("Multiple definition for property 'LOGIN'", e.getMessage());
+        }
+    }
+
+    @Test
+    public void emptyByDefault()
+    {
+        RoleOptions opts = new RoleOptions();
+        assertTrue(opts.isEmpty());
+        assertFalse(opts.getLogin().isPresent());
+
+        opts.setOption(IRoleManager.Option.LOGIN, true);
+        assertFalse(opts.isEmpty());
+        assertTrue(opts.getLogin().isPresent());
+        assertTrue(opts.getLogin().get());
+    }
+
+    private void assertInvalidOptions(RoleOptions opts, String message)
+    {
+        try
+        {
+            opts.validate();
+            fail("Expected error but didn't get one");
+        }
+        catch (InvalidRequestException e)
+        {
+            assertTrue(e.getMessage().equals(message));
+        }
+    }
+
+    private void setupRoleManager(IRoleManager manager)
+    {
+        Field field = FBUtilities.getProtectedField(DatabaseDescriptor.class, "roleManager");
+        field.setAccessible(true);
+        try
+        {
+            field.set(null, manager);
+        }
+        catch (IllegalAccessException e)
+        {
+            fail("Error setting IRoleManager instance for test");
+        }
+    }
+
+    private IRoleManager getRoleManager(final IRoleManager.Option...supportedOptions)
+    {
+        return new IRoleManager()
+        {
+            public Set<Option> supportedOptions()
+            {
+                return ImmutableSet.copyOf(supportedOptions);
+            }
+
+            public Set<Option> alterableOptions()
+            {
+                return null;
+            }
+
+            public void createRole(AuthenticatedUser performer,
+                                   RoleResource role,
+                                   RoleOptions options) throws RequestValidationException, RequestExecutionException
+            {
+
+            }
+
+            public void dropRole(AuthenticatedUser performer,
+                                 RoleResource role) throws RequestValidationException, RequestExecutionException
+            {
+
+            }
+
+            public void alterRole(AuthenticatedUser performer,
+                                  RoleResource role,
+                                  RoleOptions options) throws RequestValidationException, RequestExecutionException
+            {
+
+            }
+
+            public void grantRole(AuthenticatedUser performer,
+                                  RoleResource role,
+                                  RoleResource grantee) throws RequestValidationException, RequestExecutionException
+            {
+
+            }
+
+            public void revokeRole(AuthenticatedUser performer,
+                                   RoleResource role,
+                                   RoleResource revokee) throws RequestValidationException, RequestExecutionException
+            {
+
+            }
+
+            public Set<RoleResource> getRoles(RoleResource grantee,
+                                              boolean includeInherited) throws RequestValidationException, RequestExecutionException
+            {
+                return null;
+            }
+
+            public Set<RoleResource> getAllRoles() throws RequestValidationException, RequestExecutionException
+            {
+                return null;
+            }
+
+            public boolean isSuper(RoleResource role)
+            {
+                return false;
+            }
+
+            public boolean canLogin(RoleResource role)
+            {
+                return false;
+            }
+
+            public Map<String, String> getCustomOptions(RoleResource role)
+            {
+                return Collections.EMPTY_MAP;
+            }
+
+            public boolean isExistingRole(RoleResource role)
+            {
+                return false;
+            }
+
+            public Set<? extends IResource> protectedResources()
+            {
+                return null;
+            }
+
+            public void validateConfiguration() throws ConfigurationException
+            {
+
+            }
+
+            public void setup()
+            {
+
+            }
+        };
+    }
+}

http://git-wip-us.apache.org/repos/asf/cassandra/blob/2f3fd416/test/unit/org/apache/cassandra/cql3/CQLTester.java
----------------------------------------------------------------------
diff --git a/test/unit/org/apache/cassandra/cql3/CQLTester.java b/test/unit/org/apache/cassandra/cql3/CQLTester.java
index 1baebb6..831a8d7 100644
--- a/test/unit/org/apache/cassandra/cql3/CQLTester.java
+++ b/test/unit/org/apache/cassandra/cql3/CQLTester.java
@@ -31,27 +31,26 @@ import java.util.concurrent.atomic.AtomicInteger;
 
 import com.google.common.base.Objects;
 import com.google.common.collect.ImmutableSet;
-import org.junit.AfterClass;
-import org.junit.After;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.BeforeClass;
+import org.junit.*;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import com.datastax.driver.core.*;
 import com.datastax.driver.core.ResultSet;
-import com.datastax.driver.core.Row;
 import org.apache.cassandra.SchemaLoader;
 import org.apache.cassandra.concurrent.ScheduledExecutors;
 import org.apache.cassandra.config.CFMetaData;
 import org.apache.cassandra.config.Schema;
 import org.apache.cassandra.cql3.functions.FunctionName;
 import org.apache.cassandra.cql3.statements.ParsedStatement;
-import org.apache.cassandra.db.*;
+import org.apache.cassandra.db.Directories;
+import org.apache.cassandra.db.Keyspace;
+import org.apache.cassandra.db.SystemKeyspace;
 import org.apache.cassandra.db.marshal.*;
 import org.apache.cassandra.db.marshal.TupleType;
-import org.apache.cassandra.exceptions.*;
+import org.apache.cassandra.exceptions.CassandraException;
+import org.apache.cassandra.exceptions.ConfigurationException;
+import org.apache.cassandra.exceptions.SyntaxException;
 import org.apache.cassandra.io.util.FileUtils;
 import org.apache.cassandra.serializers.TypeSerializer;
 import org.apache.cassandra.service.ClientState;
@@ -670,6 +669,19 @@ public abstract class CQLTester
                : replaceValues(query, values);
     }
 
+    protected void assertValidSyntax(String query) throws Throwable
+    {
+        try
+        {
+            QueryProcessor.parseStatement(query);
+        }
+        catch(SyntaxException e)
+        {
+            Assert.fail(String.format("Expected query syntax to be valid but was invalid. Query is: %s; Error is %s",
+                                      query, e.getMessage()));
+        }
+    }
+
     protected void assertInvalidSyntax(String query, Object... values) throws Throwable
     {
         assertInvalidSyntaxMessage(null, query, values);

http://git-wip-us.apache.org/repos/asf/cassandra/blob/2f3fd416/test/unit/org/apache/cassandra/cql3/RoleSyntaxTest.java
----------------------------------------------------------------------
diff --git a/test/unit/org/apache/cassandra/cql3/RoleSyntaxTest.java b/test/unit/org/apache/cassandra/cql3/RoleSyntaxTest.java
new file mode 100644
index 0000000..02bfe61
--- /dev/null
+++ b/test/unit/org/apache/cassandra/cql3/RoleSyntaxTest.java
@@ -0,0 +1,51 @@
+/*
+ * 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.cassandra.cql3;
+
+import org.junit.Test;
+
+public class RoleSyntaxTest extends CQLTester
+{
+    @Test
+    public void standardOptionsSyntaxTest() throws Throwable
+    {
+        assertValidSyntax("CREATE ROLE r WITH LOGIN = true AND SUPERUSER = false AND PASSWORD = 'foo'");
+        assertValidSyntax("CREATE ROLE r WITH PASSWORD = 'foo' AND LOGIN = true AND SUPERUSER = false");
+        assertValidSyntax("CREATE ROLE r WITH SUPERUSER = true AND PASSWORD = 'foo' AND LOGIN = false");
+        assertValidSyntax("CREATE ROLE r WITH LOGIN = true AND PASSWORD = 'foo' AND SUPERUSER = false");
+        assertValidSyntax("CREATE ROLE r WITH SUPERUSER = true AND PASSWORD = 'foo' AND LOGIN = false");
+
+        assertValidSyntax("ALTER ROLE r WITH LOGIN = true AND SUPERUSER = false AND PASSWORD = 'foo'");
+        assertValidSyntax("ALTER ROLE r WITH PASSWORD = 'foo' AND LOGIN = true AND SUPERUSER = false");
+        assertValidSyntax("ALTER ROLE r WITH SUPERUSER = true AND PASSWORD = 'foo' AND LOGIN = false");
+        assertValidSyntax("ALTER ROLE r WITH LOGIN = true AND PASSWORD = 'foo' AND SUPERUSER = false");
+        assertValidSyntax("ALTER ROLE r WITH SUPERUSER = true AND PASSWORD = 'foo' AND LOGIN = false");
+    }
+
+    @Test
+    public void customOptionsSyntaxTestl() throws Throwable
+    {
+        assertValidSyntax("CREATE ROLE r WITH OPTIONS = {'a':'b', 'b':1}");
+        assertInvalidSyntax("CREATE ROLE r WITH OPTIONS = 'term'");
+        assertInvalidSyntax("CREATE ROLE r WITH OPTIONS = 99");
+
+        assertValidSyntax("ALTER ROLE r WITH OPTIONS = {'a':'b', 'b':1}");
+        assertInvalidSyntax("ALTER ROLE r WITH OPTIONS = 'term'");
+        assertInvalidSyntax("ALTER ROLE r WITH OPTIONS = 99");
+    }
+}