Merge pull request #1235 from code-corps/fix-project-users

Fix project user membership assignment

Author: joshsmith

config/dev.exs

@@ -1,4 +1,5 @@
 use Mix.Config
+require Logger
 
 # For development, we disable any cache and enable
 # debugging and code reloading.
@@ -26,8 +27,7 @@ config :code_corps, CodeCorpsWeb.Endpoint,
   ]
 
 # Do not include metadata nor timestamps in development logs
-config :logger,
-  :console, format: "[$level] $message\n"
+config :logger, :console, format: "[$level] $message\n"
 
 # Set a higher stacktrace during development. Avoid configuring such
 # in production as building large stacktraces may be expensive.
@@ -54,11 +54,9 @@ config :code_corps, :analytics, CodeCorps.Analytics.InMemoryAPI
 config :code_corps, :stripe, Stripe
 config :code_corps, :stripe_env, :dev
 
-config :sentry,
-  environment_name: Mix.env || :dev
+config :sentry, environment_name: Mix.env() || :dev
 
-config :code_corps, CodeCorps.Mailer,
-  adapter: Bamboo.LocalAdapter
+config :code_corps, CodeCorps.Mailer, adapter: Bamboo.LocalAdapter
 
 config :code_corps,
   postmark_forgot_password_template: "123",
@@ -69,7 +67,7 @@ config :code_corps,
 # If the dev environment has no CLOUDEX_API_KEY set, we want the app
 # to still run, with cloudex in test API mode
 if System.get_env("CLOUDEX_API_KEY") == nil do
-  IO.puts("NOTE: No Cloudex configuration found. Cloudex is running in test mode.")
+  Logger.info("NOTE: No Cloudex configuration found. Cloudex is running in test mode.")
   config :code_corps, :cloudex, CloudexTest
   config :cloudex, api_key: "test_key", secret: "test_secret", cloud_name: "test_cloud_name"
 end

lib/code_corps/policy/helpers.ex

@@ -5,40 +5,50 @@ defmodule CodeCorps.Policy.Helpers do
   """
 
   alias CodeCorps.{
-    Organization, ProjectUser,
-    Project, ProjectUser, Repo,
-    TaskSkill, Task, User, UserTask
+    Organization,
+    ProjectUser,
+    Project,
+    ProjectUser,
+    Repo,
+    TaskSkill,
+    Task,
+    User,
+    UserTask
   }
+
   alias Ecto.Changeset
 
   @doc """
   Determines if the provided organization or project is owned by the provided user
   """
-  @spec owned_by?(nil | Organization.t | Project.t, User.t) :: boolean
-  def owned_by?(%Organization{owner_id: owner_id}, %User{id: user_id}),
-    do: owner_id == user_id
+  @spec owned_by?(nil | Organization.t() | Project.t(), User.t()) :: boolean
+  def owned_by?(%Organization{owner_id: owner_id}, %User{id: user_id}), do: owner_id == user_id
+
   def owned_by?(%Project{} = project, %User{} = user),
     do: project |> get_membership(user) |> get_role |> owner?
+
   def owned_by?(nil, _), do: false
 
   @doc """
   Determines if the provided project is being administered by the provided User
 
   Returns `true` if the user is an admin or higher member of the project
   """
-  @spec administered_by?(nil | Project.t, User.t) :: boolean
+  @spec administered_by?(nil | Project.t(), User.t()) :: boolean
   def administered_by?(%Project{} = project, %User{} = user),
     do: project |> get_membership(user) |> get_role |> admin_or_higher?
+
   def administered_by?(nil, _), do: false
 
   @doc """
   Determines if the provided project is being contributed to by the provided User
 
   Returns `true` if the user is a contributor or higher member of the project
   """
-  @spec contributed_by?(nil | Project.t, User.t) :: boolean
+  @spec contributed_by?(nil | Project.t(), User.t()) :: boolean
   def contributed_by?(%Project{} = project, %User{} = user),
     do: project |> get_membership(user) |> get_role |> contributor_or_higher?
+
   def contributed_by?(nil, _), do: false
 
   @doc """
@@ -47,10 +57,13 @@ defmodule CodeCorps.Policy.Helpers do
 
   Returns `CodeCorps.Organization`
   """
-  @spec get_organization(struct | Changeset.t | any) :: Organization.t
+  @spec get_organization(struct | Changeset.t() | any) :: Organization.t()
   def get_organization(%{"organization_id" => id}), do: Organization |> Repo.get(id)
   def get_organization(%{organization_id: id}), do: Organization |> Repo.get(id)
-  def get_organization(%Changeset{changes: %{organization_id: id}}), do: Organization |> Repo.get(id)
+
+  def get_organization(%Changeset{changes: %{organization_id: id}}),
+    do: Organization |> Repo.get(id)
+
   def get_organization(_), do: nil
 
   @doc """
@@ -59,7 +72,7 @@ defmodule CodeCorps.Policy.Helpers do
 
   Returns `CodeCorps.Project`
   """
-  @spec get_project(struct | Changeset.t | any) :: Project.t
+  @spec get_project(struct | Changeset.t() | any) :: Project.t()
   def get_project(%{"project_id" => id}), do: Project |> Repo.get(id)
   def get_project(%{project_id: id}), do: Project |> Repo.get(id)
   def get_project(%Changeset{changes: %{project_id: id}}), do: Project |> Repo.get(id)
@@ -68,27 +81,27 @@ defmodule CodeCorps.Policy.Helpers do
   @doc """
   Retrieves the role field from a `CodeCorps.ProjectUser` struct or an `Ecto.Changeset`
   """
-  @spec get_role(nil | ProjectUser.t | Changeset.t) :: String.t | nil
+  @spec get_role(nil | ProjectUser.t() | Changeset.t()) :: String.t() | nil
   def get_role(nil), do: nil
   def get_role(%ProjectUser{role: role}), do: role
   def get_role(%Changeset{} = changeset), do: changeset |> Changeset.get_field(:role)
 
-  @spec admin_or_higher?(String.t) :: boolean
+  @spec admin_or_higher?(String.t()) :: boolean
   defp admin_or_higher?(role) when role in ["admin", "owner"], do: true
   defp admin_or_higher?(_), do: false
 
-  @spec contributor_or_higher?(String.t) :: boolean
+  @spec contributor_or_higher?(String.t()) :: boolean
   defp contributor_or_higher?(role) when role in ["contributor", "admin", "owner"], do: true
   defp contributor_or_higher?(_), do: false
 
-  @spec owner?(String.t) :: boolean
+  @spec owner?(String.t()) :: boolean
   defp owner?("owner"), do: true
   defp owner?(_), do: false
 
   @doc """
   Retrieves task from associated record
   """
-  @spec get_task(Changeset.t | TaskSkill.t | UserTask.t | map) :: Task.t
+  @spec get_task(Changeset.t() | TaskSkill.t() | UserTask.t() | map) :: Task.t()
   def get_task(%TaskSkill{task_id: task_id}), do: Repo.get(Task, task_id)
   def get_task(%UserTask{task_id: task_id}), do: Repo.get(Task, task_id)
   def get_task(%{"task_id" => task_id}), do: Repo.get(Task, task_id)
@@ -97,12 +110,14 @@ defmodule CodeCorps.Policy.Helpers do
   @doc """
   Determines if the provided task was authored by the provided user
   """
-  @spec task_authored_by?(Task.t, User.t) :: boolean
+  @spec task_authored_by?(Task.t(), User.t()) :: boolean
   def task_authored_by?(%Task{user_id: author_id}, %User{id: user_id}), do: user_id == author_id
 
-  # Returns `CodeCorps.ProjectUser` for specified `CodeCorps.Project`
-  # and `CodeCorps.User`, or nil
-  @spec get_membership(Project.t, User.t) :: nil | ProjectUser.t
-  defp get_membership(%Project{id: project_id}, %User{id: user_id}),
+  @doc """
+  Returns `CodeCorps.ProjectUser` for specified `CodeCorps.Project`
+  and `CodeCorps.User`, or nil
+  """
+  @spec get_membership(Project.t(), User.t()) :: nil | ProjectUser.t()
+  def get_membership(%Project{id: project_id}, %User{id: user_id}),
     do: ProjectUser |> Repo.get_by(project_id: project_id, user_id: user_id)
 end

lib/code_corps/policy/project_user.ex

@@ -2,51 +2,83 @@ defmodule CodeCorps.Policy.ProjectUser do
   @moduledoc """
   Handles `User` authorization of actions on `ProjectUser` records
   """
-  import CodeCorps.Policy.Helpers, only: [get_role: 1]
+
+  import CodeCorps.Policy.Helpers, only: [get_membership: 2, get_project: 1, get_role: 1]
 
   alias CodeCorps.{ProjectUser, Repo, User}
 
-  @spec create?(User.t, map) :: boolean
-  def create?(%User{id: user_id}, %{"user_id" => author_id})
-    when user_id == author_id and not is_nil(user_id), do: true
-  def create?(%User{}, %{}), do: false
+  @spec create?(User.t(), map) :: boolean
+  def create?(%User{id: user_id}, %{"user_id" => author_id, "role" => "pending"})
+      when user_id == author_id do
+    # Non-member can only make pending if they're the user
+    true
+  end
+
+  def create?(%User{} = user, %{"user_id" => _, "project_id" => _} = params) do
+    user_role =
+      params
+      |> get_project()
+      |> get_membership(user)
+      |> get_role()
+
+    new_role = Map.get(params, "role")
+    do_create?(user_role, new_role)
+  end
+
+  def create?(_, _), do: false
 
-  @spec update?(User.t, ProjectUser.t, map) :: boolean
+  defp do_create?("pending", _), do: false
+  defp do_create?("contributor", _), do: false
+  defp do_create?("admin", "pending"), do: true
+  defp do_create?("admin", "contributor"), do: true
+  defp do_create?("admin", _), do: false
+  defp do_create?("owner", _), do: true
+  defp do_create?(_, _), do: false
+
+  @spec update?(User.t(), ProjectUser.t(), map) :: boolean
   def update?(%User{} = user, %ProjectUser{} = existing_record, params) do
-    user_membership = existing_record |> get_project_membership(user)
+    user_role =
+      existing_record
+      |> get_project_membership(user)
+      |> get_role()
 
-    user_role = user_membership |> get_role
-    old_role = existing_record |> get_role
+    old_role = existing_record |> get_role()
     new_role = Map.get(params, "role")
 
-    case [user_role, old_role, new_role] do
-      # Non-member, pending and contributors can't do anything
-      [nil, _, _] -> false
-      ["pending", _, _] -> false
-      ["contributor", _, _] -> false
-      # Admins can only approve pending memberships and nothing else
-      ["admin", "pending", "contributor"] -> true
-      ["admin", _, _] -> false
-      # Owners can do everything expect changing other owners
-      ["owner", "owner", _] -> false
-      ["owner", _, _] -> true
-      # No other role change is allowed
-      [_, _, _] -> false
-    end
+    do_update?(user_role, old_role, new_role)
   end
 
-  @spec delete?(User.t, ProjectUser.t) :: boolean
+  defp do_update?(nil, _, _), do: false
+  defp do_update?("pending", _, _), do: false
+  defp do_update?("contributor", _, _), do: false
+  defp do_update?("admin", "pending", "contributor"), do: true
+  defp do_update?("admin", _, _), do: false
+  defp do_update?("owner", "owner", _), do: false
+  defp do_update?("owner", _, _), do: true
+  defp do_update?(_, _, _), do: false
+
+  @spec delete?(User.t(), ProjectUser.t()) :: boolean
   def delete?(%User{} = user, %ProjectUser{} = record) do
     record |> get_project_membership(user) |> do_delete?(record)
   end
 
-  defp do_delete?(%ProjectUser{} = user_m, %ProjectUser{} = current_m) when user_m == current_m, do: true
+  defp do_delete?(%ProjectUser{} = user_m, %ProjectUser{} = current_m) when user_m == current_m,
+    do: true
+
   defp do_delete?(%ProjectUser{role: "owner"}, %ProjectUser{}), do: true
-  defp do_delete?(%ProjectUser{role: "admin"}, %ProjectUser{role: role}) when role in ~w(pending contributor), do: true
+
+  defp do_delete?(%ProjectUser{role: "admin"}, %ProjectUser{role: role})
+       when role in ~w(pending contributor),
+       do: true
+
   defp do_delete?(_, _), do: false
 
   defp get_project_membership(%ProjectUser{user_id: nil}, %User{id: nil}), do: nil
-  defp get_project_membership(%ProjectUser{user_id: m_id} = membership, %User{id: u_id}) when m_id == u_id, do: membership
+
+  defp get_project_membership(%ProjectUser{user_id: m_id} = membership, %User{id: u_id})
+       when m_id == u_id,
+       do: membership
+
   defp get_project_membership(%ProjectUser{project_id: project_id}, %User{id: user_id}) do
     ProjectUser |> Repo.get_by(project_id: project_id, user_id: user_id)
   end

priv/repo/structure.sql

@@ -2,8 +2,8 @@
 -- PostgreSQL database dump
 --
 
--- Dumped from database version 10.0
--- Dumped by pg_dump version 10.0
+-- Dumped from database version 9.5.10
+-- Dumped by pg_dump version 10.1
 
 SET statement_timeout = 0;
 SET lock_timeout = 0;

test/lib/code_corps/policy/project_user_test.exs

@@ -4,16 +4,73 @@ defmodule CodeCorps.Policy.ProjectUserTest do
   import CodeCorps.Policy.ProjectUser, only: [create?: 2, update?: 3, delete?: 2]
 
   describe "create?/2" do
-    test "returns true when user is creating their own membership" do
+    test "when user is creating their own pending membership" do
       user = insert(:user)
+      project = insert(:project)
 
-      assert create?(user, %{"user_id" => user.id})
+      params = %{"project_id" => project.id, "user_id" => user.id, "role" => "pending"}
+      assert create?(user, params)
     end
 
-    test "returns false for normal user, creating someone else's membership" do
+    test "when user is creating any other membership" do
       user = insert(:user)
+      project = insert(:project)
+
+      params = %{"project_id" => project.id, "user_id" => user.id, "role" => "contributor"}
+      refute create?(user, params)
+    end
+
+    test "when normal user is creating someone else's membership" do
+      user = insert(:user)
+      project = insert(:project)
+
+      params = %{"project_id" => project.id, "user_id" => "someone_else"}
+      refute create?(user, params)
+    end
+
+    test "when pending user is creating someone else's membership" do
+      pending = insert(:user)
+      project = insert(:project)
+      insert(:project_user, role: "pending", user: pending, project: project)
+
+      params = %{"project_id" => project.id, "user_id" => "someone_else"}
+      refute create?(pending, params)
+    end
+
+    test "when contributor is creating someone else's membership" do
+      contributor = insert(:user)
+      project = insert(:project)
+      insert(:project_user, role: "contributor", user: contributor, project: project)
+
+      params = %{"project_id" => project.id, "user_id" => "someone_else"}
+      refute create?(contributor, params)
+    end
+
+    test "when user is admin and role is contributor" do
+      admin = insert(:user)
+      project = insert(:project)
+      insert(:project_user, role: "admin", user: admin, project: project)
+
+      params = %{"project_id" => project.id, "user_id" => "someone_else", "role" => "contributor"}
+      assert create?(admin, params)
+    end
+
+    test "when user is admin and role is admin" do
+      admin = insert(:user)
+      project = insert(:project)
+      insert(:project_user, role: "admin", user: admin, project: project)
+
+      params = %{"project_id" => project.id, "user_id" => "someone_else", "role" => "admin"}
+      refute create?(admin, params)
+    end
+
+    test "when user is owner" do
+      owner = insert(:user)
+      project = insert(:project)
+      insert(:project_user, role: "owner", user: owner, project: project)
 
-      refute create?(user, %{"user_id" => "someone_else"})
+      params = %{"project_id" => project.id, "user_id" => "someone_else", "role" => "owner"}
+      assert create?(owner, params)
     end
   end