Merge pull request #38 from epochtalk/role-validate

Role validate

Author: akinsey

lib/epochtalk_server/models/role_permission.ex

@@ -58,56 +58,72 @@ defmodule EpochtalkServer.Models.RolePermission do
   def insert([%{} | _] = roles_permissions),
     do: Repo.insert_all(RolePermission, roles_permissions)
 
-  ## for admin api use, modifying permissions for a role
+  @doc """
+  For admin api use.
+
+  Updates the `modified` value of `RolePermission`s for a `Role`
+  and updates the `Role`'s permissions and priority restrictions
+
+  If a permission is not included in `new_permissions`, it will be set to
+  `false`.  `new_permissions` may be an empty list - in which case, all
+  permissions will be set to `false`.
+
+  Any permissions in `new_permissions` which are not valid will be ignored.
+  """
+  @spec modify_by_role(role :: Role.t()) :: {:ok, :success}
   def modify_by_role(
         %Role{
           id: role_id,
           permissions: new_permissions,
           priority_restrictions: priority_restrictions
         } = _new_role
       ) do
-    # if a permission is false, they're not included in the permissions
-    # if they're all false, permissions can be empty
-    old_role_permissions =
-      from(rp in RolePermission,
-        where: rp.role_id == ^role_id
-      )
-      |> Repo.all()
-
-    new_permissions = new_permissions |> Iteraptor.to_flatmap()
-
-    # change a permission if it's different
-    new_role_permissions =
-      Enum.reduce(old_role_permissions, [], fn %{
-                                                 permission_path: permission_path,
-                                                 value: old_value
-                                               } = _old_role_permission,
-                                               acc ->
-        # check new value for permission_path
-        # if value is not there, set it to false
-        new_value = new_permissions[permission_path] || false
-        # if new value is different
-        new_role_permission =
-          if old_value != new_value do
-            # set modified true
-            %{role_id: role_id, permission_path: permission_path, modified: true}
-            # if new value is same, set modified false
-          else
-            %{role_id: role_id, permission_path: permission_path, modified: false}
-          end
-
-        [new_role_permission | acc]
-      end)
-
-    # update role permissions for this role
-    upsert_modified(new_role_permissions)
-
-    # update role's permissions
-    permissions = RolePermission.permissions_map_by_role_id(role_id)
-    Role.set_permissions(role_id, permissions)
-
-    # update role's priority_restrictions
-    Role.set_priority_restrictions(role_id, priority_restrictions)
+    if is_map(new_permissions) do
+      # get current set of role permissions
+      current_role_permissions =
+        from(rp in RolePermission,
+          where: rp.role_id == ^role_id
+        )
+        |> Repo.all()
+
+      # flat map the new permissions into permissions paths
+      new_permissions_paths = new_permissions |> Iteraptor.to_flatmap()
+
+      # calculate updated role permissions based on default values
+      new_role_permissions =
+        Enum.reduce(current_role_permissions, [], fn %{
+                                                       permission_path: permission_path,
+                                                       value: default_value
+                                                     } = _current_role_permission,
+                                                     acc ->
+          # get new value for permission_path if it exists
+          # otherwise, set it to false
+          new_value = new_permissions_paths[permission_path] || false
+
+          new_role_permission =
+            if default_value != new_value do
+              # if new value is different from default value, set modified true
+              %{role_id: role_id, permission_path: permission_path, modified: true}
+            else
+              # if new value is same as default value, set modified false
+              %{role_id: role_id, permission_path: permission_path, modified: false}
+            end
+
+          [new_role_permission | acc]
+        end)
+
+      # update role permissions for this role
+      upsert_modified(new_role_permissions)
+
+      # update role's permissions
+      permissions = RolePermission.permissions_map_by_role_id(role_id)
+      Role.set_permissions(role_id, permissions)
+    end
+
+    if is_list(priority_restrictions) do
+      # update role's priority_restrictions
+      Role.set_priority_restrictions(role_id, priority_restrictions)
+    end
 
     # return success
     {:ok, :success}

lib/epochtalk_server_web/controllers/role_controller.ex

@@ -19,7 +19,7 @@ defmodule EpochtalkServerWeb.RoleController do
          :ok <- ACL.allow!(conn, "roles.update"),
          id <- Validate.cast(attrs, "id", :integer, min: 1),
          # TODO(boka): implement validators
-         priority_restrictions <- Validate.sanitize_list(attrs, "priority_restrictions"),
+         priority_restrictions <- attrs["priority_restrictions"],
          permissions <- attrs["permissions"],
          {:ok, data} <-
            RolePermission.modify_by_role(%Role{

lib/epochtalk_server_web/helpers/validate.ex

@@ -6,15 +6,6 @@ defmodule EpochtalkServerWeb.Helpers.Validate do
   """
   alias EpochtalkServerWeb.CustomErrors.InvalidPayload
 
-  @doc """
-  Helper used to default a list request parameter.  If the list is valid, returns the list
-  otherwise, defaults to empty list
-  """
-  @spec sanitize_list(attrs :: map, key :: String.t()) :: [any()] | []
-  def sanitize_list(attrs, key)
-      when is_map(attrs) and is_binary(key),
-      do: if(is_list(attrs[key]), do: attrs[key], else: [])
-
   @doc """
   Helper used to validate and cast request parameters directly out of the incoming
   paylod map (usually a controller function's `attrs` parameter) to the specified type.

test/epochtalk_server/session_test.exs

@@ -59,12 +59,12 @@ defmodule EpochtalkServerWeb.SessionTest do
     test "deletes an expired user session when logging in", %{conn: conn, user: user} do
       remember_me = false
       # create session that should be deleted
-      {:ok, authed_user_to_delete, _token, authed_conn_to_delete} =
+      {:ok, _authed_user_to_delete, _token, authed_conn_to_delete} =
         Session.create(user, remember_me, conn)
 
       session_id_to_delete = authed_conn_to_delete.private.guardian_default_claims["jti"]
       # create session that shouldn't be deleted
-      {:ok, authed_user_to_persist, _token, authed_conn_to_persist} =
+      {:ok, _authed_user_to_persist, _token, authed_conn_to_persist} =
         Session.create(user, remember_me, conn)
 
       session_id_to_persist = authed_conn_to_persist.private.guardian_default_claims["jti"]
@@ -94,7 +94,7 @@ defmodule EpochtalkServerWeb.SessionTest do
       ])
 
       # create a new session (should delete expired sessions)
-      {:ok, new_authed_user, _token, new_authed_conn} = Session.create(user, remember_me, conn)
+      {:ok, _new_authed_user, _token, new_authed_conn} = Session.create(user, remember_me, conn)
       new_session_id = new_authed_conn.private.guardian_default_claims["jti"]
       # check that active sessions are still active
       {:ok, resource_to_persist} = Session.get_resource(user.id, session_id_to_persist)