diff --git a/.claude/commands/commit.md b/.claude/commands/commit.md deleted file mode 100644 index 3f7ca0f10c5..00000000000 --- a/.claude/commands/commit.md +++ /dev/null @@ -1,34 +0,0 @@ ---- -description: Create git commits for session changes ---- - -# Commit Changes - -You are tasked with creating git commits for the changes made during this session. - -## Process: - -**Important:** Ensure all git commands are run from the repository root directory. Check your current working directory and navigate to the repo root if needed before proceeding. - -1. **Think about what changed:** - - Review the conversation history and understand what was accomplished - - Run `git status` to see current changes - - Run `git diff` to understand the modifications - - Consider whether changes should be one commit or multiple logical commits - -2. **Plan your commit(s):** - - Identify which files belong together - - Draft clear, descriptive commit messages - - Use .claude/deaiify.md to inform your language - - Use imperative mood in commit messages - - Focus on why the changes were made, not just what - -3. **Present your plan to the user:** - - Confirm the plan with the user before executing — list files and commit messages, then ask to proceed. - -4. **Execute upon confirmation:** - - Create commits with your planned messages - - Optionally show the result with `git log --oneline -n [number]`. - -## Important: -- **NEVER add co-author information or Claude attribution** — write commit messages as if the user wrote them. diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 00000000000..671a0844777 --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,6 @@ +{ + "attribution": { + "commit": "", + "pr": "" + } +} diff --git a/.claude/skills/implement-plan/SKILL.md b/.claude/skills/implement-plan/SKILL.md index 0b094269698..846b13662f8 100644 --- a/.claude/skills/implement-plan/SKILL.md +++ b/.claude/skills/implement-plan/SKILL.md @@ -22,5 +22,5 @@ Plans from `/create-plan` carry three things a fresh session should honour: When the code disagrees with the plan, stop and ask before adapting. -When every phase passes: `/code-review`, fix what holds, then `/commit`. +When every phase passes: `/code-review`, fix what holds, then commit. Report any manual criteria still outstanding. diff --git a/lib/lightning/actor.ex b/lib/lightning/actor.ex new file mode 100644 index 00000000000..3e4bfc28007 --- /dev/null +++ b/lib/lightning/actor.ex @@ -0,0 +1,8 @@ +defmodule Lightning.Actor do + @moduledoc """ + Whoever a context function records as having done something: a person or a + service account. + """ + + @type t :: Lightning.Accounts.User.t() | Lightning.ServiceAccount.t() +end diff --git a/lib/lightning/policies/project_users.ex b/lib/lightning/policies/project_users.ex index fe6e2f38eac..d7b3e8c43ef 100644 --- a/lib/lightning/policies/project_users.ex +++ b/lib/lightning/policies/project_users.ex @@ -18,6 +18,12 @@ defmodule Lightning.Policies.ProjectUsers do Adding an action means adding an atom to `@admin_actions` or `@editor_actions`. It inherits the guard; there is nothing to remember. + A service account holds no role. Its authority is the scope its token + carries, which `LightningWeb.Plugs.PersonOrServiceAccountAuth` checks at the + route; here it may do only what `@service_account_actions` lists. That list + is separate and written out, so a new admin or editor action is refused to + the service account until it is added there. + We deny by default: an action in none of the lists is refused. """ @behaviour Bodyguard.Policy @@ -25,6 +31,7 @@ defmodule Lightning.Policies.ProjectUsers do alias Lightning.Accounts.User alias Lightning.Projects.ProjectUser alias Lightning.Projects.Scope + alias Lightning.ServiceAccount @admin_actions [ :write_webhook_auth_method, @@ -52,6 +59,27 @@ defmodule Lightning.Policies.ProjectUsers do @other_actions [:access_project, :delete_project, :publish_template] + @service_account_actions [ + :write_webhook_auth_method, + :write_github_connection, + :edit_project, + :edit_data_retention, + :add_project_user, + :remove_project_user, + :edit_run_settings, + :create_workflow, + :edit_workflow, + :delete_workflow, + :run_workflow, + :create_project_credential, + :initiate_github_sync, + :create_channel, + :delete_channel, + :update_channel, + :access_project, + :delete_project + ] + @type actions :: :access_project | :delete_project @@ -84,10 +112,10 @@ defmodule Lightning.Policies.ProjectUsers do do: @admin_actions ++ @editor_actions ++ @self_actions ++ @other_actions @doc """ - Whether `user` may perform `action`, where the subject is anything that + Whether `actor` may perform `action`, where the subject is anything that identifies a project — see `t:Lightning.Projects.Scope.subject/0`. """ - @spec authorize(actions(), User.t(), Scope.subject()) :: boolean() + @spec authorize(actions(), Scope.actor(), Scope.subject()) :: boolean() # These act on a specific membership row, not on project standing. The alert # handlers in `ProjectLive.Settings` pass a client-supplied "project_user_id", @@ -109,8 +137,8 @@ defmodule Lightning.Policies.ProjectUsers do ) end - def authorize(action, %User{} = user, subject) do - case Scope.fetch(user, subject) do + def authorize(action, actor, subject) do + case Scope.fetch(actor, subject) do {:ok, scope} -> permitted?(action, scope) @@ -133,6 +161,9 @@ defmodule Lightning.Policies.ProjectUsers do # later — without anyone having to remember @admin_actions/@editor_actions. def permitted?(_action, %Scope{mfa_satisfied?: false}), do: false + def permitted?(action, %Scope{actor: %ServiceAccount{}}), + do: action in @service_account_actions + def permitted?(:access_project, %Scope{} = scope), do: has_standing?(scope) def permitted?(:delete_project, %Scope{role: role}), do: role == :owner diff --git a/lib/lightning/policies/provisioning.ex b/lib/lightning/policies/provisioning.ex index f0b6d1cee67..61c0e330341 100644 --- a/lib/lightning/policies/provisioning.ex +++ b/lib/lightning/policies/provisioning.ex @@ -15,8 +15,9 @@ defmodule Lightning.Policies.Provisioning do so a push could re-enable the triggers that shutting the project down had disabled. - Only a superuser can provision a project that does not exist yet. Owners and - admins can update an existing one. + Only a superuser or a service account can create a project, through + `:create_project`; provisioning a project that does not exist yet asks the + same. Owners and admins can update an existing one. """ @behaviour Bodyguard.Policy @@ -24,19 +25,26 @@ defmodule Lightning.Policies.Provisioning do alias Lightning.Policies.Permissions alias Lightning.Projects.Project alias Lightning.Projects.Scope + alias Lightning.ServiceAccount alias Lightning.VersionControl.ProjectRepoConnection - @type actor :: User.t() | ProjectRepoConnection.t() - @type actions :: :provision_project | :describe_project + @type actions :: :create_project | :provision_project | :describe_project - @spec authorize(actions(), actor(), Project.t()) :: + @spec authorize(actions(), Scope.actor(), Project.t() | nil) :: boolean() | {:error, :forbidden} + def authorize(:create_project, %User{role: :superuser}, _), do: true + + # The route already held the token to its projects scope. + def authorize(:create_project, %ServiceAccount{}, _), do: true + + def authorize(:create_project, _, _), do: {:error, :forbidden} + # STAYS FIRST. A project that does not exist yet cannot be scheduled for # deletion, and has no members to consult. Reordering this below the clauses # that resolve a Scope breaks project creation through the API. - def authorize(:provision_project, %User{role: role}, %Project{id: nil}) do - role in [:superuser] or {:error, :forbidden} + def authorize(:provision_project, %User{} = user, %Project{id: nil}) do + authorize(:create_project, user, nil) end def authorize(:provision_project, %User{} = user, %Project{} = project) do diff --git a/lib/lightning/projects.ex b/lib/lightning/projects.ex index 39a4baef74e..a5a5848b009 100644 --- a/lib/lightning/projects.ex +++ b/lib/lightning/projects.ex @@ -547,22 +547,87 @@ defmodule Lightning.Projects do |> Repo.all() end + @member_roles ProjectUser.RolesEnum.__valid_values__() + |> Enum.filter(&is_binary/1) + + @doc """ + Resolves members given as `%{"email" => email, "role" => role}` to the + `%{user_id: id, role: role}` rows a project's `project_users` take. + + Emails match regardless of case. The messages never contain an email, so + they can be shown to whoever submitted the list. + """ + @spec members_by_email([term()]) :: + {:ok, + [%{user_id: Ecto.UUID.t(), role: :owner | :admin | :editor | :viewer}]} + | {:error, [String.t()]} + def members_by_email(members) do + if Enum.all?(members, &member_shape?/1) do + user_ids = + members + |> Enum.map(& &1["email"]) + |> Lightning.Accounts.list_users_by_emails() + |> Map.new(&{String.downcase(&1.email), &1.id}) + + members + |> Enum.map(fn %{"email" => email, "role" => role} -> + %{ + user_id: Map.get(user_ids, String.downcase(email)), + role: String.to_existing_atom(role) + } + end) + |> check_members() + else + {:error, + [ + "each member needs an email and a role of #{Enum.join(@member_roles, ", ")}" + ]} + end + end + + defp member_shape?(%{"email" => email, "role" => role}), + do: is_binary(email) and role in @member_roles + + defp member_shape?(_member), do: false + + defp check_members(members) do + {found, missing} = members |> Enum.map(& &1.user_id) |> Enum.split_with(& &1) + + messages = + for {true, message} <- [ + {missing != [], "names an email no user holds"}, + {found != Enum.uniq(found), "names one user more than once"} + ], + do: message + + if messages == [], do: {:ok, members}, else: {:error, messages} + end + @doc """ - Creates a project. + Creates a project, recording `actor` as having created it and added each of + its initial members. + + ## Options + + * `:notify` - email each initial member that they were added (default `true`) ## Examples - iex> create_project(%{field: value}) + iex> create_project(%{field: value}, actor) {:ok, %Project{}} - iex> create_project(%{field: bad_value}) + iex> create_project(%{field: bad_value}, actor) {:error, %Ecto.Changeset{}} """ - def create_project(attrs \\ %{}, schedule_email? \\ true) do + @spec create_project(map(), Lightning.Actor.t(), keyword()) :: + {:ok, Project.t()} | {:error, Ecto.Changeset.t()} + def create_project(attrs, actor, opts \\ []) do Repo.transact(fn -> - with {:ok, project} <- ProjectHook.handle_create_project(attrs) do - if schedule_email? do + with {:ok, project} <- ProjectHook.handle_create_project(attrs), + project = Repo.preload(project, :project_users), + {:ok, _events} <- record_creation(project, actor) do + if Keyword.get(opts, :notify, true) do schedule_project_addition_emails(%Project{project_users: []}, project) end @@ -574,19 +639,32 @@ defmodule Lightning.Projects do end) end + defp record_creation(project, actor) do + Multi.new() + |> Audit.derive_creation_events(project, actor) + |> Repo.transaction() + |> case do + {:ok, events} -> {:ok, events} + {:error, _operation, changeset, _changes_so_far} -> {:error, changeset} + end + end + @doc """ Updates a project. ## Examples - iex> update_project(project, %{field: new_value}) + iex> update_project(project, %{field: new_value}, actor) {:ok, %Project{}} - iex> update_project(project, %{field: bad_value}) + iex> update_project(project, %{field: bad_value}, actor) {:error, %Ecto.Changeset{}} """ - def update_project(%Project{} = project, attrs, user \\ nil) do + @spec update_project(Project.t(), map(), Lightning.Actor.t()) :: + {:ok, Project.t()} + | {:error, Ecto.Changeset.t() | :not_related_to_project} + def update_project(%Project{} = project, attrs, actor) do changeset = project |> Project.changeset(attrs) @@ -594,7 +672,7 @@ defmodule Lightning.Projects do Multi.new() |> Multi.update(:project, changeset) - |> maybe_audit_changes(changeset, user) + |> Audit.derive_events(changeset, actor) |> Repo.transaction() |> case do {:ok, %{project: updated_project}} -> @@ -617,13 +695,6 @@ defmodule Lightning.Projects do end end - defp maybe_audit_changes(multi, _changeset, nil), do: multi - - defp maybe_audit_changes(multi, changeset, user) do - multi - |> Audit.derive_events(changeset, user) - end - @spec update_project_with_users(Project.t(), map(), User.t(), boolean()) :: {:ok, Project.t()} | {:error, Ecto.Changeset.t()} def update_project_with_users( @@ -638,6 +709,7 @@ defmodule Lightning.Projects do project |> membership_multi(changeset, actor) + |> Audit.derive_events(changeset, actor) |> Repo.transaction() |> case do {:ok, %{project: updated_project, membership_changes: changes}} -> @@ -2257,10 +2329,10 @@ defmodule Lightning.Projects do end @doc """ - Creates a sandbox under the given `parent` by delegating to `create_project/2`. + Creates a sandbox under the given `parent` by delegating to `create_project/3`. - This is a convenience wrapper that sets `:parent_id` and preserves the - existing behavior around collaborator emails (off by default unless `schedule_email?` is `true`). + This is a convenience wrapper that sets `:parent_id` and emails no + collaborators. ## Notes @@ -2274,10 +2346,12 @@ defmodule Lightning.Projects do * `{:ok, %Project{}}` on success * `{:error, %Ecto.Changeset{}}` on validation/unique errors """ - @spec create_sandbox(Project.t(), map(), boolean()) :: + @spec create_sandbox(Project.t(), map(), Lightning.Actor.t()) :: {:ok, Project.t()} | {:error, Ecto.Changeset.t()} - def create_sandbox(%Project{id: parent_id}, attrs, schedule_email? \\ false) do - attrs |> Map.put(:parent_id, parent_id) |> create_project(schedule_email?) + def create_sandbox(%Project{id: parent_id}, attrs, actor) do + attrs + |> Map.put(:parent_id, parent_id) + |> create_project(actor, notify: false) end @doc """ diff --git a/lib/lightning/projects/audit.ex b/lib/lightning/projects/audit.ex index 5ddeac1d857..8fc28f16b97 100644 --- a/lib/lightning/projects/audit.ex +++ b/lib/lightning/projects/audit.ex @@ -10,18 +10,24 @@ defmodule Lightning.Projects.Audit do "collaborator_added", "collaborator_removed", "collaborator_role_changed", + "created", "dataclip_retention_period_updated", + "description_updated", "history_retention_period_updated", + "name_updated", "requires_mfa_updated" ] alias Ecto.Multi + alias Lightning.Projects.Project def derive_events(multi, changeset, user) do [ :allow_support_access, :dataclip_retention_period, + :description, :history_retention_period, + :name, :requires_mfa ] |> Enum.reduce(multi, fn field, multi -> @@ -38,6 +44,26 @@ defmodule Lightning.Projects.Audit do end) end + @doc """ + Appends the events for a newly created project: `created`, then one + `collaborator_added` per member in its `project_users`. + """ + def derive_creation_events(multi, %Project{} = project, actor) do + multi + |> Multi.insert( + :audit_created, + event("created", project.id, actor, %{ + before: nil, + after: %{"name" => project.name, "description" => project.description} + }) + ) + |> derive_membership_events( + project.id, + Enum.map(project.project_users, &{:added, &1}), + actor + ) + end + @doc """ Appends an audit event for every collaborator change in `changes`. diff --git a/lib/lightning/projects/project.ex b/lib/lightning/projects/project.ex index b8046faf01b..da3c0d8a546 100644 --- a/lib/lightning/projects/project.ex +++ b/lib/lightning/projects/project.ex @@ -299,6 +299,27 @@ defmodule Lightning.Projects.Project do |> cast_assoc(:project_users, required: true, sort_param: :users_sort) |> validate() |> validate_project_owner() + |> write_owner_last() + |> unique_constraint(:id, name: :projects_pkey) + end + + # Children are written in list order and the one-owner index can't be + # deferred, so an ownership handover only succeeds if the old owner's + # demotion reaches the database before the new owner's row. + defp write_owner_last(changeset) do + case get_change(changeset, :project_users) do + nil -> + changeset + + project_users -> + {owners, others} = + Enum.split_with(project_users, fn project_user -> + project_user.action in [:insert, :update] and + get_field(project_user, :role) == :owner + end) + + put_change(changeset, :project_users, others ++ owners) + end end defp validate_project_owner(changeset) do diff --git a/lib/lightning/projects/sandboxes.ex b/lib/lightning/projects/sandboxes.ex index 2bfeb556f47..647fdc7c1f1 100644 --- a/lib/lightning/projects/sandboxes.ex +++ b/lib/lightning/projects/sandboxes.ex @@ -1027,7 +1027,11 @@ defmodule Lightning.Projects.Sandboxes do sandbox_env ) - case create_empty_sandbox(parent_with_data, sandbox_attrs) do + case Lightning.Projects.create_sandbox( + parent_with_data, + sandbox_attrs, + actor + ) do {:ok, sandbox} -> sandbox |> Repo.preload(:project_users) @@ -1083,10 +1087,6 @@ defmodule Lightning.Projects.Sandboxes do }) end - defp create_empty_sandbox(parent, attrs) do - Lightning.Projects.create_sandbox(parent, attrs, false) - end - defp clone_credentials_from_parent(sandbox, parent) do current_time = DateTime.utc_now() |> DateTime.truncate(:second) diff --git a/lib/lightning/projects/scope.ex b/lib/lightning/projects/scope.ex index bcfeacd5cd3..18e2de98817 100644 --- a/lib/lightning/projects/scope.ex +++ b/lib/lightning/projects/scope.ex @@ -25,6 +25,9 @@ defmodule Lightning.Projects.Scope do `nil`. Ownership is checked before liveness, so a connection learns nothing about a project that is not its own. + A `%ServiceAccount{}` has no membership row either and is not tied to one + project; its `role` is always `nil`. + Scheduling deletion removes no membership rows and revokes no token, so this refusal is the whole of the offboarding gate during the purge window. @@ -50,6 +53,7 @@ defmodule Lightning.Projects.Scope do alias Lightning.Projects.Project alias Lightning.Projects.ProjectUser alias Lightning.Repo + alias Lightning.ServiceAccount alias Lightning.VersionControl.ProjectRepoConnection # `mfa_satisfied?` defaults to false so a `%Scope{}` that never went through @@ -77,9 +81,9 @@ defmodule Lightning.Projects.Scope do @typedoc """ Whoever is asking. A `%User{}` may hold a role; a `%ProjectRepoConnection{}` - never does. + and a `%ServiceAccount{}` never do. """ - @type actor :: User.t() | ProjectRepoConnection.t() + @type actor :: Lightning.Actor.t() | ProjectRepoConnection.t() @typedoc """ Anything that identifies a project: a loaded `%Project{}`, a project id, or @@ -107,6 +111,8 @@ defmodule Lightning.Projects.Scope do a project that exists, and — for a `%ProjectRepoConnection{}` actor — `{:error, :connection_not_for_this_project}` when the resolved project is not the one the connection belongs to. + + A `%ServiceAccount{}` resolves any live project, with no role. """ @spec fetch(actor(), subject()) :: {:ok, t()} | {:error, error()} def fetch(%User{} = user, subject) do @@ -127,6 +133,14 @@ defmodule Lightning.Projects.Scope do end end + def fetch(%ServiceAccount{} = account, subject) do + with {:ok, project} <- resolve_project(subject), + :ok <- still_operable(project) do + # Set rather than defaulted: a machine credential cannot enrol in MFA. + {:ok, %__MODULE__{project: project, actor: account, mfa_satisfied?: true}} + end + end + @doc """ Whether the user holds one of `roles` in the project. diff --git a/lib/lightning/service_account/access_token.ex b/lib/lightning/service_account/access_token.ex index 459e73c5fcc..1417bdd32e3 100644 --- a/lib/lightning/service_account/access_token.ex +++ b/lib/lightning/service_account/access_token.ex @@ -9,7 +9,7 @@ defmodule Lightning.ServiceAccount.AccessToken do alias Lightning.ServiceAccount - @scopes ["users:read", "users:write"] + @scopes ["users:read", "users:write", "projects:write"] @lifetime 300 @typ "at+jwt" @@ -47,6 +47,17 @@ defmodule Lightning.ServiceAccount.AccessToken do token end + @doc """ + Whether `token` declares itself an access token by its `typ`, without + checking anything else about it. + """ + @spec access_token?(String.t()) :: boolean() + def access_token?(token) do + match?(%{"typ" => @typ}, Jason.decode!(JOSE.JWS.peek_protected(token))) + rescue + _ -> false + end + @doc """ Returns the claims of `token` when its `typ`, signature, `iss`, `aud` and `exp` all check out. diff --git a/lib/lightning/setup_utils.ex b/lib/lightning/setup_utils.ex index fab20c43446..d087a74d166 100644 --- a/lib/lightning/setup_utils.ex +++ b/lib/lightning/setup_utils.ex @@ -197,7 +197,8 @@ defmodule Lightning.SetupUtils do Application.get_env(:lightning, :default_retention_period), project_users: project_users }, - false + owner!(project_users), + notify: false ) if with_workflow do @@ -341,7 +342,8 @@ defmodule Lightning.SetupUtils do id: "4adf2644-ed4e-4f97-a24c-ab35b3cb1efa", project_users: project_users }, - false + owner!(project_users), + notify: false ) user = get_most_privileged_user!(openhie_project) @@ -592,7 +594,8 @@ defmodule Lightning.SetupUtils do name: "dhis2-project", project_users: project_users }, - false + owner!(project_users), + notify: false ) user = get_most_privileged_user!(project) @@ -944,6 +947,11 @@ defmodule Lightning.SetupUtils do dataclip end + defp owner!(project_users) do + %{user_id: user_id} = Enum.find(project_users, &(&1.role == :owner)) + Repo.get!(User, user_id) + end + defp get_most_privileged_user!(project) do role_ordering_query = from( diff --git a/lib/lightning_web/controllers/api/project_controller.ex b/lib/lightning_web/controllers/api/project_controller.ex index f39db4da342..7cffe6203cf 100644 --- a/lib/lightning_web/controllers/api/project_controller.ex +++ b/lib/lightning_web/controllers/api/project_controller.ex @@ -5,6 +5,9 @@ defmodule LightningWeb.API.ProjectController do Provides read access to projects for authenticated users and API tokens. Users can list projects they have access to and retrieve individual project details. + `PUT /api/projects/:id` creates the project under that id with its members, + or replaces the name and description of the project that has it. + ## Query Parameters (index) - `page` - Page number (default: 1) @@ -33,9 +36,14 @@ defmodule LightningWeb.API.ProjectController do """ use LightningWeb, :controller - alias Lightning.Policies.Permissions + import Ecto.Changeset + alias Lightning.Policies.ProjectUsers + alias Lightning.Policies.Provisioning alias Lightning.Projects + alias Lightning.Projects.Project + alias Lightning.Projects.Scope + alias Lightning.ServiceAccount action_fallback LightningWeb.FallbackController @@ -87,6 +95,7 @@ defmodule LightningWeb.API.ProjectController do - `200 OK` with project JSON on success - `404 Not Found` if project doesn't exist - `403 Forbidden` if user lacks access to the project + - `409 Conflict` to a service account, if the project is scheduled for deletion ## Examples @@ -94,19 +103,189 @@ defmodule LightningWeb.API.ProjectController do """ @spec show(Plug.Conn.t(), map()) :: Plug.Conn.t() def show(conn, %{"id" => id}) do - with %Lightning.Projects.Project{} = project <- - Projects.get_project(id), + with %Project{} = project <- Projects.get_project(id), :ok <- - ProjectUsers - |> Permissions.can( - :access_project, - conn.assigns.current_resource, - project - ) do - render(conn, "show.json", project: project, conn: conn) + authorize(:access_project, conn.assigns.current_resource, project) do + render_project(conn, project) else nil -> {:error, :not_found} error -> error end end + + @doc """ + Creates the project under the path's id, or replaces the name and description + of the one that has it. + + `members` and `notify` only apply when the project is created. + """ + @spec update(Plug.Conn.t(), map()) :: Plug.Conn.t() | {:error, term()} + def update(conn, %{"id" => id}) do + actor = conn.assigns.current_resource + + with :ok <- known_keys(conn.body_params), + {:ok, attrs} <- parse_body(conn.body_params, id) do + case Projects.get_project(attrs.id) do + nil -> create(conn, attrs, actor) + project -> replace(conn, project, attrs, actor) + end + else + {:error, :unknown_keys, keys} -> + render_errors(conn, Map.new(keys, &{&1, ["is not accepted"]})) + + error -> + error + end + end + + @body_types %{ + id: :binary_id, + name: :string, + description: :string, + members: {:array, :map}, + notify: :boolean + } + + @body_keys @body_types |> Map.keys() |> Enum.map(&to_string/1) + + defp known_keys(%{"_json" => _not_an_object}), do: :ok + + defp known_keys(body) do + case Map.keys(body) -- @body_keys do + [] -> :ok + keys -> {:error, :unknown_keys, keys} + end + end + + defp parse_body(%{"_json" => _not_an_object}, _id) do + {:error, + {%{}, %{body: :map}} + |> change() + |> add_error(:body, "must be a JSON object")} + end + + defp parse_body(body, id) do + changeset = + {%{notify: true}, @body_types} + |> cast(body, Map.keys(@body_types)) + |> validate_required(:notify, message: "must be true or false") + + changeset = + case Ecto.UUID.cast(id) do + {:ok, id} -> + changeset + |> validate_change(:id, fn :id, body_id -> + if body_id == id, + do: [], + else: [id: "does not match the id in the path"] + end) + |> put_change(:id, id) + + :error -> + add_error(changeset, :id, "is not a UUID") + end + + apply_action(changeset, :validate) + end + + defp replace(conn, project, attrs, actor) do + with :ok <- authorize(:edit_project, actor, project), + {:ok, project} <- + Projects.update_project( + project, + %{name: attrs[:name], description: attrs[:description]}, + actor + ) do + render_project(conn, project) + end + end + + # A service account passes every project policy, so telling it the project + # is scheduled for deletion leaks nothing; a person gets the policy's answer. + defp authorize(action, actor, project) do + case Scope.fetch(actor, project) do + {:ok, scope} -> + if ProjectUsers.permitted?(action, scope), + do: :ok, + else: {:error, :forbidden} + + {:error, :project_scheduled_for_deletion} + when is_struct(actor, ServiceAccount) -> + {:error, :scheduled_for_deletion} + + {:error, _reason} -> + {:error, :forbidden} + end + end + + defp create(conn, attrs, actor) do + with true <- Provisioning.authorize(:create_project, actor, nil), + {:ok, project_users} <- + Projects.members_by_email(attrs[:members] || []) do + attrs + |> Map.take([:id, :name, :description]) + |> Map.put(:project_users, project_users) + |> Projects.create_project(actor, notify: attrs.notify) + |> case do + {:ok, project} -> + conn |> put_status(:created) |> render_project(project) + + {:error, changeset} -> + if id_taken?(changeset), + do: replace(conn, Projects.get_project(attrs.id), attrs, actor), + else: render_create_errors(conn, changeset) + end + else + {:error, messages} when is_list(messages) -> + render_errors(conn, %{members: messages}) + + error -> + error + end + end + + # Another request created the project between our lookup and our insert. + defp id_taken?(changeset) do + Enum.any?(changeset.errors, fn {field, {_message, opts}} -> + field == :id and opts[:constraint] == :unique + end) + end + + # The project's changeset checks members as `owner` and `project_users`; + # this API calls them `members`. + defp render_create_errors(conn, changeset) do + errors = LightningWeb.CoreComponents.translate_errors(changeset) + + {member_errors, errors} = Map.split(errors, [:owner, :project_users]) + + errors = + if member_errors == %{}, + do: errors, + else: Map.put(errors, :members, messages(member_errors)) + + render_errors(conn, errors) + end + + # A member's own errors arrive as one map per member, empty for the members + # that are fine. + defp messages(errors) when is_map(errors), + do: errors |> Map.values() |> messages() + + defp messages(errors) when is_list(errors), + do: Enum.flat_map(errors, &messages/1) + + defp messages(message) when is_binary(message), do: [message] + + defp render_errors(conn, errors) do + conn + |> put_status(:unprocessable_entity) + |> json(%{errors: errors}) + end + + defp render_project(conn, project) do + render(conn, "show.json", + project: Lightning.Repo.preload(project, project_users: :user), + conn: conn + ) + end end diff --git a/lib/lightning_web/controllers/api/project_json.ex b/lib/lightning_web/controllers/api/project_json.ex index 9874b937dd9..e73fe05f01a 100644 --- a/lib/lightning_web/controllers/api/project_json.ex +++ b/lib/lightning_web/controllers/api/project_json.ex @@ -21,7 +21,12 @@ defmodule LightningWeb.API.ProjectJSON do def render("show.json", %{project: project, conn: conn}) do %{ - data: resource(conn, project), + data: + put_in( + resource(conn, project), + [:attributes, :members], + members(project) + ), included: [], links: %{ self: url_for(conn) @@ -29,6 +34,10 @@ defmodule LightningWeb.API.ProjectJSON do } end + defp members(project) do + Enum.map(project.project_users, &%{email: &1.user.email, role: &1.role}) + end + defp resource(conn, project) do %{ type: "projects", diff --git a/lib/lightning_web/controllers/api/user_controller.ex b/lib/lightning_web/controllers/api/user_controller.ex index a6049bb53a5..6a9d890ee20 100644 --- a/lib/lightning_web/controllers/api/user_controller.ex +++ b/lib/lightning_web/controllers/api/user_controller.ex @@ -48,7 +48,7 @@ defmodule LightningWeb.API.UserController do Accounts.update_user( user, Map.take(params, @update_fields), - conn.assigns.service_account + conn.assigns.current_resource ) do render(conn, "show.json", user: user, conn: conn) end @@ -57,7 +57,7 @@ defmodule LightningWeb.API.UserController do def create(conn, params) do case Accounts.create_user( Map.take(params, @create_fields), - conn.assigns.service_account + conn.assigns.current_resource ) do {:ok, user} -> conn diff --git a/lib/lightning_web/controllers/fallback_controller.ex b/lib/lightning_web/controllers/fallback_controller.ex index 5098c4aea94..7c4a9b7644a 100644 --- a/lib/lightning_web/controllers/fallback_controller.ex +++ b/lib/lightning_web/controllers/fallback_controller.ex @@ -38,6 +38,12 @@ defmodule LightningWeb.FallbackController do }) end + def call(conn, {:error, :scheduled_for_deletion}) do + conn + |> put_status(:conflict) + |> json(%{error: "scheduled_for_deletion"}) + end + def call(conn, {:error, :forbidden}) do conn |> put_status(:forbidden) diff --git a/lib/lightning_web/controllers/token_exchange_controller.ex b/lib/lightning_web/controllers/token_exchange_controller.ex index e14f9b58a1d..f017f8d0e4d 100644 --- a/lib/lightning_web/controllers/token_exchange_controller.ex +++ b/lib/lightning_web/controllers/token_exchange_controller.ex @@ -81,19 +81,15 @@ defmodule LightningWeb.TokenExchangeController do defp client_assertion(_params), do: {:error, :invalid_request} defp requested_scopes(%{"scope" => scope}) when is_binary(scope) do - case String.split(scope) do - [] -> - {:ok, AccessToken.scopes()} - - scopes -> - if Enum.all?(scopes, &(&1 in AccessToken.scopes())), - do: {:ok, Enum.uniq(scopes)}, - else: {:error, :invalid_scope} - end + scopes = String.split(scope) + + if scopes != [] and Enum.all?(scopes, &(&1 in AccessToken.scopes())), + do: {:ok, Enum.uniq(scopes)}, + else: {:error, :invalid_scope} end defp requested_scopes(%{"scope" => _}), do: {:error, :invalid_request} - defp requested_scopes(_params), do: {:ok, AccessToken.scopes()} + defp requested_scopes(_params), do: {:error, :invalid_scope} defp authenticate(assertion, params) do case Assertion.verify(assertion, url(~p"/api/oauth/token")) do diff --git a/lib/lightning_web/live/dashboard_live/project_creation_modal.ex b/lib/lightning_web/live/dashboard_live/project_creation_modal.ex index fbb2b1f31c4..680c940860c 100644 --- a/lib/lightning_web/live/dashboard_live/project_creation_modal.ex +++ b/lib/lightning_web/live/dashboard_live/project_creation_modal.ex @@ -52,7 +52,7 @@ defmodule LightningWeb.DashboardLive.ProjectCreationModal do "role" => "owner" } }) - |> Projects.create_project(false) + |> Projects.create_project(current_user, notify: false) |> case do {:ok, project} -> {:noreply, diff --git a/lib/lightning_web/live/project_live/form_component.ex b/lib/lightning_web/live/project_live/form_component.ex index 21e58442c3d..62616aa7317 100644 --- a/lib/lightning_web/live/project_live/form_component.ex +++ b/lib/lightning_web/live/project_live/form_component.ex @@ -181,7 +181,7 @@ defmodule LightningWeb.ProjectLive.FormComponent do end defp save_project(socket, :new, project_params) do - case Projects.create_project(project_params) do + case Projects.create_project(project_params, socket.assigns.current_user) do {:ok, _project} -> {:noreply, socket diff --git a/lib/lightning_web/plugs/access_token_auth.ex b/lib/lightning_web/plugs/access_token_auth.ex index a62aa06686c..b69cc4d5424 100644 --- a/lib/lightning_web/plugs/access_token_auth.ex +++ b/lib/lightning_web/plugs/access_token_auth.ex @@ -6,8 +6,9 @@ defmodule LightningWeb.Plugs.AccessTokenAuth do `call/2` goes in the pipeline. It accepts only tokens issued to the service account registered now, so unsetting `SERVICE_ACCOUNT_PUBLIC_KEY` cuts off tokens already issued, and assigns the token's claims as `:access_token` and - the service account as `:service_account`. Each controller then names the scope its actions need with - `plug :require_scope, "users:read"`. Refusals follow RFC 6750 §3. + the service account as `:current_resource`. Each controller then names the + scope its actions need with `plug :require_scope, "users:read"`. Refusals + follow RFC 6750 §3. """ import Plug.Conn @@ -23,7 +24,7 @@ defmodule LightningWeb.Plugs.AccessTokenAuth do Lightning.Config.service_account() do conn |> assign(:access_token, claims) - |> assign(:service_account, service_account) + |> assign(:current_resource, service_account) else _other -> refuse(conn, 401, "invalid_token") end diff --git a/lib/lightning_web/plugs/person_or_service_account_auth.ex b/lib/lightning_web/plugs/person_or_service_account_auth.ex new file mode 100644 index 00000000000..ecfc2b91ad6 --- /dev/null +++ b/lib/lightning_web/plugs/person_or_service_account_auth.ex @@ -0,0 +1,39 @@ +defmodule LightningWeb.Plugs.PersonOrServiceAccountAuth do + @moduledoc """ + Authenticates a request by either a service account's access token or the + bearer tokens `LightningWeb.UserAuth` accepts (a personal access token or a + repo connection token). + + A token that declares itself an access token goes to + `LightningWeb.Plugs.AccessTokenAuth` and must carry the `scope:` this plug is + given. Any other request gets exactly the answer + `UserAuth.authenticate_bearer/2` and + `UserAuth.require_authenticated_api_resource/2` give it elsewhere. + + A resource opts in by adding a router pipeline that names its scope: + + plug LightningWeb.Plugs.PersonOrServiceAccountAuth, scope: "projects:write" + + A person's token carries no scopes, so the scope binds service accounts + only; for a person, the controller's policy decides. + """ + alias Lightning.ServiceAccount.AccessToken + alias LightningWeb.Plugs.AccessTokenAuth + alias LightningWeb.UserAuth + + def init(opts), do: Keyword.fetch!(opts, :scope) + + def call(conn, scope) do + with {:ok, token} <- UserAuth.get_bearer(conn), + true <- AccessToken.access_token?(token) do + conn = AccessTokenAuth.call(conn, []) + + if conn.halted, do: conn, else: AccessTokenAuth.require_scope(conn, scope) + else + _person -> + conn + |> UserAuth.authenticate_bearer([]) + |> UserAuth.require_authenticated_api_resource([]) + end + end +end diff --git a/lib/lightning_web/router.ex b/lib/lightning_web/router.ex index 68175472317..b5f22558ffd 100644 --- a/lib/lightning_web/router.ex +++ b/lib/lightning_web/router.ex @@ -51,6 +51,11 @@ defmodule LightningWeb.Router do plug LightningWeb.Plugs.AccessTokenAuth end + pipeline :projects_api do + plug :accepts, ["json"] + plug LightningWeb.Plugs.PersonOrServiceAccountAuth, scope: "projects:write" + end + pipeline :authenticated_api do plug :accepts, ["json"] plug LightningWeb.Plugs.ApiAuth @@ -101,6 +106,14 @@ defmodule LightningWeb.Router do patch "/users/:id", API.UserController, :update end + ## A service account's access token or a person's bearer token + scope "/api", LightningWeb, as: :api do + pipe_through [:projects_api] + + get "/projects/:id", API.ProjectController, :show + put "/projects/:id", API.ProjectController, :update + end + scope "/api", LightningWeb, as: :api do pipe_through [:api] @@ -114,7 +127,7 @@ defmodule LightningWeb.Router do get "/provision/yaml", API.ProvisioningController, :show_yaml resources "/provision", API.ProvisioningController, only: [:create, :show] - resources "/projects", API.ProjectController, only: [:index, :show] do + resources "/projects", API.ProjectController, only: [:index] do resources "/credentials", API.CredentialController, only: [:index] resources "/workflows", API.WorkflowsController, except: [:delete] resources "/jobs", API.JobController, only: [:index, :show] diff --git a/priv/bench/collections.exs b/priv/bench/collections.exs index 8cdd1b321dd..a20d9f13a5c 100644 --- a/priv/bench/collections.exs +++ b/priv/bench/collections.exs @@ -9,7 +9,7 @@ Repo.delete_all(Collections.Collection) project = with nil <- Repo.get_by(Projects.Project, name: "benchee") do user = Repo.get_by(Lightning.Accounts.User, email: "demo@openfn.org") - {:ok, project} = Projects.create_project(%{name: "benchee", project_users: [%{user_id: user.id, role: :owner}]}) + {:ok, project} = Projects.create_project(%{name: "benchee", project_users: [%{user_id: user.id, role: :owner}]}, user) project end diff --git a/priv/bench/collections_put_all.exs b/priv/bench/collections_put_all.exs index 2380475c25c..fd69497ae13 100644 --- a/priv/bench/collections_put_all.exs +++ b/priv/bench/collections_put_all.exs @@ -16,7 +16,7 @@ Repo.delete_all(Collections.Collection) project = with nil <- Repo.get_by(Projects.Project, name: "bench") do user = Repo.get_by(Lightning.Accounts.User, email: "demo@openfn.org") || raise "This benchmark requires demo/known user" - {:ok, project} = Projects.create_project(%{name: "bench", project_users: [%{user_id: user.id, role: :owner}]}) + {:ok, project} = Projects.create_project(%{name: "bench", project_users: [%{user_id: user.id, role: :owner}]}, user) project end diff --git a/test/lightning/kickstart_test.exs b/test/lightning/kickstart_test.exs index cc9254b457b..bb4f0978566 100644 --- a/test/lightning/kickstart_test.exs +++ b/test/lightning/kickstart_test.exs @@ -235,9 +235,9 @@ defmodule Lightning.KickstartTest do scenario(), ["projects", Access.at(0), "members"], fn [owner_member, editor_member] -> - # The incoming owner is declared FIRST on purpose. Declared in the - # other order the handover succeeds even without reconcile_members/2 - # sorting owners last, so this order is what pins the sort. + # Project.project_with_users_changeset/2 writes the owner row last, + # so the handover succeeds whatever order the members are declared + # in or the existing rows come back from the database in. [ Map.put(editor_member, "role", "owner"), Map.put(owner_member, "role", "editor") diff --git a/test/lightning/policies/project_user_permissions_test.exs b/test/lightning/policies/project_user_permissions_test.exs index 3df44ea5aa1..548d6b6af58 100644 --- a/test/lightning/policies/project_user_permissions_test.exs +++ b/test/lightning/policies/project_user_permissions_test.exs @@ -457,6 +457,69 @@ defmodule Lightning.Policies.ProjectUserPermissionsTest do end end + # The expected set is written out rather than read from the module, so a new + # action fails here until someone decides whether a service account gets it. + describe "a service account" do + setup do + {account, _private_key} = + Lightning.ServiceAccountHelpers.service_account_with_key() + + %{account: account} + end + + test "is allowed exactly the project actions, and nothing about its own row", + %{account: account, project: project} do + expected = + ~w( + access_project + delete_project + add_project_user + remove_project_user + edit_project + edit_data_retention + edit_run_settings + write_webhook_auth_method + write_github_connection + create_workflow + edit_workflow + delete_workflow + run_workflow + create_project_credential + initiate_github_sync + create_channel + update_channel + delete_channel + )a + + refused = ~w(publish_template edit_digest_alerts edit_failure_alerts)a + + assert Enum.sort(ProjectUsers.actions()) == + Enum.sort(expected ++ refused), + "classify every new action as allowed or refused for a service account" + + allowed = + for action <- ProjectUsers.actions(), + Permissions.can?(ProjectUsers, action, account, project), + do: action + + assert Enum.sort(allowed) == Enum.sort(expected) + end + + # A %ProjectUser{} subject goes to the general clause, not the "is this + # row mine" one, and is refused there. + test "is refused the self actions given someone's membership row", %{ + account: account, + project: project + } do + refute_can( + ProjectUsers, + [:edit_digest_alerts, :edit_failure_alerts], + account, + hd(project.project_users) + ) + end + end + describe "blocked_by_mfa?/1" do setup %{editor: editor} do enrolled = insert(:user, mfa_enabled: true) diff --git a/test/lightning/policies/provisioning_test.exs b/test/lightning/policies/provisioning_test.exs index fe038773cb2..2c422caceab 100644 --- a/test/lightning/policies/provisioning_test.exs +++ b/test/lightning/policies/provisioning_test.exs @@ -143,4 +143,40 @@ defmodule Lightning.Policies.ProvisioningTest do ) end end + + describe ":create_project" do + test "is allowed for a superuser" do + superuser = insert(:user, role: :superuser) + + assert Provisioning.authorize(:create_project, superuser, nil) + end + + test "is allowed for a service account" do + {account, _private_key} = + Lightning.ServiceAccountHelpers.service_account_with_key() + + assert Provisioning.authorize(:create_project, account, nil) + end + + test "is refused for a user who is not a superuser, whatever roles they hold elsewhere" do + for role <- [:owner, :admin, :editor, :viewer] do + user = insert(:user) + insert(:project, project_users: [%{user_id: user.id, role: role}]) + + assert Provisioning.authorize(:create_project, user, nil) == + {:error, :forbidden} + + assert Provisioning.authorize(:provision_project, user, %Project{ + id: nil + }) == {:error, :forbidden} + end + end + + test "is refused for a repo connection" do + repo_connection = insert(:project_repo_connection) + + assert Provisioning.authorize(:create_project, repo_connection, nil) == + {:error, :forbidden} + end + end end diff --git a/test/lightning/projects/membership_events_test.exs b/test/lightning/projects/membership_events_test.exs index dfa026613d9..896846e9105 100644 --- a/test/lightning/projects/membership_events_test.exs +++ b/test/lightning/projects/membership_events_test.exs @@ -123,6 +123,39 @@ defmodule Lightning.Projects.MembershipEventsTest do assert audit_count(project_id) == 3 end + test "audits a rename alongside the membership it changes", %{ + project: %{id: project_id, name: old_name} = project, + actor: %{id: actor_id} = actor, + owner_project_user: owner_project_user, + member_project_user: member_project_user + } do + assert {:ok, _project} = + Projects.update_project_with_users( + project, + %{ + name: "renamed", + project_users: [ + %{id: owner_project_user.id}, + %{id: member_project_user.id, delete: true} + ] + }, + actor, + false + ) + + assert %{ + item_id: ^project_id, + actor_id: ^actor_id, + changes: %Audit.Changes{ + before: %{"name" => ^old_name}, + after: %{"name" => "renamed"} + } + } = Repo.get_by!(Audit, event: "name_updated") + + assert Repo.get_by!(Audit, event: "collaborator_removed").actor_id == + actor_id + end + test "broadcasts and audits nothing when membership is resubmitted unchanged", %{ project: %{id: project_id} = project, diff --git a/test/lightning/projects/scope_test.exs b/test/lightning/projects/scope_test.exs index b06af7f2a3a..e521db01078 100644 --- a/test/lightning/projects/scope_test.exs +++ b/test/lightning/projects/scope_test.exs @@ -309,4 +309,55 @@ defmodule Lightning.Projects.ScopeTest do Scope.fetch(repo_connection, project) end end + + describe "a service account as the actor" do + setup do + {account, _private_key} = + Lightning.ServiceAccountHelpers.service_account_with_key() + + %{account: account} + end + + test "resolves a live project by project, id, or membership row, with no role", + %{account: account, live_project: project} do + project_user = + hd(Lightning.Repo.preload(project, :project_users).project_users) + + for subject <- [project, project.id, project_user] do + assert {:ok, scope} = Scope.fetch(account, subject) + assert scope.project.id == project.id + assert scope.actor == account + assert scope.role == nil + assert scope.project_user == nil + assert scope.support? == false + end + end + + # A machine credential cannot enrol, so the requirement does not apply. + test "meets the MFA requirement of a project that has one", %{ + account: account + } do + project = insert(:project, requires_mfa: true) + + assert {:ok, scope} = Scope.fetch(account, project) + assert scope.mfa_satisfied? == true + end + + test "is refused a scheduled-deleted project, same as a user", %{ + account: account, + scheduled_deleted_project: project + } do + assert {:error, :project_scheduled_for_deletion} = + Scope.fetch(account, %Project{id: project.id}) + end + + test "gets no such project for an unknown or malformed id", %{ + account: account + } do + assert {:error, :no_such_project} = + Scope.fetch(account, Ecto.UUID.generate()) + + assert {:error, :no_such_project} = Scope.fetch(account, "not-a-uuid") + end + end end diff --git a/test/lightning/projects_test.exs b/test/lightning/projects_test.exs index 57ec7a1f3b9..06a5076bab4 100644 --- a/test/lightning/projects_test.exs +++ b/test/lightning/projects_test.exs @@ -6,6 +6,7 @@ defmodule Lightning.ProjectsTest do import Lightning.AccountsFixtures import Lightning.Factories import Lightning.ProjectsFixtures + import Lightning.ServiceAccountHelpers import Mox import Swoosh.TestAssertions @@ -121,8 +122,8 @@ defmodule Lightning.ProjectsTest do assert Projects.get_project_user(project_user.id) == project_user end - test "create_project/1 with valid data creates a project" do - %{id: user_id} = insert(:user) + test "create_project/3 with valid data creates a project" do + %{id: user_id} = user = insert(:user) valid_attrs = %{ name: "some-name", @@ -130,7 +131,7 @@ defmodule Lightning.ProjectsTest do } assert {:ok, %Project{id: project_id} = project} = - Projects.create_project(valid_attrs) + Projects.create_project(valid_attrs, user) assert project.name == "some-name" @@ -138,23 +139,29 @@ defmodule Lightning.ProjectsTest do project.project_users end - test "create_project/1 expects project to have exactly one owner" do + test "create_project/3 expects project to have exactly one owner" do user = insert(:user) # creates successfully if there's one assert {:ok, _project} = - Projects.create_project(%{ - name: "some-name", - project_users: [%{user_id: user.id, role: :owner}] - }) + Projects.create_project( + %{ + name: "some-name", + project_users: [%{user_id: user.id, role: :owner}] + }, + user + ) # errors out if there is none for role <- [:admin, :editor, :viewer] do assert {:error, %Ecto.Changeset{errors: errors}} = - Projects.create_project(%{ - name: "some-name", - project_users: [%{user_id: user.id, role: role}] - }) + Projects.create_project( + %{ + name: "some-name", + project_users: [%{user_id: user.id, role: role}] + }, + user + ) assert [ {:owner, @@ -168,24 +175,61 @@ defmodule Lightning.ProjectsTest do another_user = insert(:user) assert {:error, %Ecto.Changeset{errors: errors}} = - Projects.create_project(%{ - name: "some-name", - project_users: [ - %{user_id: user.id, role: :owner}, - %{user_id: another_user.id, role: :owner} - ] - }) + Projects.create_project( + %{ + name: "some-name", + project_users: [ + %{user_id: user.id, role: :owner}, + %{user_id: another_user.id, role: :owner} + ] + }, + user + ) assert [{:owner, {"A project can have only one owner.", []}}] == errors end - test "create_project/1 with invalid data returns error changeset" do + test "update_project_with_users/4 hands ownership over when the promotion is listed before the demotion" do + owner = insert(:user) + editor = insert(:user) + + project = + insert(:project, + project_users: [ + %{user: owner, role: :owner}, + %{user: editor, role: :editor} + ] + ) + + owner_pu = Enum.find(project.project_users, &(&1.user_id == owner.id)) + editor_pu = Enum.find(project.project_users, &(&1.user_id == editor.id)) + + assert {:ok, _project} = + Projects.update_project_with_users( + project, + %{ + project_users: [ + %{id: editor_pu.id, role: :owner}, + %{id: owner_pu.id, role: :editor} + ] + }, + owner, + false + ) + + assert %{role: :editor} = Repo.reload!(owner_pu) + assert %{role: :owner} = Repo.reload!(editor_pu) + end + + test "create_project/3 with invalid data returns error changeset" do + user = insert(:user) + assert {:error, %Ecto.Changeset{}} = - Projects.create_project(@invalid_attrs) + Projects.create_project(@invalid_attrs, user) assert {:error, %Ecto.Changeset{}} = - Projects.create_project(%{"name" => "Can't have spaces!"}) + Projects.create_project(%{"name" => "Can't have spaces!"}, user) end test "set_notification_pref/3 with a changed value updates the field" do @@ -3248,34 +3292,213 @@ defmodule Lightning.ProjectsTest do end end + describe "members_by_email/1" do + test "resolves each member to its user, ignoring the email's case" do + owner = insert(:user, email: "owner@example.com") + viewer = insert(:user) + + assert {:ok, members} = + Projects.members_by_email([ + %{"email" => "OWNER@Example.com", "role" => "owner"}, + %{"email" => viewer.email, "role" => "viewer"} + ]) + + assert Enum.sort(members) == + Enum.sort([ + %{user_id: owner.id, role: :owner}, + %{user_id: viewer.id, role: :viewer} + ]) + end + + test "refuses members it cannot resolve, without echoing an email" do + user = insert(:user) + + refused = [ + unknown_email: [%{"email" => "nobody@example.com", "role" => "admin"}], + same_user_twice: [ + %{"email" => user.email, "role" => "admin"}, + %{"email" => String.upcase(user.email), "role" => "viewer"} + ], + unknown_role: [%{"email" => user.email, "role" => "superuser"}], + missing_role: [%{"email" => user.email}], + not_a_map: ["someone@example.com"] + ] + + for {case_name, members} <- refused do + assert {:error, [_ | _] = messages} = Projects.members_by_email(members), + "#{case_name}" + + refute Enum.any?(messages, &String.contains?(&1, "@")), + "#{case_name} echoes an email: #{inspect(messages)}" + end + end + + test "looks every member up in one query" do + users = insert_list(4, :user) + ref = make_ref() + pid = self() + + :telemetry.attach( + inspect(ref), + [:lightning, :repo, :query], + fn _, _, %{source: source}, _ -> + if source == "users" and self() == pid, + do: send(pid, {ref, :users_query}) + end, + nil + ) + + on_exit(fn -> :telemetry.detach(inspect(ref)) end) + + assert {:ok, [_, _, _, _]} = + Projects.members_by_email( + Enum.map(users, &%{"email" => &1.email, "role" => "editor"}) + ) + + assert_received {^ref, :users_query} + refute_received {^ref, :users_query} + end + end + + describe "create_project/3 audit and notification" do + setup do + owner = insert(:user) + admin = insert(:user) + viewer = insert(:user) + + members = [{owner, :owner}, {admin, :admin}, {viewer, :viewer}] + + %{ + members: members, + attrs: %{ + name: "audited", + description: "made by apply", + project_users: + Enum.map(members, fn {user, role} -> + %{user_id: user.id, role: role} + end) + } + } + end + + test "a service account is recorded as creating the project and adding each member", + %{attrs: attrs, members: members} do + {account, _key} = service_account_with_key() + + assert {:ok, project} = Projects.create_project(attrs, account) + + assert_creation_audited(project, members, account.uuid, :service_account) + end + + test "a person is recorded as creating the project and adding each member", + %{attrs: attrs, members: members} do + user = insert(:user) + + assert {:ok, project} = Projects.create_project(attrs, user) + + assert_creation_audited(project, members, user.id, :user) + end + + test "an id another project already holds is an error on id, not a raise", + %{attrs: attrs} do + taken = insert(:project) + + assert {:error, changeset} = + Projects.create_project( + Map.put(attrs, :id, taken.id), + insert(:user) + ) + + assert {_message, opts} = changeset.errors[:id] + assert opts[:constraint] == :unique + end + + test "an audit event it cannot record is an error, and leaves no project", + %{attrs: attrs} do + id = Ecto.UUID.generate() + + assert {:error, %Ecto.Changeset{} = changeset} = + Projects.create_project(Map.put(attrs, :id, id), %User{}) + + assert changeset.errors[:actor_id] + refute Repo.get(Project, id) + end + + test "emails each initial member by default, and none with notify: false", + %{attrs: attrs} do + user = insert(:user) + + Oban.Testing.with_testing_mode(:manual, fn -> + {:ok, quiet} = + Projects.create_project(%{attrs | name: "quiet"}, user, notify: false) + + {:ok, loud} = Projects.create_project(attrs, user) + + notified = + all_enqueued(worker: Lightning.Accounts.UserNotifier) + |> Enum.map(& &1.args["project_user_id"]) + |> Enum.sort() + + assert notified == loud.project_users |> Enum.map(& &1.id) |> Enum.sort() + + refute Enum.any?(quiet.project_users, &(&1.id in notified)) + end) + end + + defp assert_creation_audited(project, members, actor_id, actor_type) do + events = + from(a in Audit, where: a.item_id == ^project.id, order_by: a.event) + |> Repo.all() + |> Enum.map(&{&1.event, &1.actor_id, &1.actor_type, &1.changes}) + + added = + for {user, role} <- members do + {"collaborator_added", actor_id, actor_type, + %Audit.Changes{ + before: nil, + after: %{"user_id" => user.id, "role" => to_string(role)} + }} + end + + created = + {"created", actor_id, actor_type, + %Audit.Changes{ + before: nil, + after: %{"name" => "audited", "description" => "made by apply"} + }} + + assert Enum.sort([created | added]) == Enum.sort(events) + end + end + describe ".update_project/3" do setup do %{user: insert(:user)} end - test "update_project/3 with valid data updates the project" do + test "update_project/3 with valid data updates the project", %{user: user} do project = project_fixture() update_attrs = %{name: "some-updated-name"} assert {:ok, %Project{} = project} = - Projects.update_project(project, update_attrs) + Projects.update_project(project, update_attrs, user) assert project.name == "some-updated-name" end - test "update_project/3 updates the MFA requirement" do + test "update_project/3 updates the MFA requirement", %{user: user} do project = insert(:project) refute project.requires_mfa update_attrs = %{requires_mfa: true} assert {:ok, %Project{} = project} = - Projects.update_project(project, update_attrs) + Projects.update_project(project, update_attrs, user) assert project.requires_mfa end - test "update_project/3 updates the data retention periods" do + test "update_project/3 updates the data retention periods", %{user: user} do project = insert(:project, project_users: @@ -3296,7 +3519,7 @@ defmodule Lightning.ProjectsTest do } assert {:ok, %Project{} = updated_project} = - Projects.update_project(project, update_attrs) + Projects.update_project(project, update_attrs, user) # admins and owners receives an email admins = @@ -3340,7 +3563,7 @@ defmodule Lightning.ProjectsTest do # no email is sent when there's no change assert {:ok, updated_project} = - Projects.update_project(updated_project, update_attrs) + Projects.update_project(updated_project, update_attrs, user) for %{user: %{email: email}} <- project.project_users do refute_receive {:email, @@ -3353,10 +3576,14 @@ defmodule Lightning.ProjectsTest do # no email is sent when there's an error in the changeset assert {:error, _changeset} = - Projects.update_project(updated_project, %{ - history_retention_period: "xyz", - dataclip_retention_period: 7 - }) + Projects.update_project( + updated_project, + %{ + history_retention_period: "xyz", + dataclip_retention_period: 7 + }, + user + ) for %{user: %{email: email}} <- project.project_users do refute_receive {:email, @@ -3368,7 +3595,8 @@ defmodule Lightning.ProjectsTest do end end - test "update_project/3 rejects lowering history below existing dataclip retention" do + test "update_project/3 rejects lowering history below existing dataclip retention", + %{user: user} do project = insert(:project, history_retention_period: 30, @@ -3376,23 +3604,29 @@ defmodule Lightning.ProjectsTest do ) assert {:error, changeset} = - Projects.update_project(project, %{history_retention_period: 7}) + Projects.update_project( + project, + %{history_retention_period: 7}, + user + ) assert "dataclip retention period must be less or equal to the history retention period" in errors_on( changeset ).dataclip_retention_period end - test "update_project/3 with invalid data returns error changeset" do + test "update_project/3 with invalid data returns error changeset", %{ + user: user + } do project = project_fixture() |> unload_relation(:project_users) assert {:error, %Ecto.Changeset{}} = - Projects.update_project(project, @invalid_attrs) + Projects.update_project(project, @invalid_attrs, user) assert Repo.preload(project, :parent) == Projects.get_project!(project.id) end - test "update_project/2 calls the validate_changeset hook" do + test "update_project/3 calls the validate_changeset hook", %{user: user} do verify_on_exit!() project = @@ -3414,9 +3648,13 @@ defmodule Lightning.ProjectsTest do ) assert {:error, changeset} = - Projects.update_project(project, %{ - name: "new-name" - }) + Projects.update_project( + project, + %{ + name: "new-name" + }, + user + ) assert errors_on(changeset) == %{name: [error_msg]} end @@ -3518,24 +3756,41 @@ defmodule Lightning.ProjectsTest do } = Repo.get_by!(Audit, event: "requires_mfa_updated") end - test "does not create events if no user was provided" do - project = - insert( - :project, - dataclip_retention_period: 7, - history_retention_period: 30, - retention_policy: :retain_all - ) + test "records a service account renaming the project" do + {%{uuid: account_uuid} = account, _key} = service_account_with_key() + %{id: project_id} = project = insert(:project, name: "before") - update_attrs = %{ - dataclip_retention_period: 14, - history_retention_period: 90, - retention_policy: :retain_with_errors - } + assert {:ok, _} = + Projects.update_project(project, %{name: "after"}, account) - Projects.update_project(project, update_attrs) + assert %{ + item_id: ^project_id, + actor_id: ^account_uuid, + actor_type: :service_account, + changes: %Audit.Changes{ + before: %{"name" => "before"}, + after: %{"name" => "after"} + } + } = Repo.get_by!(Audit, event: "name_updated") + end - assert Audit |> Repo.all() |> Enum.empty?() + test "records a person changing the description", %{ + user: %{id: user_id} = user + } do + %{id: project_id} = project = insert(:project, description: "old") + + assert {:ok, _} = + Projects.update_project(project, %{description: "new"}, user) + + assert %{ + item_id: ^project_id, + actor_id: ^user_id, + actor_type: :user, + changes: %Audit.Changes{ + before: %{"description" => "old"}, + after: %{"description" => "new"} + } + } = Repo.get_by!(Audit, event: "description_updated") end test "does not create events if the project change fails", %{ @@ -3697,7 +3952,7 @@ defmodule Lightning.ProjectsTest do assert Enum.uniq(Enum.map(got, & &1.parent_id)) == [parent.id] end - test "create_sandbox/3 sets parent and creates a normal project (emails off by default)" do + test "create_sandbox/3 sets parent and creates a normal project" do owner = insert(:user) parent = @@ -3708,7 +3963,7 @@ defmodule Lightning.ProjectsTest do project_users: [%{user_id: owner.id, role: :owner}] } - assert {:ok, sandbox} = Projects.create_sandbox(parent, attrs) + assert {:ok, sandbox} = Projects.create_sandbox(parent, attrs, owner) assert sandbox.parent_id == parent.id assert sandbox.name == "sandbox-1" end @@ -5009,10 +5264,14 @@ defmodule Lightning.ProjectsTest do end defp change_retention_periods(project) do - Projects.update_project(project, %{ - history_retention_period: 14, - dataclip_retention_period: 7 - }) + Projects.update_project( + project, + %{ + history_retention_period: 14, + dataclip_retention_period: 7 + }, + insert(:user) + ) end defp data_retention_email(updated_project) do diff --git a/test/lightning/setup_utils_test.exs b/test/lightning/setup_utils_test.exs index f0145509612..01b0e04ab23 100644 --- a/test/lightning/setup_utils_test.exs +++ b/test/lightning/setup_utils_test.exs @@ -66,10 +66,10 @@ defmodule Lightning.SetupUtilsTest do assert viewer.email == "viewer@openfn.org" User.valid_password?(viewer, "welcome12345") - assert Enum.map( - openhie_project.project_users, - fn project_user -> project_user.user_id end - ) == [super_user.id, admin.id, editor.id, viewer.id] + assert openhie_project.project_users + |> Enum.map(fn project_user -> project_user.user_id end) + |> Enum.sort() == + Enum.sort([super_user.id, admin.id, editor.id, viewer.id]) assert Enum.map( dhis2_project.project_users, diff --git a/test/lightning_web/channels/run_channel_test.exs b/test/lightning_web/channels/run_channel_test.exs index ffd5f4d1487..f9a34f2af05 100644 --- a/test/lightning_web/channels/run_channel_test.exs +++ b/test/lightning_web/channels/run_channel_test.exs @@ -3480,7 +3480,7 @@ defmodule LightningWeb.RunChannelTest do end test "stops a support user's channel when the project withdraws support access", - %{project: project, run: run} do + %{actor: actor, project: project, run: run} do Mox.stub( Lightning.Extensions.MockProjectHook, :handle_project_validation, @@ -3488,7 +3488,11 @@ defmodule LightningWeb.RunChannelTest do ) {:ok, project} = - Lightning.Projects.update_project(project, %{allow_support_access: true}) + Lightning.Projects.update_project( + project, + %{allow_support_access: true}, + actor + ) support_user = insert(:user, support_user: true) @@ -3500,9 +3504,11 @@ defmodule LightningWeb.RunChannelTest do monitor_ref = Process.monitor(socket.channel_pid) {:ok, _project} = - Lightning.Projects.update_project(project, %{ - allow_support_access: false - }) + Lightning.Projects.update_project( + project, + %{allow_support_access: false}, + actor + ) assert_receive {:DOWN, ^monitor_ref, :process, _pid, :normal} end diff --git a/test/lightning_web/channels/workflow_channel_test.exs b/test/lightning_web/channels/workflow_channel_test.exs index 5e69ccb55cb..96611283604 100644 --- a/test/lightning_web/channels/workflow_channel_test.exs +++ b/test/lightning_web/channels/workflow_channel_test.exs @@ -2124,7 +2124,7 @@ defmodule LightningWeb.WorkflowChannelTest do channel_pid = socket.channel_pid monitor_ref = Process.monitor(channel_pid) - revoke_support_access(project) + revoke_support_access(project, insert(:user)) assert_receive {:DOWN, ^monitor_ref, :process, ^channel_pid, :normal} end @@ -2143,7 +2143,7 @@ defmodule LightningWeb.WorkflowChannelTest do log = capture_log(fn -> - revoke_support_access(project) + revoke_support_access(project, insert(:user)) ref = push(socket, "validate_workflow_name", %{"workflow" => %{}}) assert_reply ref, :ok, %{workflow: _} @@ -2169,7 +2169,7 @@ defmodule LightningWeb.WorkflowChannelTest do socket = join_as(support_user, project, workflow) assert socket.assigns.project_user - revoke_support_access(project) + revoke_support_access(project, insert(:user)) ref = push(socket, "validate_workflow_name", %{"workflow" => %{}}) assert_reply ref, :ok, %{workflow: _} diff --git a/test/lightning_web/controllers/api/project_controller_test.exs b/test/lightning_web/controllers/api/project_controller_test.exs index f5a9dd0a661..67e741c03d2 100644 --- a/test/lightning_web/controllers/api/project_controller_test.exs +++ b/test/lightning_web/controllers/api/project_controller_test.exs @@ -2,6 +2,8 @@ defmodule LightningWeb.API.ProjectControllerTest do use LightningWeb.ConnCase, async: true import Lightning.Factories + import Ecto.Query + import Lightning.ServiceAccountHelpers setup %{conn: conn} do {:ok, conn: put_req_header(conn, "accept", "application/json")} @@ -133,17 +135,31 @@ defmodule LightningWeb.API.ProjectControllerTest do test "with token for other project", %{conn: conn} do other_project = insert(:project) conn = get(conn, ~p"/api/projects/#{other_project.id}") - assert json_response(conn, 401) == %{"error" => "Unauthorized"} + assert json_response(conn, 403) == %{"error" => "Forbidden"} + end + + test "is refused with 403, not 409, on a project scheduled for deletion", + %{conn: conn, user: user} do + project = scheduled_project([%{user: user, role: :owner}]) + + conn = get(conn, ~p"/api/projects/#{project.id}") + + assert json_response(conn, 403) == %{"error" => "Forbidden"} end - test "shows the project", %{conn: conn, project: project} do + test "shows the project with its members", %{ + conn: conn, + project: project, + user: user + } do conn = get(conn, Routes.api_project_path(conn, :show, project)) response = json_response(conn, 200) assert response["data"] == %{ "attributes" => %{ "name" => project.name, - "description" => nil + "description" => nil, + "members" => [%{"email" => user.email, "role" => "editor"}] }, "id" => project.id, "links" => %{ @@ -155,4 +171,477 @@ defmodule LightningWeb.API.ProjectControllerTest do } end end + + describe "show, for a service account with projects:write" do + setup %{conn: conn} do + {conn, _account} = with_service_account(conn, ["projects:write"]) + %{conn: conn} + end + + test "shows a project it is not a member of, with its members", %{ + conn: conn + } do + owner = insert(:user) + project = insert(:project, project_users: [%{user: owner, role: :owner}]) + + conn = get(conn, ~p"/api/projects/#{project.id}") + + assert %{"id" => id, "attributes" => attributes} = + json_response(conn, 200)["data"] + + assert id == project.id + + assert attributes == %{ + "name" => project.name, + "description" => project.description, + "members" => [%{"email" => owner.email, "role" => "owner"}] + } + end + + test "answers 409 for a project scheduled for deletion", %{conn: conn} do + conn = get(conn, ~p"/api/projects/#{scheduled_project().id}") + + assert json_response(conn, 409) == %{"error" => "scheduled_for_deletion"} + end + end + + describe "show, for a repo connection" do + test "is refused its own project with 403", %{conn: conn} do + project = insert(:project) + repo_connection = insert(:project_repo_connection, project: project) + + conn = + conn + |> put_req_header( + "authorization", + "Bearer " <> repo_connection.access_token + ) + |> get(~p"/api/projects/#{project.id}") + + assert json_response(conn, 403) == %{"error" => "Forbidden"} + end + end + + defp put_project(conn, id, body) do + conn + |> put_req_header("content-type", "application/json") + |> put(~p"/api/projects/#{id}", Jason.encode!(body)) + end + + defp with_service_account(conn, scopes) do + {account, _private_key} = service_account_with_key() + Mox.stub(Lightning.MockConfig, :service_account, fn -> account end) + + token = Lightning.ServiceAccount.AccessToken.issue(account, scopes) + + {put_req_header(conn, "authorization", "Bearer " <> token), account} + end + + defp with_pat(conn, user) do + assign_bearer(conn, Lightning.Accounts.generate_api_token(user)) + end + + defp members_of(project_id) do + from(pu in Lightning.Projects.ProjectUser, + join: u in assoc(pu, :user), + where: pu.project_id == ^project_id, + select: {u.email, pu.role} + ) + |> Lightning.Repo.all() + |> Enum.sort() + end + + defp sorted_members(response) do + response["data"]["attributes"]["members"] + |> Enum.sort_by(& &1["email"]) + end + + defp four_members do + for role <- ~w(owner admin editor viewer) do + %{"email" => insert(:user).email, "role" => role} + end + end + + defp scheduled_project(project_users \\ []) do + insert(:project, + scheduled_deletion: DateTime.utc_now() |> DateTime.truncate(:second), + project_users: project_users + ) + end + + describe "PUT, for a service account with projects:write" do + setup %{conn: conn} do + {conn, account} = with_service_account(conn, ["projects:write"]) + %{conn: conn, account: account} + end + + test "creates the project with its members, and is not one of them", %{ + conn: conn, + account: account + } do + id = Ecto.UUID.generate() + members = four_members() + + conn = + put_project(conn, id, %{ + "name" => "acme", + "description" => "The acme project", + "members" => members, + "notify" => false + }) + + response = json_response(conn, 201) + + assert %{ + "id" => ^id, + "type" => "projects", + "attributes" => %{ + "name" => "acme", + "description" => "The acme project" + } + } = response["data"] + + assert sorted_members(response) == Enum.sort_by(members, & &1["email"]) + + assert members_of(id) == + members + |> Enum.map(&{&1["email"], String.to_existing_atom(&1["role"])}) + |> Enum.sort() + + actors = + from(a in Lightning.Auditing.Audit, + where: a.item_id == ^id, + select: {a.event, a.actor_id, a.actor_type} + ) + |> Lightning.Repo.all() + + assert Enum.count(actors, &match?({"collaborator_added", _, _}, &1)) == 4 + assert {"created", account.uuid, :service_account} in actors + + assert Enum.all?(actors, fn {_, actor_id, actor_type} -> + actor_id == account.uuid and actor_type == :service_account + end) + end + + test "emails the members it adds unless told not to", %{conn: conn} do + Oban.Testing.with_testing_mode(:manual, fn -> + quiet_id = Ecto.UUID.generate() + + put_project(conn, quiet_id, %{ + "name" => "quiet", + "members" => four_members(), + "notify" => false + }) + |> json_response(201) + + assert all_enqueued(worker: Lightning.Accounts.UserNotifier) == [] + + put_project(conn, Ecto.UUID.generate(), %{ + "name" => "loud", + "members" => four_members() + }) + |> json_response(201) + + assert length(all_enqueued(worker: Lightning.Accounts.UserNotifier)) == + 4 + end) + end + + test "matches a member's email without regard to case", %{conn: conn} do + owner = insert(:user, email: "mixed.case@example.com") + id = Ecto.UUID.generate() + + conn = + put_project(conn, id, %{ + "name" => "acme", + "members" => [ + %{"email" => "Mixed.CASE@example.com", "role" => "owner"} + ] + }) + + assert json_response(conn, 201) + assert members_of(id) == [{owner.email, :owner}] + end + + test "replaces the name and description and keeps the members", %{ + conn: conn + } do + owner = insert(:user) + + project = + insert(:project, + name: "before", + description: "old", + project_users: [%{user: owner, role: :owner}] + ) + + conn = + put_project(conn, project.id, %{ + "name" => "after", + "description" => "new", + "members" => [%{"email" => insert(:user).email, "role" => "owner"}] + }) + + response = json_response(conn, 200) + + assert response["data"]["attributes"] == %{ + "name" => "after", + "description" => "new", + "members" => [%{"email" => owner.email, "role" => "owner"}] + } + + assert %{name: "after", description: "new"} = + Lightning.Repo.reload!(project) + + assert members_of(project.id) == [{owner.email, :owner}] + end + + test "refuses a body id that differs from the path", %{conn: conn} do + conn = + put_project(conn, Ecto.UUID.generate(), %{ + "id" => Ecto.UUID.generate(), + "name" => "acme", + "members" => four_members() + }) + + assert %{"id" => [_]} = json_response(conn, 422)["errors"] + end + + test "refuses a key the body does not take, under that key", %{conn: conn} do + id = Ecto.UUID.generate() + + conn = + put_project(conn, id, %{ + "name" => "acme", + "members" => four_members(), + "requires_mfa" => true, + "retention_policy" => "erase_all" + }) + + assert json_response(conn, 422) == %{ + "errors" => %{ + "requires_mfa" => ["is not accepted"], + "retention_policy" => ["is not accepted"] + } + } + + refute Lightning.Repo.get(Lightning.Projects.Project, id) + end + + test "refuses a path id that is not a UUID", %{conn: conn} do + conn = + put_project(conn, "not-a-uuid", %{ + "name" => "acme", + "members" => four_members() + }) + + assert %{"id" => [_]} = json_response(conn, 422)["errors"] + end + + test "refuses members it cannot write, under members", %{conn: conn} do + [owner, admin | _] = four_members() + + refused = [ + unknown_email: [ + owner, + %{"email" => "nobody@example.com", "role" => "admin"} + ], + two_owners: [owner, %{admin | "role" => "owner"}], + no_owner: [admin], + no_members: [], + not_a_list: %{"email" => owner["email"], "role" => "owner"} + ] + + for {case_name, members} <- refused do + id = Ecto.UUID.generate() + + response = + conn + |> put_project(id, %{"name" => "acme", "members" => members}) + |> json_response(422) + + assert %{"members" => [_ | _] = messages} = response["errors"], + "#{case_name}: #{inspect(response)}" + + assert map_size(response["errors"]) == 1 and + Enum.all?(messages, &is_binary/1), + "#{case_name}: #{inspect(response)}" + + refute Enum.any?(messages, &String.contains?(&1, "@")), + "#{case_name} echoes an email: #{inspect(response)}" + + refute Lightning.Repo.get(Lightning.Projects.Project, id) + end + end + + test "lists a member the project refuses as a plain message under members", + %{conn: conn} do + # An extension refusing one member, the way a seat limit would. + Mox.expect(Lightning.Extensions.MockProjectHook, :handle_create_project, fn + attrs -> + changeset = + Lightning.Projects.Project.project_with_users_changeset( + %Lightning.Projects.Project{}, + attrs + ) + + [first | rest] = Ecto.Changeset.get_change(changeset, :project_users) + + changeset + |> Ecto.Changeset.put_change(:project_users, [ + first, + rest + |> hd() + |> Ecto.Changeset.add_error(:user_id, "has no seat left") + | tl(rest) + ]) + |> Ecto.Changeset.apply_action(:insert) + end) + + response = + conn + |> put_project(Ecto.UUID.generate(), %{ + "name" => "acme", + "members" => four_members() + }) + |> json_response(422) + + assert response["errors"] == %{"members" => ["has no seat left"]} + end + + test "refuses a notify of null, under notify", %{conn: conn} do + id = Ecto.UUID.generate() + + response = + conn + |> put_project(id, %{ + "name" => "acme", + "members" => four_members(), + "notify" => nil + }) + |> json_response(422) + + assert %{"notify" => [_]} = response["errors"] + refute Lightning.Repo.get(Lightning.Projects.Project, id) + end + + test "refuses a body that is not a JSON object", %{conn: conn} do + conn = put_project(conn, Ecto.UUID.generate(), [%{"name" => "acme"}]) + + assert %{"errors" => %{"body" => [_]}} = json_response(conn, 422) + end + + test "refuses a name or description the project cannot hold", %{conn: conn} do + conn_name = + put_project(conn, Ecto.UUID.generate(), %{ + "name" => "Not Allowed", + "members" => four_members() + }) + + assert Map.keys(json_response(conn_name, 422)["errors"]) == ["name"] + + conn_description = + put_project(conn, Ecto.UUID.generate(), %{ + "name" => "acme", + "description" => String.duplicate("a", 241), + "members" => four_members() + }) + + assert Map.keys(json_response(conn_description, 422)["errors"]) == [ + "description" + ] + end + + test "answers 409 for a project scheduled for deletion", %{conn: conn} do + project = scheduled_project() + + conn = put_project(conn, project.id, %{"name" => "renamed"}) + + assert json_response(conn, 409) == %{"error" => "scheduled_for_deletion"} + assert Lightning.Repo.reload!(project).name == project.name + end + end + + describe "PUT, for a service account without projects:write" do + test "is refused for insufficient scope, creating or replacing", %{ + conn: conn + } do + {conn, _account} = with_service_account(conn, ["users:write"]) + + for id <- [Ecto.UUID.generate(), insert(:project).id] do + refused = + put_project(conn, id, %{"name" => "acme", "members" => four_members()}) + + assert json_response(refused, 403) == %{"error" => "insufficient_scope"} + + assert get_resp_header(refused, "www-authenticate") == [ + ~s(Bearer error="insufficient_scope") + ] + end + end + end + + describe "PUT, for a person's token" do + test "a superuser creates a project", %{conn: conn} do + conn = with_pat(conn, insert(:user, role: :superuser)) + id = Ecto.UUID.generate() + + conn = + put_project(conn, id, %{"name" => "acme", "members" => four_members()}) + + assert %{"id" => ^id} = json_response(conn, 201)["data"] + end + + test "anyone else is refused creating one", %{conn: conn} do + conn = with_pat(conn, insert(:user)) + id = Ecto.UUID.generate() + + conn = + put_project(conn, id, %{"name" => "acme", "members" => four_members()}) + + assert json_response(conn, 403) == %{"error" => "Forbidden"} + refute Lightning.Repo.get(Lightning.Projects.Project, id) + end + + test "the owner or an admin replaces it; an editor, viewer or non-member is refused", + %{conn: conn} do + for {role, status} <- [ + owner: 200, + admin: 200, + editor: 403, + viewer: 403, + none: 403 + ] do + user = insert(:user) + + project_users = + if role == :none, do: [], else: [%{user: user, role: role}] + + project = insert(:project, name: "before", project_users: project_users) + + conn + |> with_pat(user) + |> put_project(project.id, %{"name" => "after"}) + |> json_response(status) + + expected = if status == 200, do: "after", else: "before" + + assert Lightning.Repo.reload!(project).name == expected, + "#{role} got #{status}" + end + end + + test "is refused with 403, not 409, on a project scheduled for deletion", + %{conn: conn} do + user = insert(:user) + project = scheduled_project([%{user: user, role: :owner}]) + + conn = + conn + |> with_pat(user) + |> put_project(project.id, %{"name" => "renamed"}) + + assert json_response(conn, 403) == %{"error" => "Forbidden"} + end + end end diff --git a/test/lightning_web/controllers/project_mfa_enforcement_test.exs b/test/lightning_web/controllers/project_mfa_enforcement_test.exs index 3fe12fad4a0..773fa9f8faf 100644 --- a/test/lightning_web/controllers/project_mfa_enforcement_test.exs +++ b/test/lightning_web/controllers/project_mfa_enforcement_test.exs @@ -285,7 +285,7 @@ defmodule LightningWeb.ProjectMFAEnforcementControllerTest do workflow = insert(:workflow, project: project) |> with_snapshot() for {path, refusals} <- [ - {~p"/api/projects/#{project.id}", [401, 403]}, + {~p"/api/projects/#{project.id}", [403]}, {~p"/api/projects/#{project.id}/workflows", [401, 403]}, {~p"/api/projects/#{project.id}/workflows/#{workflow.id}", [401, 403]}, diff --git a/test/lightning_web/controllers/token_exchange_controller_test.exs b/test/lightning_web/controllers/token_exchange_controller_test.exs index 7709b863719..1bddf7c246b 100644 --- a/test/lightning_web/controllers/token_exchange_controller_test.exs +++ b/test/lightning_web/controllers/token_exchange_controller_test.exs @@ -31,7 +31,8 @@ defmodule LightningWeb.TokenExchangeControllerTest do "grant_type" => "client_credentials", "client_assertion_type" => @assertion_type, "client_assertion" => - sign_assertion(private_key, assertion_claims(account, audience)) + sign_assertion(private_key, assertion_claims(account, audience)), + "scope" => "users:read" }, extra ) @@ -52,7 +53,11 @@ defmodule LightningWeb.TokenExchangeControllerTest do "grant_types_supported" => ["client_credentials"], "token_endpoint_auth_methods_supported" => ["private_key_jwt"], "token_endpoint_auth_signing_alg_values_supported" => ["RS256"], - "scopes_supported" => ["users:read", "users:write"], + "scopes_supported" => [ + "users:read", + "users:write", + "projects:write" + ], "response_types_supported" => [] } @@ -69,14 +74,18 @@ defmodule LightningWeb.TokenExchangeControllerTest do account: account, private_key: private_key } do - conn = - exchange(conn, valid_params(account, private_key, token_endpoint(conn))) + params = + valid_params(account, private_key, token_endpoint(conn), %{ + "scope" => "users:read users:write projects:write" + }) + + conn = exchange(conn, params) assert %{ "access_token" => access_token, "token_type" => "Bearer", "expires_in" => 300, - "scope" => "users:read users:write" + "scope" => "users:read users:write projects:write" } = json_response(conn, 200) assert get_resp_header(conn, "cache-control") == ["no-store"] @@ -95,7 +104,7 @@ defmodule LightningWeb.TokenExchangeControllerTest do "aud" => aud, "sub" => sub, "client_id" => sub, - "scope" => "users:read users:write", + "scope" => "users:read users:write projects:write", "iat" => iat, "exp" => exp, "jti" => jti @@ -166,6 +175,20 @@ defmodule LightningWeb.TokenExchangeControllerTest do conn |> exchange(params) |> json_response(400) end + test "refuses a request that names no scope with invalid_scope", %{ + conn: conn, + account: account, + private_key: private_key + } do + params = valid_params(account, private_key, token_endpoint(conn)) + + for params <- [Map.delete(params, "scope"), Map.put(params, "scope", " ")] do + assert conn |> exchange(params) |> json_response(400) == %{ + "error" => "invalid_scope" + } + end + end + test "refuses a failed assertion with invalid_client", %{ conn: conn, account: account, diff --git a/test/lightning_web/live/project_live_test.exs b/test/lightning_web/live/project_live_test.exs index d7a3cfb3504..f5cf5a4e1d0 100644 --- a/test/lightning_web/live/project_live_test.exs +++ b/test/lightning_web/live/project_live_test.exs @@ -153,16 +153,14 @@ defmodule LightningWeb.ProjectLiveTest do assert render(index_live) =~ "Project created successfully" project_name = String.replace(@create_attrs.raw_name, " ", "-") + subject = "You now have access to \"#{project_name}\"" - assert_email_sent( - to: [Swoosh.Email.Recipient.format(user_1)], - subject: "You now have access to \"#{project_name}\"" - ) + for user <- [user_1, user_2] do + recipient = Swoosh.Email.Recipient.format(user) - assert_email_sent( - to: [Swoosh.Email.Recipient.format(user_2)], - subject: "You now have access to \"#{project_name}\"" - ) + assert_received {:email, + %Swoosh.Email{to: [^recipient], subject: ^subject}} + end end test "project owners can delete a project from the settings page", diff --git a/test/lightning_web/plugs/access_token_auth_test.exs b/test/lightning_web/plugs/access_token_auth_test.exs index 5043976c93b..a18529f758d 100644 --- a/test/lightning_web/plugs/access_token_auth_test.exs +++ b/test/lightning_web/plugs/access_token_auth_test.exs @@ -39,7 +39,7 @@ defmodule LightningWeb.Plugs.AccessTokenAuthTest do conn.assigns.access_token assert sub == account.id - assert conn.assigns.service_account == account + assert conn.assigns.current_resource == account end test "refuses a token for a service account that is no longer registered", @@ -75,21 +75,9 @@ defmodule LightningWeb.Plugs.AccessTokenAuthTest do end test "refuses an expired access token", %{conn: conn, account: account} do - signer = Lightning.Config.token_signer() - now = System.system_time(:second) - - {_, token} = - signer.jwk - |> JOSE.JWT.sign(%{"alg" => "RS256", "typ" => "at+jwt"}, %{ - "iss" => AccessToken.issuer(), - "aud" => AccessToken.issuer() <> "/api", - "sub" => account.id, - "scope" => "users:read", - "exp" => now - 1 - }) - |> JOSE.JWS.compact() - - conn |> authenticate(token) |> assert_refused(401, "invalid_token") + conn + |> authenticate(expired_access_token(account, "users:read")) + |> assert_refused(401, "invalid_token") end test "refuses a token that is right in every way but its typ", %{ diff --git a/test/lightning_web/plugs/person_or_service_account_auth_test.exs b/test/lightning_web/plugs/person_or_service_account_auth_test.exs new file mode 100644 index 00000000000..9076a7ff33c --- /dev/null +++ b/test/lightning_web/plugs/person_or_service_account_auth_test.exs @@ -0,0 +1,124 @@ +defmodule LightningWeb.Plugs.PersonOrServiceAccountAuthTest do + use LightningWeb.ConnCase, async: true + + import Lightning.Factories + import Lightning.ServiceAccountHelpers + + alias Lightning.ServiceAccount.AccessToken + + setup %{conn: conn} do + {account, _private_key} = service_account_with_key() + Mox.stub(Lightning.MockConfig, :service_account, fn -> account end) + + %{ + conn: put_req_header(conn, "accept", "application/json"), + account: account + } + end + + defp with_token(conn, token), + do: put_req_header(conn, "authorization", "Bearer " <> token) + + defp with_scopes(conn, account, scopes), + do: with_token(conn, AccessToken.issue(account, scopes)) + + describe "GET /api/projects/:id" do + test "refuses a service account without projects:write with 403", %{ + conn: conn, + account: account + } do + project = insert(:project) + + conn = + conn + |> with_scopes(account, ["users:read", "users:write"]) + |> get(~p"/api/projects/#{project.id}") + + assert json_response(conn, 403) == %{"error" => "insufficient_scope"} + + assert get_resp_header(conn, "www-authenticate") == [ + ~s(Bearer error="insufficient_scope") + ] + end + + test "refuses an expired service-account token with 401 invalid_token", %{ + conn: conn, + account: account + } do + conn = + conn + |> with_token(expired_access_token(account, "projects:write")) + |> get(~p"/api/projects/#{insert(:project).id}") + + assert json_response(conn, 401) == %{"error" => "invalid_token"} + + assert get_resp_header(conn, "www-authenticate") == [ + ~s(Bearer error="invalid_token") + ] + end + + test "shows a project to a member's personal access token", %{conn: conn} do + user = insert(:user) + project = insert(:project, project_users: [%{user: user}]) + + conn = + conn + |> with_token(Lightning.Accounts.generate_api_token(user)) + |> get(~p"/api/projects/#{project.id}") + + assert %{"id" => id} = json_response(conn, 200)["data"] + assert id == project.id + end + + test "refuses a non-member's personal access token with 403", %{ + conn: conn + } do + user = insert(:user) + project = insert(:project) + + conn = + conn + |> with_token(Lightning.Accounts.generate_api_token(user)) + |> get(~p"/api/projects/#{project.id}") + + assert json_response(conn, 403) == %{"error" => "Forbidden"} + assert get_resp_header(conn, "www-authenticate") == [] + end + + test "refuses a garbage token with 401", %{conn: conn} do + conn = + conn + |> with_token("Oooops") + |> get(~p"/api/projects/#{insert(:project).id}") + + assert json_response(conn, 401) == %{"error" => "Unauthorized"} + end + + test "refuses a request without a token with 401", %{conn: conn} do + conn = get(conn, ~p"/api/projects/#{insert(:project).id}") + + assert json_response(conn, 401) == %{"error" => "Unauthorized"} + end + end + + describe "routes outside /api/projects/:id" do + setup %{conn: conn, account: account} do + %{conn: with_scopes(conn, account, AccessToken.scopes())} + end + + test "refuse a service account with 401", %{conn: conn} do + project = insert(:project) + + for {method, path} <- [ + {:post, ~p"/api/provision"}, + {:get, ~p"/api/projects"}, + {:get, ~p"/api/projects/#{project.id}/workflows"} + ] do + conn = dispatch(conn, @endpoint, method, path, %{}) + + assert json_response(conn, 401) == %{"error" => "Unauthorized"}, + "#{method} #{path}" + end + end + end +end diff --git a/test/support/projects_helpers.ex b/test/support/projects_helpers.ex index c4639ff05c6..0fff31caf2e 100644 --- a/test/support/projects_helpers.ex +++ b/test/support/projects_helpers.ex @@ -52,7 +52,7 @@ defmodule Lightning.ProjectsHelpers do @doc """ Turns off a project's support access as `actor`, returning the updated project. """ - def revoke_support_access(project, actor \\ nil) do + def revoke_support_access(project, actor) do {:ok, project} = Projects.update_project(project, %{allow_support_access: false}, actor) diff --git a/test/support/service_account_helpers.ex b/test/support/service_account_helpers.ex index 736349c31fa..09a1015aa18 100644 --- a/test/support/service_account_helpers.ex +++ b/test/support/service_account_helpers.ex @@ -2,6 +2,7 @@ defmodule Lightning.ServiceAccountHelpers do @moduledoc false alias Lightning.ServiceAccount + alias Lightning.ServiceAccount.AccessToken @doc """ A fresh key pair and the service account its public half registers. @@ -39,6 +40,24 @@ defmodule Lightning.ServiceAccountHelpers do token end + @doc """ + An access token for `account` carrying `scope`, signed as Lightning signs + one, that expired a second ago. + """ + def expired_access_token(%ServiceAccount{id: id}, scope) do + sign_assertion( + Lightning.Config.token_signer().jwk, + %{ + "iss" => AccessToken.issuer(), + "aud" => AccessToken.issuer() <> "/api", + "sub" => id, + "scope" => scope, + "exp" => System.system_time(:second) - 1 + }, + %{"alg" => "RS256", "typ" => "at+jwt"} + ) + end + @doc """ A token that claims `alg: none` and carries no signature. """