From 9e70e80539acf04f98a30bbd3ae9f1e6880a2670 Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 12:16:49 +0200 Subject: [PATCH 01/31] Drop the /commit command and turn off Claude attribution in settings Claude Code's own commit habits cover what the command said. The one rule it held that nothing else did, no Claude attribution on commits or PRs, now lives in a checked-in .claude/settings.json so the harness enforces it. --- .claude/commands/commit.md | 34 -------------------------- .claude/settings.json | 6 +++++ .claude/skills/implement-plan/SKILL.md | 2 +- 3 files changed, 7 insertions(+), 35 deletions(-) delete mode 100644 .claude/commands/commit.md create mode 100644 .claude/settings.json 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. From c9e67cfed98c180aa783c6d7ac038621a4df4c6c Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 14:03:45 +0200 Subject: [PATCH 02/31] Let service accounts with projects:write through GET /api/projects/:id's pipeline --- lib/lightning/service_account/access_token.ex | 2 +- .../controllers/api/project_controller.ex | 4 + lib/lightning_web/plugs/access_token_auth.ex | 25 +++- .../plugs/person_or_service_account_auth.ex | 45 ++++++ lib/lightning_web/router.ex | 14 +- .../token_exchange_controller_test.exs | 10 +- .../person_or_service_account_auth_test.exs | 135 ++++++++++++++++++ 7 files changed, 226 insertions(+), 9 deletions(-) create mode 100644 lib/lightning_web/plugs/person_or_service_account_auth.ex create mode 100644 test/lightning_web/plugs/person_or_service_account_auth_test.exs diff --git a/lib/lightning/service_account/access_token.ex b/lib/lightning/service_account/access_token.ex index 459e73c5fcc..e9d7f79be41 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" diff --git a/lib/lightning_web/controllers/api/project_controller.ex b/lib/lightning_web/controllers/api/project_controller.ex index f39db4da342..4359c468b54 100644 --- a/lib/lightning_web/controllers/api/project_controller.ex +++ b/lib/lightning_web/controllers/api/project_controller.ex @@ -37,8 +37,12 @@ defmodule LightningWeb.API.ProjectController do alias Lightning.Policies.ProjectUsers alias Lightning.Projects + import LightningWeb.Plugs.PersonOrServiceAccountAuth, only: [require_scope: 2] + action_fallback LightningWeb.FallbackController + plug :require_scope, "projects:write" when action == :show + @doc """ Lists all projects accessible to the authenticated user. diff --git a/lib/lightning_web/plugs/access_token_auth.ex b/lib/lightning_web/plugs/access_token_auth.ex index a62aa06686c..2cd4241a766 100644 --- a/lib/lightning_web/plugs/access_token_auth.ex +++ b/lib/lightning_web/plugs/access_token_auth.ex @@ -17,15 +17,32 @@ defmodule LightningWeb.Plugs.AccessTokenAuth do def init(opts), do: opts def call(conn, _opts) do + case service_account(conn) do + {:ok, service_account, claims} -> + conn + |> assign(:access_token, claims) + |> assign(:service_account, service_account) + + :error -> + refuse(conn, 401, "invalid_token") + end + end + + @doc """ + The service account a request's bearer token was issued to, with the token's + claims, or `:error` when the token is absent, invalid, or issued to an + account no longer registered. + """ + @spec service_account(Plug.Conn.t()) :: + {:ok, ServiceAccount.t(), map()} | :error + def service_account(conn) do with ["Bearer " <> token] <- get_req_header(conn, "authorization"), {:ok, %{"sub" => sub} = claims} <- AccessToken.verify(token), %ServiceAccount{id: ^sub} = service_account <- Lightning.Config.service_account() do - conn - |> assign(:access_token, claims) - |> assign(:service_account, service_account) + {:ok, service_account, claims} else - _other -> refuse(conn, 401, "invalid_token") + _other -> :error end 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..0f6ddbc2539 --- /dev/null +++ b/lib/lightning_web/plugs/person_or_service_account_auth.ex @@ -0,0 +1,45 @@ +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 valid service-account token assigns the service account as + `:current_resource` and the token's claims as `:access_token`. Any other + request gets exactly the answer `UserAuth.authenticate_bearer/2` and + `UserAuth.require_authenticated_api_resource/2` give it elsewhere. + + `require_scope/2` holds a service account to the scope a controller names. + Personal access tokens and repo connections carry no scopes, so it lets them + through and leaves the decision to the controller's policy. + """ + import Plug.Conn + + alias Lightning.ServiceAccount + alias LightningWeb.Plugs.AccessTokenAuth + alias LightningWeb.UserAuth + + def init(opts), do: opts + + def call(conn, _opts) do + case AccessTokenAuth.service_account(conn) do + {:ok, service_account, claims} -> + conn + |> assign(:current_resource, service_account) + |> assign(:access_token, claims) + + :error -> + conn + |> UserAuth.authenticate_bearer([]) + |> UserAuth.require_authenticated_api_resource([]) + end + end + + def require_scope( + %Plug.Conn{assigns: %{current_resource: %ServiceAccount{}}} = conn, + scope + ), + do: AccessTokenAuth.require_scope(conn, scope) + + def require_scope(conn, _scope), do: conn +end diff --git a/lib/lightning_web/router.ex b/lib/lightning_web/router.ex index 68175472317..aceff48ec22 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 :person_or_service_account_api do + plug :accepts, ["json"] + plug LightningWeb.Plugs.PersonOrServiceAccountAuth + end + pipeline :authenticated_api do plug :accepts, ["json"] plug LightningWeb.Plugs.ApiAuth @@ -101,6 +106,13 @@ 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 [:person_or_service_account_api] + + get "/projects/:id", API.ProjectController, :show + end + scope "/api", LightningWeb, as: :api do pipe_through [:api] @@ -114,7 +126,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/test/lightning_web/controllers/token_exchange_controller_test.exs b/test/lightning_web/controllers/token_exchange_controller_test.exs index 7709b863719..58e10c5b1ee 100644 --- a/test/lightning_web/controllers/token_exchange_controller_test.exs +++ b/test/lightning_web/controllers/token_exchange_controller_test.exs @@ -52,7 +52,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" => [] } @@ -76,7 +80,7 @@ defmodule LightningWeb.TokenExchangeControllerTest do "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 +99,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 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..d4f566ec112 --- /dev/null +++ b/test/lightning_web/plugs/person_or_service_account_auth_test.exs @@ -0,0 +1,135 @@ +defmodule LightningWeb.Plugs.PersonOrServiceAccountAuthTest do + use LightningWeb.ConnCase, async: true + + import Lightning.Factories + import Lightning.ServiceAccountHelpers + + alias Lightning.ServiceAccount.AccessToken + alias LightningWeb.Plugs.PersonOrServiceAccountAuth + + 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 "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 401", %{ + 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, 401) == %{"error" => "Unauthorized"} + 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 on POST /api/provision with 401", %{ + conn: conn + } do + conn = post(conn, ~p"/api/provision", %{}) + + assert json_response(conn, 401) == %{"error" => "Unauthorized"} + end + + test "refuse a service account on GET /api/projects with 401", %{ + conn: conn + } do + conn = get(conn, ~p"/api/projects") + + assert json_response(conn, 401) == %{"error" => "Unauthorized"} + end + + test "refuse a service account on GET /api/projects/:id/workflows with 401", + %{conn: conn} do + conn = get(conn, ~p"/api/projects/#{insert(:project).id}/workflows") + + assert json_response(conn, 401) == %{"error" => "Unauthorized"} + end + end + + # A repo connection on GET /api/projects/:id reaches ProjectUsers, which + # doesn't decide for one yet, so this exercises the plugs on their own. + describe "a repo connection token" do + test "is authenticated and passes require_scope", %{conn: conn} do + repo_connection = + insert(:project_repo_connection, project: insert(:project)) + + conn = + conn + |> with_token(repo_connection.access_token) + |> PersonOrServiceAccountAuth.call([]) + |> PersonOrServiceAccountAuth.require_scope("projects:write") + + refute conn.halted + assert conn.assigns.current_resource.id == repo_connection.id + end + end +end From d12ebc702c2ae574ae02cfb1ea98ab2102d9a63c Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 14:08:40 +0200 Subject: [PATCH 03/31] Give service accounts project standing in Scope and ProjectUsers --- lib/lightning/policies/project_users.ex | 21 ++++- lib/lightning/projects/scope.ex | 31 +++++++- .../project_user_permissions_test.exs | 77 +++++++++++++++++++ test/lightning/projects/scope_test.exs | 51 ++++++++++++ .../api/project_controller_test.exs | 53 +++++++++++++ .../person_or_service_account_auth_test.exs | 4 +- 6 files changed, 229 insertions(+), 8 deletions(-) diff --git a/lib/lightning/policies/project_users.ex b/lib/lightning/policies/project_users.ex index fe6e2f38eac..bfbd4c1c6ce 100644 --- a/lib/lightning/policies/project_users.ex +++ b/lib/lightning/policies/project_users.ex @@ -25,6 +25,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 +53,10 @@ defmodule Lightning.Policies.ProjectUsers do @other_actions [:access_project, :delete_project, :publish_template] + @service_account_actions @admin_actions ++ + @editor_actions ++ + [:access_project, :delete_project] + @type actions :: :access_project | :delete_project @@ -84,10 +89,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 +114,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 +138,14 @@ defmodule Lightning.Policies.ProjectUsers do # later — without anyone having to remember @admin_actions/@editor_actions. def permitted?(_action, %Scope{mfa_satisfied?: false}), do: false + # A service account holds no role; its authority is its token's scope, which + # the route has already checked. It gets every project action except the + # self actions, which act on the caller's own membership row (it has none), + # and :publish_template, which is for support staff rather than any project + # role. + 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/projects/scope.ex b/lib/lightning/projects/scope.ex index bcfeacd5cd3..d3f336bb513 100644 --- a/lib/lightning/projects/scope.ex +++ b/lib/lightning/projects/scope.ex @@ -25,6 +25,10 @@ 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 authority is its token's scope, checked at the route. 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 +54,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 +82,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 :: User.t() | ProjectRepoConnection.t() | ServiceAccount.t() @typedoc """ Anything that identifies a project: a loaded `%Project{}`, a project id, or @@ -107,6 +112,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 +134,13 @@ defmodule Lightning.Projects.Scope do end end + def fetch(%ServiceAccount{} = account, subject) do + with {:ok, project} <- resolve_project(subject), + :ok <- still_operable(project) do + {:ok, build(account, project)} + end + end + @doc """ Whether the user holds one of `roles` in the project. @@ -214,6 +228,19 @@ defmodule Lightning.Projects.Scope do } end + # No role, because it holds no membership row: what it may do is decided by + # its token's scope at the route, and `ProjectUsers` answers for it on that + # basis. MFA is exempted explicitly for the same reason as a repo connection + # above: a machine credential cannot enrol. + defp build(%ServiceAccount{} = account, %Project{} = project) do + %__MODULE__{ + project: project, + actor: account, + role: nil, + mfa_satisfied?: true + } + end + # Only an enrolled `true` counts; an unset flag is not enrolled. # # A support user is a human reading project data, so the rule binds them as diff --git a/test/lightning/policies/project_user_permissions_test.exs b/test/lightning/policies/project_user_permissions_test.exs index 3df44ea5aa1..ed2ee5ff0ed 100644 --- a/test/lightning/policies/project_user_permissions_test.exs +++ b/test/lightning/policies/project_user_permissions_test.exs @@ -457,6 +457,83 @@ defmodule Lightning.Policies.ProjectUserPermissionsTest do end end + # A service account holds no membership row; its authority is its token's + # scope, checked at the route. 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 + + 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 + + test "refuses every action on a project scheduled for deletion", %{ + account: account, + marked_project: marked_project + } do + assert :access_project in ProjectUsers.actions() + refute_can(ProjectUsers, ProjectUsers.actions(), account, marked_project) + end + end + + describe "a repo connection" do + # Reached from GET /api/projects/:id; it has no role, so it is refused + # rather than crashing the request. + test "is refused :access_project on its own project", %{project: project} do + repo_connection = insert(:project_repo_connection, project: project) + + refute_can(ProjectUsers, :access_project, repo_connection, project) + end + end + describe "blocked_by_mfa?/1" do setup %{editor: editor} do enrolled = insert(:user, mfa_enabled: true) 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_web/controllers/api/project_controller_test.exs b/test/lightning_web/controllers/api/project_controller_test.exs index f5a9dd0a661..026b9a557c6 100644 --- a/test/lightning_web/controllers/api/project_controller_test.exs +++ b/test/lightning_web/controllers/api/project_controller_test.exs @@ -2,6 +2,7 @@ defmodule LightningWeb.API.ProjectControllerTest do use LightningWeb.ConnCase, async: true import Lightning.Factories + import Lightning.ServiceAccountHelpers setup %{conn: conn} do {:ok, conn: put_req_header(conn, "accept", "application/json")} @@ -155,4 +156,56 @@ defmodule LightningWeb.API.ProjectControllerTest do } end end + + describe "show, for a service account with projects:write" do + setup %{conn: conn} do + {account, _private_key} = service_account_with_key() + Mox.stub(Lightning.MockConfig, :service_account, fn -> account end) + + token = + Lightning.ServiceAccount.AccessToken.issue(account, ["projects:write"]) + + %{conn: put_req_header(conn, "authorization", "Bearer " <> token)} + end + + test "shows a project it is not a member of", %{conn: conn} do + project = insert(:project) + + conn = get(conn, ~p"/api/projects/#{project.id}") + + assert %{"id" => id, "attributes" => %{"name" => name}} = + json_response(conn, 200)["data"] + + assert id == project.id + assert name == project.name + end + + test "is refused a project scheduled for deletion with 401", %{conn: conn} do + project = + insert(:project, + scheduled_deletion: DateTime.utc_now() |> DateTime.truncate(:second) + ) + + conn = get(conn, ~p"/api/projects/#{project.id}") + + assert json_response(conn, 401) == %{"error" => "Unauthorized"} + end + end + + describe "show, for a repo connection" do + test "is refused its own project with 401", %{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, 401) == %{"error" => "Unauthorized"} + end + end end 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 index d4f566ec112..66ed027454a 100644 --- a/test/lightning_web/plugs/person_or_service_account_auth_test.exs +++ b/test/lightning_web/plugs/person_or_service_account_auth_test.exs @@ -115,8 +115,8 @@ defmodule LightningWeb.Plugs.PersonOrServiceAccountAuthTest do end end - # A repo connection on GET /api/projects/:id reaches ProjectUsers, which - # doesn't decide for one yet, so this exercises the plugs on their own. + # The plugs on their own: what the policy then answers is pinned in + # project_controller_test.exs. describe "a repo connection token" do test "is authenticated and passes require_scope", %{conn: conn} do repo_connection = From 5b016985f2565cfd8895576195ea47811610cfb4 Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 14:11:35 +0200 Subject: [PATCH 04/31] Make the service-account permissions test classify every ProjectUsers action --- test/lightning/policies/project_user_permissions_test.exs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/lightning/policies/project_user_permissions_test.exs b/test/lightning/policies/project_user_permissions_test.exs index ed2ee5ff0ed..3025d898f55 100644 --- a/test/lightning/policies/project_user_permissions_test.exs +++ b/test/lightning/policies/project_user_permissions_test.exs @@ -493,6 +493,12 @@ defmodule Lightning.Policies.ProjectUserPermissionsTest do 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), From ef4c6ab09c55401e9c789cee8c1bc6ee3ea3f49d Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 14:15:20 +0200 Subject: [PATCH 05/31] Refuse a failing service-account token with invalid_token on the person-or-service-account pipeline --- lib/lightning/policies/project_users.ex | 12 +++--- lib/lightning/service_account/access_token.ex | 11 ++++++ .../controllers/api/user_controller.ex | 4 +- lib/lightning_web/plugs/access_token_auth.ex | 38 ++++++++++++++----- .../plugs/person_or_service_account_auth.ex | 10 +++-- .../project_user_permissions_test.exs | 4 +- .../plugs/access_token_auth_test.exs | 2 +- .../person_or_service_account_auth_test.exs | 27 +++++++++++++ 8 files changed, 85 insertions(+), 23 deletions(-) diff --git a/lib/lightning/policies/project_users.ex b/lib/lightning/policies/project_users.ex index bfbd4c1c6ce..1752c0a1db5 100644 --- a/lib/lightning/policies/project_users.ex +++ b/lib/lightning/policies/project_users.ex @@ -10,13 +10,15 @@ defmodule Lightning.Policies.ProjectUsers do standing in a wound-down project. `permitted?/2` makes the **judgement**: given that standing, is this action - allowed. It decides on `role` and `mfa_satisfied?`, and never mentions - `Project.scheduled_deletion` or `Project.requires_mfa` — there is no - shut-down project left for it to see, and the MFA rule reaches it as a fact - on the scope. + allowed. For a person it decides on `role` and `mfa_satisfied?`; for a + service account it decides on `actor`, whose authority is the scope its token + carries, checked at the route. It never mentions `Project.scheduled_deletion` + or `Project.requires_mfa` — there is no shut-down project left for it to see, + and the MFA rule reaches it as a fact on the scope. Adding an action means adding an atom to `@admin_actions` or - `@editor_actions`. It inherits the guard; there is nothing to remember. + `@editor_actions`. It inherits the guard, and the service account is granted + it too, so decide whether it should be. We deny by default: an action in none of the lists is refused. """ diff --git a/lib/lightning/service_account/access_token.ex b/lib/lightning/service_account/access_token.ex index e9d7f79be41..1417bdd32e3 100644 --- a/lib/lightning/service_account/access_token.ex +++ b/lib/lightning/service_account/access_token.ex @@ -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_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/plugs/access_token_auth.ex b/lib/lightning_web/plugs/access_token_auth.ex index 2cd4241a766..98c3b1c0b92 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 @@ -21,28 +22,41 @@ defmodule LightningWeb.Plugs.AccessTokenAuth do {:ok, service_account, claims} -> conn |> assign(:access_token, claims) - |> assign(:service_account, service_account) + |> assign(:current_resource, service_account) - :error -> + _refused -> refuse(conn, 401, "invalid_token") end end @doc """ The service account a request's bearer token was issued to, with the token's - claims, or `:error` when the token is absent, invalid, or issued to an - account no longer registered. + claims. + + `{:error, :invalid_token}` when the token is an access token by its `typ` but + fails verification or names an account no longer registered, and + `:not_an_access_token` when there is no bearer token or it is some other kind. """ @spec service_account(Plug.Conn.t()) :: - {:ok, ServiceAccount.t(), map()} | :error + {:ok, ServiceAccount.t(), map()} + | {:error, :invalid_token} + | :not_an_access_token def service_account(conn) do with ["Bearer " <> token] <- get_req_header(conn, "authorization"), - {:ok, %{"sub" => sub} = claims} <- AccessToken.verify(token), + true <- AccessToken.access_token?(token) do + verify(token) + else + _other -> :not_an_access_token + end + end + + defp verify(token) do + with {:ok, %{"sub" => sub} = claims} <- AccessToken.verify(token), %ServiceAccount{id: ^sub} = service_account <- Lightning.Config.service_account() do {:ok, service_account, claims} else - _other -> :error + _other -> {:error, :invalid_token} end end @@ -52,7 +66,11 @@ defmodule LightningWeb.Plugs.AccessTokenAuth do else: refuse(conn, 403, "insufficient_scope") end - defp refuse(conn, status, error) do + @doc """ + Halts `conn` with an RFC 6750 §3 error response. + """ + @spec refuse(Plug.Conn.t(), 401 | 403, String.t()) :: Plug.Conn.t() + def refuse(conn, status, error) do conn |> put_resp_header("www-authenticate", ~s(Bearer error="#{error}")) |> put_status(status) diff --git a/lib/lightning_web/plugs/person_or_service_account_auth.ex b/lib/lightning_web/plugs/person_or_service_account_auth.ex index 0f6ddbc2539..86bfc445a1c 100644 --- a/lib/lightning_web/plugs/person_or_service_account_auth.ex +++ b/lib/lightning_web/plugs/person_or_service_account_auth.ex @@ -5,8 +5,9 @@ defmodule LightningWeb.Plugs.PersonOrServiceAccountAuth do repo connection token). A valid service-account token assigns the service account as - `:current_resource` and the token's claims as `:access_token`. Any other - request gets exactly the answer `UserAuth.authenticate_bearer/2` and + `:current_resource` and the token's claims as `:access_token`; a + service-account token that fails verification is refused as RFC 6750 §3 + says. Any other request gets exactly the answer `UserAuth.authenticate_bearer/2` and `UserAuth.require_authenticated_api_resource/2` give it elsewhere. `require_scope/2` holds a service account to the scope a controller names. @@ -28,7 +29,10 @@ defmodule LightningWeb.Plugs.PersonOrServiceAccountAuth do |> assign(:current_resource, service_account) |> assign(:access_token, claims) - :error -> + {:error, :invalid_token} -> + AccessTokenAuth.refuse(conn, 401, "invalid_token") + + :not_an_access_token -> conn |> UserAuth.authenticate_bearer([]) |> UserAuth.require_authenticated_api_resource([]) diff --git a/test/lightning/policies/project_user_permissions_test.exs b/test/lightning/policies/project_user_permissions_test.exs index 3025d898f55..c1128402030 100644 --- a/test/lightning/policies/project_user_permissions_test.exs +++ b/test/lightning/policies/project_user_permissions_test.exs @@ -531,8 +531,8 @@ defmodule Lightning.Policies.ProjectUserPermissionsTest do end describe "a repo connection" do - # Reached from GET /api/projects/:id; it has no role, so it is refused - # rather than crashing the request. + # Reached from GET /api/projects/:id; it has no role in the project, so + # the policy refuses it. test "is refused :access_project on its own project", %{project: project} do repo_connection = insert(:project_repo_connection, project: project) diff --git a/test/lightning_web/plugs/access_token_auth_test.exs b/test/lightning_web/plugs/access_token_auth_test.exs index 5043976c93b..9fc38e4fa6c 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", 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 index 66ed027454a..ad84868f90f 100644 --- a/test/lightning_web/plugs/person_or_service_account_auth_test.exs +++ b/test/lightning_web/plugs/person_or_service_account_auth_test.exs @@ -42,6 +42,33 @@ defmodule LightningWeb.Plugs.PersonOrServiceAccountAuthTest do ] end + test "refuses an expired service-account token with 401 invalid_token", %{ + conn: conn, + account: account + } do + {_, token} = + Lightning.Config.token_signer().jwk + |> JOSE.JWT.sign(%{"alg" => "RS256", "typ" => "at+jwt"}, %{ + "iss" => AccessToken.issuer(), + "aud" => AccessToken.issuer() <> "/api", + "sub" => account.id, + "scope" => "projects:write", + "exp" => System.system_time(:second) - 1 + }) + |> JOSE.JWS.compact() + + conn = + conn + |> with_token(token) + |> 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}]) From 341cd39c230a97f83ea3c54b84a8a34d4080fcda Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 14:19:35 +0200 Subject: [PATCH 06/31] Declare the projects:write scope on the :projects_api pipeline and restore AccessTokenAuth's single with --- .../controllers/api/project_controller.ex | 4 -- lib/lightning_web/plugs/access_token_auth.ex | 46 ++------------ .../plugs/person_or_service_account_auth.ex | 48 ++++++-------- lib/lightning_web/router.ex | 6 +- .../plugs/access_token_auth_test.exs | 18 +----- .../person_or_service_account_auth_test.exs | 62 ++++--------------- test/support/service_account_helpers.ex | 19 ++++++ 7 files changed, 62 insertions(+), 141 deletions(-) diff --git a/lib/lightning_web/controllers/api/project_controller.ex b/lib/lightning_web/controllers/api/project_controller.ex index 4359c468b54..f39db4da342 100644 --- a/lib/lightning_web/controllers/api/project_controller.ex +++ b/lib/lightning_web/controllers/api/project_controller.ex @@ -37,12 +37,8 @@ defmodule LightningWeb.API.ProjectController do alias Lightning.Policies.ProjectUsers alias Lightning.Projects - import LightningWeb.Plugs.PersonOrServiceAccountAuth, only: [require_scope: 2] - action_fallback LightningWeb.FallbackController - plug :require_scope, "projects:write" when action == :show - @doc """ Lists all projects accessible to the authenticated user. diff --git a/lib/lightning_web/plugs/access_token_auth.ex b/lib/lightning_web/plugs/access_token_auth.ex index 98c3b1c0b92..b69cc4d5424 100644 --- a/lib/lightning_web/plugs/access_token_auth.ex +++ b/lib/lightning_web/plugs/access_token_auth.ex @@ -18,45 +18,15 @@ defmodule LightningWeb.Plugs.AccessTokenAuth do def init(opts), do: opts def call(conn, _opts) do - case service_account(conn) do - {:ok, service_account, claims} -> - conn - |> assign(:access_token, claims) - |> assign(:current_resource, service_account) - - _refused -> - refuse(conn, 401, "invalid_token") - end - end - - @doc """ - The service account a request's bearer token was issued to, with the token's - claims. - - `{:error, :invalid_token}` when the token is an access token by its `typ` but - fails verification or names an account no longer registered, and - `:not_an_access_token` when there is no bearer token or it is some other kind. - """ - @spec service_account(Plug.Conn.t()) :: - {:ok, ServiceAccount.t(), map()} - | {:error, :invalid_token} - | :not_an_access_token - def service_account(conn) do with ["Bearer " <> token] <- get_req_header(conn, "authorization"), - true <- AccessToken.access_token?(token) do - verify(token) - else - _other -> :not_an_access_token - end - end - - defp verify(token) do - with {:ok, %{"sub" => sub} = claims} <- AccessToken.verify(token), + {:ok, %{"sub" => sub} = claims} <- AccessToken.verify(token), %ServiceAccount{id: ^sub} = service_account <- Lightning.Config.service_account() do - {:ok, service_account, claims} + conn + |> assign(:access_token, claims) + |> assign(:current_resource, service_account) else - _other -> {:error, :invalid_token} + _other -> refuse(conn, 401, "invalid_token") end end @@ -66,11 +36,7 @@ defmodule LightningWeb.Plugs.AccessTokenAuth do else: refuse(conn, 403, "insufficient_scope") end - @doc """ - Halts `conn` with an RFC 6750 §3 error response. - """ - @spec refuse(Plug.Conn.t(), 401 | 403, String.t()) :: Plug.Conn.t() - def refuse(conn, status, error) do + defp refuse(conn, status, error) do conn |> put_resp_header("www-authenticate", ~s(Bearer error="#{error}")) |> put_status(status) diff --git a/lib/lightning_web/plugs/person_or_service_account_auth.ex b/lib/lightning_web/plugs/person_or_service_account_auth.ex index 86bfc445a1c..ecfc2b91ad6 100644 --- a/lib/lightning_web/plugs/person_or_service_account_auth.ex +++ b/lib/lightning_web/plugs/person_or_service_account_auth.ex @@ -4,46 +4,36 @@ defmodule LightningWeb.Plugs.PersonOrServiceAccountAuth do bearer tokens `LightningWeb.UserAuth` accepts (a personal access token or a repo connection token). - A valid service-account token assigns the service account as - `:current_resource` and the token's claims as `:access_token`; a - service-account token that fails verification is refused as RFC 6750 §3 - says. Any other request gets exactly the answer `UserAuth.authenticate_bearer/2` and + 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. - `require_scope/2` holds a service account to the scope a controller names. - Personal access tokens and repo connections carry no scopes, so it lets them - through and leaves the decision to the controller's policy. - """ - import Plug.Conn + A resource opts in by adding a router pipeline that names its scope: + + plug LightningWeb.Plugs.PersonOrServiceAccountAuth, scope: "projects:write" - alias Lightning.ServiceAccount + 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: opts + def init(opts), do: Keyword.fetch!(opts, :scope) - def call(conn, _opts) do - case AccessTokenAuth.service_account(conn) do - {:ok, service_account, claims} -> - conn - |> assign(:current_resource, service_account) - |> assign(:access_token, claims) + def call(conn, scope) do + with {:ok, token} <- UserAuth.get_bearer(conn), + true <- AccessToken.access_token?(token) do + conn = AccessTokenAuth.call(conn, []) - {:error, :invalid_token} -> - AccessTokenAuth.refuse(conn, 401, "invalid_token") - - :not_an_access_token -> + if conn.halted, do: conn, else: AccessTokenAuth.require_scope(conn, scope) + else + _person -> conn |> UserAuth.authenticate_bearer([]) |> UserAuth.require_authenticated_api_resource([]) end end - - def require_scope( - %Plug.Conn{assigns: %{current_resource: %ServiceAccount{}}} = conn, - scope - ), - do: AccessTokenAuth.require_scope(conn, scope) - - def require_scope(conn, _scope), do: conn end diff --git a/lib/lightning_web/router.ex b/lib/lightning_web/router.ex index aceff48ec22..ffd5a1efe3e 100644 --- a/lib/lightning_web/router.ex +++ b/lib/lightning_web/router.ex @@ -51,9 +51,9 @@ defmodule LightningWeb.Router do plug LightningWeb.Plugs.AccessTokenAuth end - pipeline :person_or_service_account_api do + pipeline :projects_api do plug :accepts, ["json"] - plug LightningWeb.Plugs.PersonOrServiceAccountAuth + plug LightningWeb.Plugs.PersonOrServiceAccountAuth, scope: "projects:write" end pipeline :authenticated_api do @@ -108,7 +108,7 @@ defmodule LightningWeb.Router do ## A service account's access token or a person's bearer token scope "/api", LightningWeb, as: :api do - pipe_through [:person_or_service_account_api] + pipe_through [:projects_api] get "/projects/:id", API.ProjectController, :show end diff --git a/test/lightning_web/plugs/access_token_auth_test.exs b/test/lightning_web/plugs/access_token_auth_test.exs index 9fc38e4fa6c..a18529f758d 100644 --- a/test/lightning_web/plugs/access_token_auth_test.exs +++ b/test/lightning_web/plugs/access_token_auth_test.exs @@ -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 index ad84868f90f..9782811ea98 100644 --- a/test/lightning_web/plugs/person_or_service_account_auth_test.exs +++ b/test/lightning_web/plugs/person_or_service_account_auth_test.exs @@ -5,7 +5,6 @@ defmodule LightningWeb.Plugs.PersonOrServiceAccountAuthTest do import Lightning.ServiceAccountHelpers alias Lightning.ServiceAccount.AccessToken - alias LightningWeb.Plugs.PersonOrServiceAccountAuth setup %{conn: conn} do {account, _private_key} = service_account_with_key() @@ -46,20 +45,9 @@ defmodule LightningWeb.Plugs.PersonOrServiceAccountAuthTest do conn: conn, account: account } do - {_, token} = - Lightning.Config.token_signer().jwk - |> JOSE.JWT.sign(%{"alg" => "RS256", "typ" => "at+jwt"}, %{ - "iss" => AccessToken.issuer(), - "aud" => AccessToken.issuer() <> "/api", - "sub" => account.id, - "scope" => "projects:write", - "exp" => System.system_time(:second) - 1 - }) - |> JOSE.JWS.compact() - conn = conn - |> with_token(token) + |> with_token(expired_access_token(account, "projects:write")) |> get(~p"/api/projects/#{insert(:project).id}") assert json_response(conn, 401) == %{"error" => "invalid_token"} @@ -118,45 +106,19 @@ defmodule LightningWeb.Plugs.PersonOrServiceAccountAuthTest do %{conn: with_scopes(conn, account, AccessToken.scopes())} end - test "refuse a service account on POST /api/provision with 401", %{ - conn: conn - } do - conn = post(conn, ~p"/api/provision", %{}) - - assert json_response(conn, 401) == %{"error" => "Unauthorized"} - end - - test "refuse a service account on GET /api/projects with 401", %{ - conn: conn - } do - conn = get(conn, ~p"/api/projects") - - assert json_response(conn, 401) == %{"error" => "Unauthorized"} - end - - test "refuse a service account on GET /api/projects/:id/workflows with 401", - %{conn: conn} do - conn = get(conn, ~p"/api/projects/#{insert(:project).id}/workflows") - - assert json_response(conn, 401) == %{"error" => "Unauthorized"} - end - end - - # The plugs on their own: what the policy then answers is pinned in - # project_controller_test.exs. - describe "a repo connection token" do - test "is authenticated and passes require_scope", %{conn: conn} do - repo_connection = - insert(:project_repo_connection, project: insert(:project)) + test "refuse a service account with 401", %{conn: conn} do + project = insert(:project) - conn = - conn - |> with_token(repo_connection.access_token) - |> PersonOrServiceAccountAuth.call([]) - |> PersonOrServiceAccountAuth.require_scope("projects:write") + 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, %{}) - refute conn.halted - assert conn.assigns.current_resource.id == repo_connection.id + assert json_response(conn, 401) == %{"error" => "Unauthorized"}, + "#{method} #{path}" + end end end end 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. """ From d2b650eb58fc35072394179abc983f82e35ba42d Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 14:19:37 +0200 Subject: [PATCH 07/31] Write out the service account's allowed project actions and inline its scope build --- lib/lightning/policies/project_users.ex | 46 +++++++++++++------ lib/lightning/projects/scope.ex | 21 ++------- .../project_user_permissions_test.exs | 24 +--------- 3 files changed, 37 insertions(+), 54 deletions(-) diff --git a/lib/lightning/policies/project_users.ex b/lib/lightning/policies/project_users.ex index 1752c0a1db5..d7b3e8c43ef 100644 --- a/lib/lightning/policies/project_users.ex +++ b/lib/lightning/policies/project_users.ex @@ -10,15 +10,19 @@ defmodule Lightning.Policies.ProjectUsers do standing in a wound-down project. `permitted?/2` makes the **judgement**: given that standing, is this action - allowed. For a person it decides on `role` and `mfa_satisfied?`; for a - service account it decides on `actor`, whose authority is the scope its token - carries, checked at the route. It never mentions `Project.scheduled_deletion` - or `Project.requires_mfa` — there is no shut-down project left for it to see, - and the MFA rule reaches it as a fact on the scope. + allowed. It decides on `role` and `mfa_satisfied?`, and never mentions + `Project.scheduled_deletion` or `Project.requires_mfa` — there is no + shut-down project left for it to see, and the MFA rule reaches it as a fact + on the scope. Adding an action means adding an atom to `@admin_actions` or - `@editor_actions`. It inherits the guard, and the service account is granted - it too, so decide whether it should be. + `@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. """ @@ -55,9 +59,26 @@ defmodule Lightning.Policies.ProjectUsers do @other_actions [:access_project, :delete_project, :publish_template] - @service_account_actions @admin_actions ++ - @editor_actions ++ - [:access_project, :delete_project] + @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 @@ -140,11 +161,6 @@ defmodule Lightning.Policies.ProjectUsers do # later — without anyone having to remember @admin_actions/@editor_actions. def permitted?(_action, %Scope{mfa_satisfied?: false}), do: false - # A service account holds no role; its authority is its token's scope, which - # the route has already checked. It gets every project action except the - # self actions, which act on the caller's own membership row (it has none), - # and :publish_template, which is for support staff rather than any project - # role. def permitted?(action, %Scope{actor: %ServiceAccount{}}), do: action in @service_account_actions diff --git a/lib/lightning/projects/scope.ex b/lib/lightning/projects/scope.ex index d3f336bb513..ab9ead79a26 100644 --- a/lib/lightning/projects/scope.ex +++ b/lib/lightning/projects/scope.ex @@ -25,9 +25,8 @@ 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 authority is its token's scope, checked at the route. Its `role` - is always `nil`. + 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. @@ -137,7 +136,8 @@ defmodule Lightning.Projects.Scope do def fetch(%ServiceAccount{} = account, subject) do with {:ok, project} <- resolve_project(subject), :ok <- still_operable(project) do - {:ok, build(account, project)} + # Set rather than defaulted: a machine credential cannot enrol in MFA. + {:ok, %__MODULE__{project: project, actor: account, mfa_satisfied?: true}} end end @@ -228,19 +228,6 @@ defmodule Lightning.Projects.Scope do } end - # No role, because it holds no membership row: what it may do is decided by - # its token's scope at the route, and `ProjectUsers` answers for it on that - # basis. MFA is exempted explicitly for the same reason as a repo connection - # above: a machine credential cannot enrol. - defp build(%ServiceAccount{} = account, %Project{} = project) do - %__MODULE__{ - project: project, - actor: account, - role: nil, - mfa_satisfied?: true - } - end - # Only an enrolled `true` counts; an unset flag is not enrolled. # # A support user is a human reading project data, so the rule binds them as diff --git a/test/lightning/policies/project_user_permissions_test.exs b/test/lightning/policies/project_user_permissions_test.exs index c1128402030..548d6b6af58 100644 --- a/test/lightning/policies/project_user_permissions_test.exs +++ b/test/lightning/policies/project_user_permissions_test.exs @@ -457,10 +457,8 @@ defmodule Lightning.Policies.ProjectUserPermissionsTest do end end - # A service account holds no membership row; its authority is its token's - # scope, checked at the route. 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. + # 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} = @@ -520,24 +518,6 @@ defmodule Lightning.Policies.ProjectUserPermissionsTest do hd(project.project_users) ) end - - test "refuses every action on a project scheduled for deletion", %{ - account: account, - marked_project: marked_project - } do - assert :access_project in ProjectUsers.actions() - refute_can(ProjectUsers, ProjectUsers.actions(), account, marked_project) - end - end - - describe "a repo connection" do - # Reached from GET /api/projects/:id; it has no role in the project, so - # the policy refuses it. - test "is refused :access_project on its own project", %{project: project} do - repo_connection = insert(:project_repo_connection, project: project) - - refute_can(ProjectUsers, :access_project, repo_connection, project) - end end describe "blocked_by_mfa?/1" do From d124eb3b59d4ba5d1e17086014b432ac04b00dd9 Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 15:40:54 +0200 Subject: [PATCH 08/31] Add Lightning.Actor, the person or service account a context function records --- lib/lightning/actor.ex | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 lib/lightning/actor.ex 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 From 5eb8fcb01472392e24ae4e27af548ee941544f79 Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 15:43:08 +0200 Subject: [PATCH 09/31] Record who created a project and added its first members, and take notify as an option --- lib/lightning/projects.ex | 38 +++-- lib/lightning/projects/audit.ex | 22 +++ lib/lightning/projects/sandboxes.ex | 6 +- lib/lightning/setup_utils.ex | 14 +- .../dashboard_live/project_creation_modal.ex | 2 +- .../live/project_live/form_component.ex | 2 +- priv/bench/collections.exs | 53 +++++-- priv/bench/collections_put_all.exs | 84 ++++++---- test/lightning/projects_test.exs | 146 +++++++++++++++--- 9 files changed, 288 insertions(+), 79 deletions(-) diff --git a/lib/lightning/projects.ex b/lib/lightning/projects.ex index 39a4baef74e..ca0e62a791a 100644 --- a/lib/lightning/projects.ex +++ b/lib/lightning/projects.ex @@ -548,21 +548,35 @@ defmodule Lightning.Projects do 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 + project = Repo.preload(project, :project_users) + + {:ok, _} = + Multi.new() + |> Audit.derive_creation_events(project, actor) + |> Repo.transaction() + + if Keyword.get(opts, :notify, true) do schedule_project_addition_emails(%Project{project_users: []}, project) end @@ -2257,10 +2271,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 +2288,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..8496d7aec1c 100644 --- a/lib/lightning/projects/audit.ex +++ b/lib/lightning/projects/audit.ex @@ -10,12 +10,14 @@ defmodule Lightning.Projects.Audit do "collaborator_added", "collaborator_removed", "collaborator_role_changed", + "created", "dataclip_retention_period_updated", "history_retention_period_updated", "requires_mfa_updated" ] alias Ecto.Multi + alias Lightning.Projects.Project def derive_events(multi, changeset, user) do [ @@ -38,6 +40,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/sandboxes.ex b/lib/lightning/projects/sandboxes.ex index 2bfeb556f47..cefae951c79 100644 --- a/lib/lightning/projects/sandboxes.ex +++ b/lib/lightning/projects/sandboxes.ex @@ -1027,7 +1027,7 @@ defmodule Lightning.Projects.Sandboxes do sandbox_env ) - case create_empty_sandbox(parent_with_data, sandbox_attrs) do + case create_empty_sandbox(parent_with_data, sandbox_attrs, actor) do {:ok, sandbox} -> sandbox |> Repo.preload(:project_users) @@ -1083,8 +1083,8 @@ defmodule Lightning.Projects.Sandboxes do }) end - defp create_empty_sandbox(parent, attrs) do - Lightning.Projects.create_sandbox(parent, attrs, false) + defp create_empty_sandbox(parent, attrs, actor) do + Lightning.Projects.create_sandbox(parent, attrs, actor) end defp clone_credentials_from_parent(sandbox, parent) do 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/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/priv/bench/collections.exs b/priv/bench/collections.exs index 8cdd1b321dd..770e007ef64 100644 --- a/priv/bench/collections.exs +++ b/priv/bench/collections.exs @@ -9,13 +9,22 @@ 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 {:ok, collection} = with {:error, :not_found} <- Collections.get_collection("benchee") do - Collections.create_collection(%{"project_id" => project.id, "name" => "benchee"}) + Collections.create_collection(%{ + "project_id" => project.id, + "name" => "benchee" + }) end IO.puts("\n### Setup:") @@ -23,14 +32,17 @@ IO.puts("Generating items for benchee collection...") record = fn prefix, i -> i_str = String.pad_leading(to_string(i), 5, "0") - {"#{prefix}:foo#{i_str}:bar#{i_str}", Jason.encode!(%{fieldA: "value#{1_000_000 + i}"})} + + {"#{prefix}:foo#{i_str}:bar#{i_str}", + Jason.encode!(%{fieldA: "value#{1_000_000 + i}"})} end sampleA = Enum.map(1..keys_count, fn i -> record.("keyA", i) end) sampleB = Enum.map(1..keys_count, fn i -> record.("keyB", i) end) sampleC1 = Enum.map(1..keys_count, fn i -> record.("keyC", i) end) + sampleC2 = - keys_count..keys_count * 2 + keys_count..(keys_count * 2) |> Enum.map(fn i -> record.("keyC", i) end) |> Enum.map(fn {k, v} -> %{"key" => k, "value" => v} end) @@ -42,6 +54,7 @@ sampleC2 = if rem(idx, 1000) == 0, do: IO.puts("Inserting " <> key) :ok = Collections.put(collection, key, value) end) + :ok end) end) @@ -53,6 +66,7 @@ end) end) IO.puts("Inserting #{length(sampleC2)} items with put_all...") + :timer.tc(fn -> {:ok, _n} = Collections.put_all(collection, sampleC2) end) @@ -63,6 +77,7 @@ end) sampleD = Enum.map(1..keys_count, fn i -> record.("keyD", i) end) IO.puts("Inserting sampleD (w/ sorted keys)...") + :timer.tc(fn -> sampleD |> Enum.chunk_every(1000) @@ -78,6 +93,7 @@ end) end) IO.puts("Upserting sampleD...") + :timer.tc(fn -> sampleD |> Enum.chunk_every(1000) @@ -119,7 +135,11 @@ stream_match_all = stream_match_prefix = fn -> Stream.unfold(nil, fn cursor -> - case Collections.get_all(collection, %{cursor: cursor, limit: 500}, "keyA*") do + case Collections.get_all( + collection, + %{cursor: cursor, limit: 500}, + "keyA*" + ) do [] -> nil list -> {list, List.last(list).updated_at} end @@ -131,7 +151,11 @@ stream_match_prefix = stream_match_trigram = fn -> Stream.unfold(nil, fn cursor -> - case Collections.get_all(collection, %{cursor: cursor, limit: 500}, "keyB*bar*") do + case Collections.get_all( + collection, + %{cursor: cursor, limit: 500}, + "keyB*bar*" + ) do [] -> nil list -> {list, List.last(list).updated_at} end @@ -143,10 +167,19 @@ stream_match_trigram = # Process.exit(self(), :normal) IO.puts("\n### Round record count ({microsecs, count}):") -:timer.tc(fn -> stream_all.() |> Enum.count() end) |> IO.inspect(label: "stream_all") -:timer.tc(fn -> stream_match_all.() |> Enum.count() end) |> IO.inspect(label: "stream_match_all") -:timer.tc(fn -> stream_match_prefix.() |> Enum.count() end) |> IO.inspect(label: "stream_match_prefix") -:timer.tc(fn -> stream_match_trigram.() |> Enum.count() end) |> IO.inspect(label: "stream_match_trigram") + +:timer.tc(fn -> stream_all.() |> Enum.count() end) +|> IO.inspect(label: "stream_all") + +:timer.tc(fn -> stream_match_all.() |> Enum.count() end) +|> IO.inspect(label: "stream_match_all") + +:timer.tc(fn -> stream_match_prefix.() |> Enum.count() end) +|> IO.inspect(label: "stream_match_prefix") + +:timer.tc(fn -> stream_match_trigram.() |> Enum.count() end) +|> IO.inspect(label: "stream_match_trigram") + IO.puts("\n") Benchee.run( diff --git a/priv/bench/collections_put_all.exs b/priv/bench/collections_put_all.exs index 2380475c25c..e5669d27eb6 100644 --- a/priv/bench/collections_put_all.exs +++ b/priv/bench/collections_put_all.exs @@ -2,8 +2,13 @@ alias Lightning.Repo alias Lightning.Collections alias Lightning.Projects -IO.puts "### Indexes on collection_items table" -%{rows: rows} = Repo.query!("select indexname, indexdef from pg_indexes where tablename = 'collection_items'") +IO.puts("### Indexes on collection_items table") + +%{rows: rows} = + Repo.query!( + "select indexname, indexdef from pg_indexes where tablename = 'collection_items'" + ) + Enum.each(rows, &IO.puts(Enum.join(&1, ":\n"))) keys_count = 5_000 @@ -15,50 +20,70 @@ 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}]}) + 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}]}, + user + ) + project end {:ok, collection1} = with {:error, :not_found} <- Collections.get_collection("bench1") do - Collections.create_collection(%{"project_id" => project.id, "name" => "bench1"}) + Collections.create_collection(%{ + "project_id" => project.id, + "name" => "bench1" + }) end {:ok, collection2} = with {:error, :not_found} <- Collections.get_collection("bench2") do - Collections.create_collection(%{"project_id" => project.id, "name" => "bench2"}) + Collections.create_collection(%{ + "project_id" => project.id, + "name" => "bench2" + }) end record1 = fn prefix, i -> i_str = String.pad_leading(to_string(i), 5, "0") + %{ "key" => "#{prefix}:foo#{i_str}:bar#{i_str}", - "value" => Jason.encode!(%{ - someid1: "soo#{1_000_000 + i}", - somefield1: "zar#{1_000_000 + i}", - anotherfield1: "yaz#{1_000_000 + i}"}) + "value" => + Jason.encode!(%{ + someid1: "soo#{1_000_000 + i}", + somefield1: "zar#{1_000_000 + i}", + anotherfield1: "yaz#{1_000_000 + i}" + }) } end record2 = fn prefix, i -> i_str = String.pad_leading(to_string(i), 5, "0") + %{ "key" => "#{prefix}:foo#{i_str}:bar#{i_str}", - "value" => Jason.encode!(%{ - someid2: "soo#{1_000_000 + i}", - somefield2: "zar#{1_000_000 + i}", - anotherfield2: "yaz#{1_000_000 + i}"}) + "value" => + Jason.encode!(%{ + someid2: "soo#{1_000_000 + i}", + somefield2: "zar#{1_000_000 + i}", + anotherfield2: "yaz#{1_000_000 + i}" + }) } end samples1 = - 1..keys_count * rounds + 1..(keys_count * rounds) |> Enum.map(fn i -> record1.("keyA", i) end) |> Enum.chunk_every(keys_count) samples2 = - 1..keys_count * rounds + 1..(keys_count * rounds) |> Enum.map(fn i -> record2.("keyB", i) end) |> Enum.chunk_every(keys_count) @@ -66,18 +91,25 @@ IO.puts("\n### Inserting #{rounds} rounds of 2x#{keys_count} with put_all...") durations = Enum.zip(samples1, samples2) - |> Enum.flat_map(fn {sample1, sample2} -> [{sample1, collection1}, {sample2, collection2}] end) - |> Task.async_stream(fn {sample, collection} -> - {duration, _res} = :timer.tc(fn -> - {:ok, _n} = Collections.put_all(collection, sample) - end) - - div(duration, 1_000) - end, max_concurrency: 2, timeout: :infinity) + |> Enum.flat_map(fn {sample1, sample2} -> + [{sample1, collection1}, {sample2, collection2}] + end) + |> Task.async_stream( + fn {sample, collection} -> + {duration, _res} = + :timer.tc(fn -> + {:ok, _n} = Collections.put_all(collection, sample) + end) + + div(duration, 1_000) + end, + max_concurrency: 2, + timeout: :infinity + ) |> Enum.map(fn {:ok, duration_ms} -> IO.puts("elapsed time: #{duration_ms}ms") duration_ms end) -IO.puts "Average: #{Statistics.mean(durations)}ms" -IO.puts "Std Deviation: #{Statistics.stdev(durations)}ms" +IO.puts("Average: #{Statistics.mean(durations)}ms") +IO.puts("Std Deviation: #{Statistics.stdev(durations)}ms") diff --git a/test/lightning/projects_test.exs b/test/lightning/projects_test.exs index 57ec7a1f3b9..80183882c53 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,29 @@ 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 "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,6 +3260,92 @@ defmodule Lightning.ProjectsTest do 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 "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)} @@ -3697,7 +3795,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 +3806,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 From 305b0c4daeda17a31728da55b949b1a4bbb7410b Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 15:49:28 +0200 Subject: [PATCH 10/31] Require an actor on update_project and audit name and description changes --- lib/lightning/projects.ex | 18 ++- lib/lightning/projects/audit.ex | 4 + test/lightning/projects_test.exs | 112 ++++++++++++------ .../channels/run_channel_test.exs | 16 ++- .../channels/workflow_channel_test.exs | 6 +- test/support/projects_helpers.ex | 2 +- 6 files changed, 100 insertions(+), 58 deletions(-) diff --git a/lib/lightning/projects.ex b/lib/lightning/projects.ex index ca0e62a791a..219fc181d7f 100644 --- a/lib/lightning/projects.ex +++ b/lib/lightning/projects.ex @@ -593,14 +593,17 @@ defmodule Lightning.Projects do ## 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) @@ -608,7 +611,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}} -> @@ -631,13 +634,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( diff --git a/lib/lightning/projects/audit.ex b/lib/lightning/projects/audit.ex index 8496d7aec1c..8fc28f16b97 100644 --- a/lib/lightning/projects/audit.ex +++ b/lib/lightning/projects/audit.ex @@ -12,7 +12,9 @@ defmodule Lightning.Projects.Audit do "collaborator_role_changed", "created", "dataclip_retention_period_updated", + "description_updated", "history_retention_period_updated", + "name_updated", "requires_mfa_updated" ] @@ -23,7 +25,9 @@ defmodule Lightning.Projects.Audit do [ :allow_support_access, :dataclip_retention_period, + :description, :history_retention_period, + :name, :requires_mfa ] |> Enum.reduce(multi, fn field, multi -> diff --git a/test/lightning/projects_test.exs b/test/lightning/projects_test.exs index 80183882c53..b4c176e2cca 100644 --- a/test/lightning/projects_test.exs +++ b/test/lightning/projects_test.exs @@ -3351,29 +3351,29 @@ defmodule Lightning.ProjectsTest 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: @@ -3394,7 +3394,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 = @@ -3438,7 +3438,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, @@ -3451,10 +3451,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, @@ -3466,7 +3470,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, @@ -3474,23 +3479,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 = @@ -3512,9 +3523,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 @@ -3616,24 +3631,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", %{ @@ -5107,10 +5139,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_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/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) From c482307d28c86698e7245d580f1fce805c6f1ebd Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 15:52:35 +0200 Subject: [PATCH 11/31] Let a superuser or a service account create a project, through Provisioning's :create_project --- lib/lightning/policies/provisioning.ex | 23 ++++++++---- test/lightning/policies/provisioning_test.exs | 36 +++++++++++++++++++ 2 files changed, 52 insertions(+), 7 deletions(-) diff --git a/lib/lightning/policies/provisioning.ex b/lib/lightning/policies/provisioning.ex index f0b6d1cee67..0122e86628f 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,27 @@ 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 actor :: User.t() | ServiceAccount.t() | ProjectRepoConnection.t() + @type actions :: :create_project | :provision_project | :describe_project - @spec authorize(actions(), actor(), Project.t()) :: + @spec authorize(actions(), 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/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 From 10bfa482ca80f08a93e20888b1c9741c45795af7 Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 15:53:05 +0200 Subject: [PATCH 12/31] Show a project's members on GET /api/projects/:id --- .../controllers/api/project_controller.ex | 9 ++++++- .../controllers/api/project_json.ex | 11 +++++++- .../api/project_controller_test.exs | 25 ++++++++++++++----- 3 files changed, 37 insertions(+), 8 deletions(-) diff --git a/lib/lightning_web/controllers/api/project_controller.ex b/lib/lightning_web/controllers/api/project_controller.ex index f39db4da342..2956ead0a81 100644 --- a/lib/lightning_web/controllers/api/project_controller.ex +++ b/lib/lightning_web/controllers/api/project_controller.ex @@ -103,10 +103,17 @@ defmodule LightningWeb.API.ProjectController do conn.assigns.current_resource, project ) do - render(conn, "show.json", project: project, conn: conn) + render_project(conn, project) else nil -> {:error, :not_found} error -> error end 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/test/lightning_web/controllers/api/project_controller_test.exs b/test/lightning_web/controllers/api/project_controller_test.exs index 026b9a557c6..04902c98679 100644 --- a/test/lightning_web/controllers/api/project_controller_test.exs +++ b/test/lightning_web/controllers/api/project_controller_test.exs @@ -137,14 +137,19 @@ defmodule LightningWeb.API.ProjectControllerTest do assert json_response(conn, 401) == %{"error" => "Unauthorized"} 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" => %{ @@ -168,16 +173,24 @@ defmodule LightningWeb.API.ProjectControllerTest do %{conn: put_req_header(conn, "authorization", "Bearer " <> token)} end - test "shows a project it is not a member of", %{conn: conn} do - project = insert(:project) + 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" => %{"name" => name}} = + assert %{"id" => id, "attributes" => attributes} = json_response(conn, 200)["data"] assert id == project.id - assert name == project.name + + assert attributes == %{ + "name" => project.name, + "description" => project.description, + "members" => [%{"email" => owner.email, "role" => "owner"}] + } end test "is refused a project scheduled for deletion with 401", %{conn: conn} do From 1317772bfe76597e072661f7879754d4a3579299 Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 15:57:23 +0200 Subject: [PATCH 13/31] Return a taken project id as an error on id instead of raising --- lib/lightning/projects/project.ex | 1 + test/lightning/projects_test.exs | 14 ++++++++++++++ 2 files changed, 15 insertions(+) diff --git a/lib/lightning/projects/project.ex b/lib/lightning/projects/project.ex index b8046faf01b..24a6d87902b 100644 --- a/lib/lightning/projects/project.ex +++ b/lib/lightning/projects/project.ex @@ -299,6 +299,7 @@ defmodule Lightning.Projects.Project do |> cast_assoc(:project_users, required: true, sort_param: :users_sort) |> validate() |> validate_project_owner() + |> unique_constraint(:id, name: :projects_pkey) end defp validate_project_owner(changeset) do diff --git a/test/lightning/projects_test.exs b/test/lightning/projects_test.exs index b4c176e2cca..887266403e3 100644 --- a/test/lightning/projects_test.exs +++ b/test/lightning/projects_test.exs @@ -3299,6 +3299,20 @@ defmodule Lightning.ProjectsTest do 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 "emails each initial member by default, and none with notify: false", %{attrs: attrs} do user = insert(:user) From badaf1865fb4d60b6cea7bbff6655e65ca424455 Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 15:57:24 +0200 Subject: [PATCH 14/31] Create or replace a project with PUT /api/projects/:id --- .../controllers/api/project_controller.ex | 174 +++++++++ .../controllers/fallback_controller.ex | 6 + lib/lightning_web/router.ex | 1 + .../api/project_controller_test.exs | 360 ++++++++++++++++++ 4 files changed, 541 insertions(+) diff --git a/lib/lightning_web/controllers/api/project_controller.ex b/lib/lightning_web/controllers/api/project_controller.ex index 2956ead0a81..13d0eb252fc 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,16 @@ defmodule LightningWeb.API.ProjectController do """ use LightningWeb, :controller + import Ecto.Changeset + + alias Lightning.Accounts + alias Lightning.Accounts.User alias Lightning.Policies.Permissions alias Lightning.Policies.ProjectUsers + alias Lightning.Policies.Provisioning alias Lightning.Projects + alias Lightning.Projects.Project + alias Lightning.ServiceAccount action_fallback LightningWeb.FallbackController @@ -110,6 +120,170 @@ defmodule LightningWeb.API.ProjectController do 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, 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 + end + end + + @body_types %{ + id: :binary_id, + name: :string, + description: :string, + members: {:array, :map}, + notify: :boolean + } + + 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)) + + 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 <- writable(project, actor), + {: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 writable(%Project{scheduled_deletion: %DateTime{}}, %ServiceAccount{}), + do: {:error, :scheduled_for_deletion} + + defp writable(project, actor) do + if Permissions.can?(ProjectUsers, :edit_project, actor, project), + do: :ok, + else: {:error, :forbidden} + end + + defp create(conn, attrs, actor) do + with true <- Provisioning.authorize(:create_project, actor, nil), + {:ok, project_users} <- resolve_members(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 + end + end + + @roles ~w(owner admin editor viewer) + + defp resolve_members(members) do + Enum.reduce_while(members, {:ok, []}, fn member, {:ok, resolved} -> + case resolve_member(member) do + {:ok, %{user_id: user_id} = project_user} -> + if Enum.any?(resolved, &(&1.user_id == user_id)), + do: + {:halt, members_error("names #{member["email"]} more than once")}, + else: {:cont, {:ok, [project_user | resolved]}} + + {:error, message} -> + {:halt, members_error(message)} + end + end) + end + + defp members_error(message) do + {:error, + {%{}, %{members: {:array, :map}}} + |> change() + |> add_error(:members, message)} + end + + defp resolve_member(%{"email" => email, "role" => role}) + when is_binary(email) and role in @roles do + case Accounts.get_user_by_email(email) do + %User{id: user_id} -> {:ok, %{user_id: user_id, role: role}} + nil -> {:error, "no user has the email #{email}"} + end + end + + defp resolve_member(_member), + do: + {:error, + "each member needs an email and a role of #{Enum.join(@roles, ", ")}"} + + # 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, + member_errors |> Map.values() |> List.flatten() + ) + + 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), 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/router.ex b/lib/lightning_web/router.ex index ffd5a1efe3e..b5f22558ffd 100644 --- a/lib/lightning_web/router.ex +++ b/lib/lightning_web/router.ex @@ -111,6 +111,7 @@ defmodule LightningWeb.Router do pipe_through [:projects_api] get "/projects/:id", API.ProjectController, :show + put "/projects/:id", API.ProjectController, :update end scope "/api", LightningWeb, as: :api do diff --git a/test/lightning_web/controllers/api/project_controller_test.exs b/test/lightning_web/controllers/api/project_controller_test.exs index 04902c98679..61bf4db9383 100644 --- a/test/lightning_web/controllers/api/project_controller_test.exs +++ b/test/lightning_web/controllers/api/project_controller_test.exs @@ -2,6 +2,7 @@ defmodule LightningWeb.API.ProjectControllerTest do use LightningWeb.ConnCase, async: true import Lightning.Factories + import Ecto.Query import Lightning.ServiceAccountHelpers setup %{conn: conn} do @@ -221,4 +222,363 @@ defmodule LightningWeb.API.ProjectControllerTest do assert json_response(conn, 401) == %{"error" => "Unauthorized"} 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 do + insert(:project, + scheduled_deletion: DateTime.utc_now() |> DateTime.truncate(:second) + ) + 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 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"} + ], + same_user_twice: [ + owner, + admin, + %{admin | "email" => String.upcase(admin["email"]), "role" => "viewer"} + ], + two_owners: [owner, %{admin | "role" => "owner"}], + no_owner: [admin], + no_members: [], + unknown_role: [owner, %{admin | "role" => "superuser"}], + missing_role: [owner, Map.delete(admin, "role")], + 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 Lightning.Repo.get(Lightning.Projects.Project, id) + end + 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 = + insert(:project, + scheduled_deletion: DateTime.utc_now() |> DateTime.truncate(:second), + project_users: [%{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 From 49d5bd9cc07efb5fe35dd0d83585408a0361d931 Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 16:19:12 +0200 Subject: [PATCH 15/31] Refuse a token request that names no scope with invalid_scope --- .../controllers/token_exchange_controller.ex | 16 +++++------- .../token_exchange_controller_test.exs | 25 ++++++++++++++++--- 2 files changed, 28 insertions(+), 13 deletions(-) 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/test/lightning_web/controllers/token_exchange_controller_test.exs b/test/lightning_web/controllers/token_exchange_controller_test.exs index 58e10c5b1ee..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 ) @@ -73,8 +74,12 @@ 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, @@ -170,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, From 2db452b8679bb56508f4251beeb1f334e017c43d Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 16:20:05 +0200 Subject: [PATCH 16/31] Answer GET /api/projects/:id with 409 for a scheduled project and 403 for a policy refusal --- .../controllers/api/project_controller.ex | 27 ++++++++--------- .../api/project_controller_test.exs | 30 ++++++++++++------- .../project_mfa_enforcement_test.exs | 2 +- .../person_or_service_account_auth_test.exs | 4 +-- 4 files changed, 35 insertions(+), 28 deletions(-) diff --git a/lib/lightning_web/controllers/api/project_controller.ex b/lib/lightning_web/controllers/api/project_controller.ex index 13d0eb252fc..c6d147d803c 100644 --- a/lib/lightning_web/controllers/api/project_controller.ex +++ b/lib/lightning_web/controllers/api/project_controller.ex @@ -97,6 +97,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 @@ -104,15 +105,9 @@ 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 + authorize(:access_project, conn.assigns.current_resource, project) do render_project(conn, project) else nil -> {:error, :not_found} @@ -177,7 +172,7 @@ defmodule LightningWeb.API.ProjectController do end defp replace(conn, project, attrs, actor) do - with :ok <- writable(project, actor), + with :ok <- authorize(:edit_project, actor, project), {:ok, project} <- Projects.update_project( project, @@ -190,11 +185,15 @@ defmodule LightningWeb.API.ProjectController do # 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 writable(%Project{scheduled_deletion: %DateTime{}}, %ServiceAccount{}), - do: {:error, :scheduled_for_deletion} - - defp writable(project, actor) do - if Permissions.can?(ProjectUsers, :edit_project, actor, project), + defp authorize( + _action, + %ServiceAccount{}, + %Project{scheduled_deletion: %DateTime{}} + ), + do: {:error, :scheduled_for_deletion} + + defp authorize(action, actor, project) do + if Permissions.can?(ProjectUsers, action, actor, project), do: :ok, else: {:error, :forbidden} end diff --git a/test/lightning_web/controllers/api/project_controller_test.exs b/test/lightning_web/controllers/api/project_controller_test.exs index 61bf4db9383..7caf17c52c6 100644 --- a/test/lightning_web/controllers/api/project_controller_test.exs +++ b/test/lightning_web/controllers/api/project_controller_test.exs @@ -135,7 +135,20 @@ 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 = + insert(:project, + scheduled_deletion: DateTime.utc_now() |> DateTime.truncate(:second), + project_users: [%{user: user, role: :owner}] + ) + + conn = get(conn, ~p"/api/projects/#{project.id}") + + assert json_response(conn, 403) == %{"error" => "Forbidden"} end test "shows the project with its members", %{ @@ -194,20 +207,15 @@ defmodule LightningWeb.API.ProjectControllerTest do } end - test "is refused a project scheduled for deletion with 401", %{conn: conn} do - project = - insert(:project, - scheduled_deletion: DateTime.utc_now() |> DateTime.truncate(:second) - ) - - conn = get(conn, ~p"/api/projects/#{project.id}") + 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, 401) == %{"error" => "Unauthorized"} + 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 401", %{conn: conn} do + test "is refused its own project with 403", %{conn: conn} do project = insert(:project) repo_connection = insert(:project_repo_connection, project: project) @@ -219,7 +227,7 @@ defmodule LightningWeb.API.ProjectControllerTest do ) |> get(~p"/api/projects/#{project.id}") - assert json_response(conn, 401) == %{"error" => "Unauthorized"} + assert json_response(conn, 403) == %{"error" => "Forbidden"} 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/plugs/person_or_service_account_auth_test.exs b/test/lightning_web/plugs/person_or_service_account_auth_test.exs index 9782811ea98..9076a7ff33c 100644 --- a/test/lightning_web/plugs/person_or_service_account_auth_test.exs +++ b/test/lightning_web/plugs/person_or_service_account_auth_test.exs @@ -70,7 +70,7 @@ defmodule LightningWeb.Plugs.PersonOrServiceAccountAuthTest do assert id == project.id end - test "refuses a non-member's personal access token with 401", %{ + test "refuses a non-member's personal access token with 403", %{ conn: conn } do user = insert(:user) @@ -81,7 +81,7 @@ defmodule LightningWeb.Plugs.PersonOrServiceAccountAuthTest do |> with_token(Lightning.Accounts.generate_api_token(user)) |> get(~p"/api/projects/#{project.id}") - assert json_response(conn, 401) == %{"error" => "Unauthorized"} + assert json_response(conn, 403) == %{"error" => "Forbidden"} assert get_resp_header(conn, "www-authenticate") == [] end From 9bd9292c888f99076eea0aeea70759cabaca03cb Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 16:21:10 +0200 Subject: [PATCH 17/31] Refuse a PUT /api/projects/:id body key it does not take with 422 under that key --- .../controllers/api/project_controller.ex | 22 ++++++++++++++++++- .../api/project_controller_test.exs | 21 ++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/lib/lightning_web/controllers/api/project_controller.ex b/lib/lightning_web/controllers/api/project_controller.ex index c6d147d803c..286897a6670 100644 --- a/lib/lightning_web/controllers/api/project_controller.ex +++ b/lib/lightning_web/controllers/api/project_controller.ex @@ -125,11 +125,20 @@ defmodule LightningWeb.API.ProjectController do def update(conn, %{"id" => id}) do actor = conn.assigns.current_resource - with {:ok, attrs} <- parse_body(conn.body_params, id) do + 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} -> + conn + |> put_status(:unprocessable_entity) + |> json(%{errors: Map.new(keys, &{&1, ["is not accepted"]})}) + + error -> + error end end @@ -141,6 +150,17 @@ defmodule LightningWeb.API.ProjectController do 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}} diff --git a/test/lightning_web/controllers/api/project_controller_test.exs b/test/lightning_web/controllers/api/project_controller_test.exs index 7caf17c52c6..3e526bd8355 100644 --- a/test/lightning_web/controllers/api/project_controller_test.exs +++ b/test/lightning_web/controllers/api/project_controller_test.exs @@ -415,6 +415,27 @@ defmodule LightningWeb.API.ProjectControllerTest do 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", %{ From e93b38f7553034fb2282b75a352aea136eeea5da Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 16:21:34 +0200 Subject: [PATCH 18/31] Keep members' emails out of PUT /api/projects/:id 422 messages --- lib/lightning_web/controllers/api/project_controller.ex | 5 ++--- .../controllers/api/project_controller_test.exs | 3 +++ 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/lightning_web/controllers/api/project_controller.ex b/lib/lightning_web/controllers/api/project_controller.ex index 286897a6670..0fb81c67f08 100644 --- a/lib/lightning_web/controllers/api/project_controller.ex +++ b/lib/lightning_web/controllers/api/project_controller.ex @@ -244,8 +244,7 @@ defmodule LightningWeb.API.ProjectController do case resolve_member(member) do {:ok, %{user_id: user_id} = project_user} -> if Enum.any?(resolved, &(&1.user_id == user_id)), - do: - {:halt, members_error("names #{member["email"]} more than once")}, + do: {:halt, members_error("names one user more than once")}, else: {:cont, {:ok, [project_user | resolved]}} {:error, message} -> @@ -265,7 +264,7 @@ defmodule LightningWeb.API.ProjectController do when is_binary(email) and role in @roles do case Accounts.get_user_by_email(email) do %User{id: user_id} -> {:ok, %{user_id: user_id, role: role}} - nil -> {:error, "no user has the email #{email}"} + nil -> {:error, "names an email no user holds"} end end diff --git a/test/lightning_web/controllers/api/project_controller_test.exs b/test/lightning_web/controllers/api/project_controller_test.exs index 3e526bd8355..94419dd95e4 100644 --- a/test/lightning_web/controllers/api/project_controller_test.exs +++ b/test/lightning_web/controllers/api/project_controller_test.exs @@ -482,6 +482,9 @@ defmodule LightningWeb.API.ProjectControllerTest do 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 From f57b5911dfc78f3590470dcfcb0529a091fa263d Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 16:21:45 +0200 Subject: [PATCH 19/31] Restore the benchmark scripts' formatting, keeping create_project's actor --- priv/bench/collections.exs | 53 ++++--------------- priv/bench/collections_put_all.exs | 84 +++++++++--------------------- 2 files changed, 36 insertions(+), 101 deletions(-) diff --git a/priv/bench/collections.exs b/priv/bench/collections.exs index 770e007ef64..a20d9f13a5c 100644 --- a/priv/bench/collections.exs +++ b/priv/bench/collections.exs @@ -9,22 +9,13 @@ 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}]}, - user - ) - + {:ok, project} = Projects.create_project(%{name: "benchee", project_users: [%{user_id: user.id, role: :owner}]}, user) project end {:ok, collection} = with {:error, :not_found} <- Collections.get_collection("benchee") do - Collections.create_collection(%{ - "project_id" => project.id, - "name" => "benchee" - }) + Collections.create_collection(%{"project_id" => project.id, "name" => "benchee"}) end IO.puts("\n### Setup:") @@ -32,17 +23,14 @@ IO.puts("Generating items for benchee collection...") record = fn prefix, i -> i_str = String.pad_leading(to_string(i), 5, "0") - - {"#{prefix}:foo#{i_str}:bar#{i_str}", - Jason.encode!(%{fieldA: "value#{1_000_000 + i}"})} + {"#{prefix}:foo#{i_str}:bar#{i_str}", Jason.encode!(%{fieldA: "value#{1_000_000 + i}"})} end sampleA = Enum.map(1..keys_count, fn i -> record.("keyA", i) end) sampleB = Enum.map(1..keys_count, fn i -> record.("keyB", i) end) sampleC1 = Enum.map(1..keys_count, fn i -> record.("keyC", i) end) - sampleC2 = - keys_count..(keys_count * 2) + keys_count..keys_count * 2 |> Enum.map(fn i -> record.("keyC", i) end) |> Enum.map(fn {k, v} -> %{"key" => k, "value" => v} end) @@ -54,7 +42,6 @@ sampleC2 = if rem(idx, 1000) == 0, do: IO.puts("Inserting " <> key) :ok = Collections.put(collection, key, value) end) - :ok end) end) @@ -66,7 +53,6 @@ end) end) IO.puts("Inserting #{length(sampleC2)} items with put_all...") - :timer.tc(fn -> {:ok, _n} = Collections.put_all(collection, sampleC2) end) @@ -77,7 +63,6 @@ end) sampleD = Enum.map(1..keys_count, fn i -> record.("keyD", i) end) IO.puts("Inserting sampleD (w/ sorted keys)...") - :timer.tc(fn -> sampleD |> Enum.chunk_every(1000) @@ -93,7 +78,6 @@ end) end) IO.puts("Upserting sampleD...") - :timer.tc(fn -> sampleD |> Enum.chunk_every(1000) @@ -135,11 +119,7 @@ stream_match_all = stream_match_prefix = fn -> Stream.unfold(nil, fn cursor -> - case Collections.get_all( - collection, - %{cursor: cursor, limit: 500}, - "keyA*" - ) do + case Collections.get_all(collection, %{cursor: cursor, limit: 500}, "keyA*") do [] -> nil list -> {list, List.last(list).updated_at} end @@ -151,11 +131,7 @@ stream_match_prefix = stream_match_trigram = fn -> Stream.unfold(nil, fn cursor -> - case Collections.get_all( - collection, - %{cursor: cursor, limit: 500}, - "keyB*bar*" - ) do + case Collections.get_all(collection, %{cursor: cursor, limit: 500}, "keyB*bar*") do [] -> nil list -> {list, List.last(list).updated_at} end @@ -167,19 +143,10 @@ stream_match_trigram = # Process.exit(self(), :normal) IO.puts("\n### Round record count ({microsecs, count}):") - -:timer.tc(fn -> stream_all.() |> Enum.count() end) -|> IO.inspect(label: "stream_all") - -:timer.tc(fn -> stream_match_all.() |> Enum.count() end) -|> IO.inspect(label: "stream_match_all") - -:timer.tc(fn -> stream_match_prefix.() |> Enum.count() end) -|> IO.inspect(label: "stream_match_prefix") - -:timer.tc(fn -> stream_match_trigram.() |> Enum.count() end) -|> IO.inspect(label: "stream_match_trigram") - +:timer.tc(fn -> stream_all.() |> Enum.count() end) |> IO.inspect(label: "stream_all") +:timer.tc(fn -> stream_match_all.() |> Enum.count() end) |> IO.inspect(label: "stream_match_all") +:timer.tc(fn -> stream_match_prefix.() |> Enum.count() end) |> IO.inspect(label: "stream_match_prefix") +:timer.tc(fn -> stream_match_trigram.() |> Enum.count() end) |> IO.inspect(label: "stream_match_trigram") IO.puts("\n") Benchee.run( diff --git a/priv/bench/collections_put_all.exs b/priv/bench/collections_put_all.exs index e5669d27eb6..fd69497ae13 100644 --- a/priv/bench/collections_put_all.exs +++ b/priv/bench/collections_put_all.exs @@ -2,13 +2,8 @@ alias Lightning.Repo alias Lightning.Collections alias Lightning.Projects -IO.puts("### Indexes on collection_items table") - -%{rows: rows} = - Repo.query!( - "select indexname, indexdef from pg_indexes where tablename = 'collection_items'" - ) - +IO.puts "### Indexes on collection_items table" +%{rows: rows} = Repo.query!("select indexname, indexdef from pg_indexes where tablename = 'collection_items'") Enum.each(rows, &IO.puts(Enum.join(&1, ":\n"))) keys_count = 5_000 @@ -20,70 +15,50 @@ 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}]}, - user - ) - + 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}]}, user) project end {:ok, collection1} = with {:error, :not_found} <- Collections.get_collection("bench1") do - Collections.create_collection(%{ - "project_id" => project.id, - "name" => "bench1" - }) + Collections.create_collection(%{"project_id" => project.id, "name" => "bench1"}) end {:ok, collection2} = with {:error, :not_found} <- Collections.get_collection("bench2") do - Collections.create_collection(%{ - "project_id" => project.id, - "name" => "bench2" - }) + Collections.create_collection(%{"project_id" => project.id, "name" => "bench2"}) end record1 = fn prefix, i -> i_str = String.pad_leading(to_string(i), 5, "0") - %{ "key" => "#{prefix}:foo#{i_str}:bar#{i_str}", - "value" => - Jason.encode!(%{ - someid1: "soo#{1_000_000 + i}", - somefield1: "zar#{1_000_000 + i}", - anotherfield1: "yaz#{1_000_000 + i}" - }) + "value" => Jason.encode!(%{ + someid1: "soo#{1_000_000 + i}", + somefield1: "zar#{1_000_000 + i}", + anotherfield1: "yaz#{1_000_000 + i}"}) } end record2 = fn prefix, i -> i_str = String.pad_leading(to_string(i), 5, "0") - %{ "key" => "#{prefix}:foo#{i_str}:bar#{i_str}", - "value" => - Jason.encode!(%{ - someid2: "soo#{1_000_000 + i}", - somefield2: "zar#{1_000_000 + i}", - anotherfield2: "yaz#{1_000_000 + i}" - }) + "value" => Jason.encode!(%{ + someid2: "soo#{1_000_000 + i}", + somefield2: "zar#{1_000_000 + i}", + anotherfield2: "yaz#{1_000_000 + i}"}) } end samples1 = - 1..(keys_count * rounds) + 1..keys_count * rounds |> Enum.map(fn i -> record1.("keyA", i) end) |> Enum.chunk_every(keys_count) samples2 = - 1..(keys_count * rounds) + 1..keys_count * rounds |> Enum.map(fn i -> record2.("keyB", i) end) |> Enum.chunk_every(keys_count) @@ -91,25 +66,18 @@ IO.puts("\n### Inserting #{rounds} rounds of 2x#{keys_count} with put_all...") durations = Enum.zip(samples1, samples2) - |> Enum.flat_map(fn {sample1, sample2} -> - [{sample1, collection1}, {sample2, collection2}] - end) - |> Task.async_stream( - fn {sample, collection} -> - {duration, _res} = - :timer.tc(fn -> - {:ok, _n} = Collections.put_all(collection, sample) - end) - - div(duration, 1_000) - end, - max_concurrency: 2, - timeout: :infinity - ) + |> Enum.flat_map(fn {sample1, sample2} -> [{sample1, collection1}, {sample2, collection2}] end) + |> Task.async_stream(fn {sample, collection} -> + {duration, _res} = :timer.tc(fn -> + {:ok, _n} = Collections.put_all(collection, sample) + end) + + div(duration, 1_000) + end, max_concurrency: 2, timeout: :infinity) |> Enum.map(fn {:ok, duration_ms} -> IO.puts("elapsed time: #{duration_ms}ms") duration_ms end) -IO.puts("Average: #{Statistics.mean(durations)}ms") -IO.puts("Std Deviation: #{Statistics.stdev(durations)}ms") +IO.puts "Average: #{Statistics.mean(durations)}ms" +IO.puts "Std Deviation: #{Statistics.stdev(durations)}ms" From 0738e036d263ea8b3aafddfe51eb5eb3253eb096 Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 16:32:30 +0200 Subject: [PATCH 20/31] Resolve the project's Scope once in ProjectController instead of reading scheduled_deletion --- .../controllers/api/project_controller.ex | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/lib/lightning_web/controllers/api/project_controller.ex b/lib/lightning_web/controllers/api/project_controller.ex index 0fb81c67f08..7cabd3e1f3c 100644 --- a/lib/lightning_web/controllers/api/project_controller.ex +++ b/lib/lightning_web/controllers/api/project_controller.ex @@ -40,11 +40,11 @@ defmodule LightningWeb.API.ProjectController do alias Lightning.Accounts alias Lightning.Accounts.User - alias Lightning.Policies.Permissions 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 @@ -205,17 +205,20 @@ defmodule LightningWeb.API.ProjectController do # 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, - %ServiceAccount{}, - %Project{scheduled_deletion: %DateTime{}} - ), - do: {:error, :scheduled_for_deletion} - defp authorize(action, actor, project) do - if Permissions.can?(ProjectUsers, action, actor, project), - do: :ok, - else: {:error, :forbidden} + 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 From c4a30e024b45b74ae3ed4bafd7b5feeedf2fc456 Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 16:34:13 +0200 Subject: [PATCH 21/31] Resolve members by email in one query through Projects.members_by_email/1 --- lib/lightning/projects.ex | 57 ++++++++++++++++ .../controllers/api/project_controller.ex | 53 ++++----------- test/lightning/projects_test.exs | 68 +++++++++++++++++++ 3 files changed, 137 insertions(+), 41 deletions(-) diff --git a/lib/lightning/projects.ex b/lib/lightning/projects.ex index 219fc181d7f..0e5214e34d4 100644 --- a/lib/lightning/projects.ex +++ b/lib/lightning/projects.ex @@ -547,6 +547,63 @@ defmodule Lightning.Projects do |> Repo.all() end + @member_roles ~w(owner admin editor viewer) + + @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 + emails = Enum.map(members, & &1["email"]) + + user_ids = + from(u in User, where: u.email in ^emails, select: {u.email, u.id}) + |> Repo.all() + |> Map.new(fn {email, id} -> {String.downcase(email), id} end) + + 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 = + [ + missing != [] && "names an email no user holds", + length(Enum.uniq(found)) < length(found) && + "names one user more than once" + ] + |> Enum.filter(&is_binary/1) + + if messages == [], do: {:ok, members}, else: {:error, messages} + end + @doc """ Creates a project, recording `actor` as having created it and added each of its initial members. diff --git a/lib/lightning_web/controllers/api/project_controller.ex b/lib/lightning_web/controllers/api/project_controller.ex index 7cabd3e1f3c..6d7f64c3e08 100644 --- a/lib/lightning_web/controllers/api/project_controller.ex +++ b/lib/lightning_web/controllers/api/project_controller.ex @@ -38,8 +38,6 @@ defmodule LightningWeb.API.ProjectController do import Ecto.Changeset - alias Lightning.Accounts - alias Lightning.Accounts.User alias Lightning.Policies.ProjectUsers alias Lightning.Policies.Provisioning alias Lightning.Projects @@ -133,9 +131,7 @@ defmodule LightningWeb.API.ProjectController do end else {:error, :unknown_keys, keys} -> - conn - |> put_status(:unprocessable_entity) - |> json(%{errors: Map.new(keys, &{&1, ["is not accepted"]})}) + render_errors(conn, Map.new(keys, &{&1, ["is not accepted"]})) error -> error @@ -223,7 +219,8 @@ defmodule LightningWeb.API.ProjectController do defp create(conn, attrs, actor) do with true <- Provisioning.authorize(:create_project, actor, nil), - {:ok, project_users} <- resolve_members(attrs[:members] || []) do + {:ok, project_users} <- + Projects.members_by_email(attrs[:members] || []) do attrs |> Map.take([:id, :name, :description]) |> Map.put(:project_users, project_users) @@ -237,45 +234,15 @@ defmodule LightningWeb.API.ProjectController do do: replace(conn, Projects.get_project(attrs.id), attrs, actor), else: render_create_errors(conn, changeset) end - end - end - - @roles ~w(owner admin editor viewer) - - defp resolve_members(members) do - Enum.reduce_while(members, {:ok, []}, fn member, {:ok, resolved} -> - case resolve_member(member) do - {:ok, %{user_id: user_id} = project_user} -> - if Enum.any?(resolved, &(&1.user_id == user_id)), - do: {:halt, members_error("names one user more than once")}, - else: {:cont, {:ok, [project_user | resolved]}} - - {:error, message} -> - {:halt, members_error(message)} - end - end) - end - - defp members_error(message) do - {:error, - {%{}, %{members: {:array, :map}}} - |> change() - |> add_error(:members, message)} - end + else + {:error, messages} when is_list(messages) -> + render_errors(conn, %{members: messages}) - defp resolve_member(%{"email" => email, "role" => role}) - when is_binary(email) and role in @roles do - case Accounts.get_user_by_email(email) do - %User{id: user_id} -> {:ok, %{user_id: user_id, role: role}} - nil -> {:error, "names an email no user holds"} + error -> + error end end - defp resolve_member(_member), - do: - {:error, - "each member needs an email and a role of #{Enum.join(@roles, ", ")}"} - # Another request created the project between our lookup and our insert. defp id_taken?(changeset) do Enum.any?(changeset.errors, fn {field, {_message, opts}} -> @@ -300,6 +267,10 @@ defmodule LightningWeb.API.ProjectController do member_errors |> Map.values() |> List.flatten() ) + render_errors(conn, errors) + end + + defp render_errors(conn, errors) do conn |> put_status(:unprocessable_entity) |> json(%{errors: errors}) diff --git a/test/lightning/projects_test.exs b/test/lightning/projects_test.exs index 887266403e3..2b6a328a660 100644 --- a/test/lightning/projects_test.exs +++ b/test/lightning/projects_test.exs @@ -3260,6 +3260,74 @@ 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) From 84aa7857fa7bfc47313a749e7a8ffdfe876d3e1e Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 16:35:15 +0200 Subject: [PATCH 22/31] Flatten a member's own errors into plain messages under members on PUT /api/projects/:id --- .../controllers/api/project_controller.ex | 17 +++++---- .../api/project_controller_test.exs | 35 +++++++++++++++++++ 2 files changed, 46 insertions(+), 6 deletions(-) diff --git a/lib/lightning_web/controllers/api/project_controller.ex b/lib/lightning_web/controllers/api/project_controller.ex index 6d7f64c3e08..ade85db8357 100644 --- a/lib/lightning_web/controllers/api/project_controller.ex +++ b/lib/lightning_web/controllers/api/project_controller.ex @@ -260,16 +260,21 @@ defmodule LightningWeb.API.ProjectController do errors = if member_errors == %{}, do: errors, - else: - Map.put( - errors, - :members, - member_errors |> Map.values() |> List.flatten() - ) + 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) diff --git a/test/lightning_web/controllers/api/project_controller_test.exs b/test/lightning_web/controllers/api/project_controller_test.exs index 94419dd95e4..35ddecc4982 100644 --- a/test/lightning_web/controllers/api/project_controller_test.exs +++ b/test/lightning_web/controllers/api/project_controller_test.exs @@ -489,6 +489,41 @@ defmodule LightningWeb.API.ProjectControllerTest do 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 body that is not a JSON object", %{conn: conn} do conn = put_project(conn, Ecto.UUID.generate(), [%{"name" => "acme"}]) From 4486f97013827b79004d864f63281aa0220ce752 Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 16:35:57 +0200 Subject: [PATCH 23/31] Roll create_project back with an error when its audit events cannot be recorded --- lib/lightning/projects.ex | 21 +++++++++++++-------- test/lightning/projects_test.exs | 11 +++++++++++ 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/lib/lightning/projects.ex b/lib/lightning/projects.ex index 0e5214e34d4..ec3e61b32fd 100644 --- a/lib/lightning/projects.ex +++ b/lib/lightning/projects.ex @@ -625,14 +625,9 @@ defmodule Lightning.Projects do {: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 - project = Repo.preload(project, :project_users) - - {:ok, _} = - Multi.new() - |> Audit.derive_creation_events(project, actor) - |> Repo.transaction() - + 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 @@ -645,6 +640,16 @@ 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. diff --git a/test/lightning/projects_test.exs b/test/lightning/projects_test.exs index 2b6a328a660..6b5f0caa058 100644 --- a/test/lightning/projects_test.exs +++ b/test/lightning/projects_test.exs @@ -3381,6 +3381,17 @@ defmodule Lightning.ProjectsTest do 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) From 983d0442b632f6a0e82b274266300123548d6c43 Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 16:36:19 +0200 Subject: [PATCH 24/31] Refuse a notify of null on PUT /api/projects/:id with 422 under notify --- .../controllers/api/project_controller.ex | 1 + .../controllers/api/project_controller_test.exs | 16 ++++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/lib/lightning_web/controllers/api/project_controller.ex b/lib/lightning_web/controllers/api/project_controller.ex index ade85db8357..7cffe6203cf 100644 --- a/lib/lightning_web/controllers/api/project_controller.ex +++ b/lib/lightning_web/controllers/api/project_controller.ex @@ -168,6 +168,7 @@ defmodule LightningWeb.API.ProjectController 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 diff --git a/test/lightning_web/controllers/api/project_controller_test.exs b/test/lightning_web/controllers/api/project_controller_test.exs index 35ddecc4982..8ed6589f330 100644 --- a/test/lightning_web/controllers/api/project_controller_test.exs +++ b/test/lightning_web/controllers/api/project_controller_test.exs @@ -524,6 +524,22 @@ defmodule LightningWeb.API.ProjectControllerTest do 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"}]) From 4da5775ab368aaf36e88368b728c642984178b1b Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 16:37:13 +0200 Subject: [PATCH 25/31] Audit name and description changes made through update_project_with_users --- lib/lightning/projects.ex | 1 + .../projects/membership_events_test.exs | 33 +++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/lib/lightning/projects.ex b/lib/lightning/projects.ex index ec3e61b32fd..274bbd05a49 100644 --- a/lib/lightning/projects.ex +++ b/lib/lightning/projects.ex @@ -710,6 +710,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}} -> 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, From 78b99309fb5bb1afbdab8dd2b34b89b3e97cebce Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 16:37:42 +0200 Subject: [PATCH 26/31] Build Scope's and Provisioning's actor types on Lightning.Actor --- lib/lightning/policies/provisioning.ex | 2 +- lib/lightning/projects/scope.ex | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/lightning/policies/provisioning.ex b/lib/lightning/policies/provisioning.ex index 0122e86628f..ed6f85f74b8 100644 --- a/lib/lightning/policies/provisioning.ex +++ b/lib/lightning/policies/provisioning.ex @@ -28,7 +28,7 @@ defmodule Lightning.Policies.Provisioning do alias Lightning.ServiceAccount alias Lightning.VersionControl.ProjectRepoConnection - @type actor :: User.t() | ServiceAccount.t() | ProjectRepoConnection.t() + @type actor :: Lightning.Actor.t() | ProjectRepoConnection.t() @type actions :: :create_project | :provision_project | :describe_project @spec authorize(actions(), actor(), Project.t() | nil) :: diff --git a/lib/lightning/projects/scope.ex b/lib/lightning/projects/scope.ex index ab9ead79a26..18e2de98817 100644 --- a/lib/lightning/projects/scope.ex +++ b/lib/lightning/projects/scope.ex @@ -83,7 +83,7 @@ defmodule Lightning.Projects.Scope do Whoever is asking. A `%User{}` may hold a role; a `%ProjectRepoConnection{}` and a `%ServiceAccount{}` never do. """ - @type actor :: User.t() | ProjectRepoConnection.t() | ServiceAccount.t() + @type actor :: Lightning.Actor.t() | ProjectRepoConnection.t() @typedoc """ Anything that identifies a project: a loaded `%Project{}`, a project id, or From d72762b2b966ecb1d703632ef3839096dedcfe78 Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 16:45:18 +0200 Subject: [PATCH 27/31] Resolve members through Accounts.list_users_by_emails and take their roles from RolesEnum --- lib/lightning/projects.ex | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/lib/lightning/projects.ex b/lib/lightning/projects.ex index 274bbd05a49..a5a5848b009 100644 --- a/lib/lightning/projects.ex +++ b/lib/lightning/projects.ex @@ -547,7 +547,8 @@ defmodule Lightning.Projects do |> Repo.all() end - @member_roles ~w(owner admin editor viewer) + @member_roles ProjectUser.RolesEnum.__valid_values__() + |> Enum.filter(&is_binary/1) @doc """ Resolves members given as `%{"email" => email, "role" => role}` to the @@ -562,12 +563,11 @@ defmodule Lightning.Projects do | {:error, [String.t()]} def members_by_email(members) do if Enum.all?(members, &member_shape?/1) do - emails = Enum.map(members, & &1["email"]) - user_ids = - from(u in User, where: u.email in ^emails, select: {u.email, u.id}) - |> Repo.all() - |> Map.new(fn {email, id} -> {String.downcase(email), id} end) + 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} -> @@ -594,12 +594,11 @@ defmodule Lightning.Projects do {found, missing} = members |> Enum.map(& &1.user_id) |> Enum.split_with(& &1) messages = - [ - missing != [] && "names an email no user holds", - length(Enum.uniq(found)) < length(found) && - "names one user more than once" - ] - |> Enum.filter(&is_binary/1) + 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 From 1047dd5dfc067d2ec644428594d37ac42c813ecc Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 16:45:19 +0200 Subject: [PATCH 28/31] Use Scope.actor in Provisioning's authorize spec instead of a copy --- lib/lightning/policies/provisioning.ex | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/lightning/policies/provisioning.ex b/lib/lightning/policies/provisioning.ex index ed6f85f74b8..61c0e330341 100644 --- a/lib/lightning/policies/provisioning.ex +++ b/lib/lightning/policies/provisioning.ex @@ -28,10 +28,9 @@ defmodule Lightning.Policies.Provisioning do alias Lightning.ServiceAccount alias Lightning.VersionControl.ProjectRepoConnection - @type actor :: Lightning.Actor.t() | ProjectRepoConnection.t() @type actions :: :create_project | :provision_project | :describe_project - @spec authorize(actions(), actor(), Project.t() | nil) :: + @spec authorize(actions(), Scope.actor(), Project.t() | nil) :: boolean() | {:error, :forbidden} def authorize(:create_project, %User{role: :superuser}, _), do: true From 038fc0184f2efd281cbf10bdded629d485ad11e3 Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 16:45:20 +0200 Subject: [PATCH 29/31] Call create_sandbox directly instead of through create_empty_sandbox --- lib/lightning/projects/sandboxes.ex | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/lightning/projects/sandboxes.ex b/lib/lightning/projects/sandboxes.ex index cefae951c79..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, actor) 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, actor) do - Lightning.Projects.create_sandbox(parent, attrs, actor) - end - defp clone_credentials_from_parent(sandbox, parent) do current_time = DateTime.utc_now() |> DateTime.truncate(:second) From 373d807f840119801514315fba58d003661dcbe7 Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 16:46:02 +0200 Subject: [PATCH 30/31] Share the service-account and scheduled-project helpers and leave the members_by_email cases to projects_test --- .../api/project_controller_test.exs | 34 ++++--------------- 1 file changed, 7 insertions(+), 27 deletions(-) diff --git a/test/lightning_web/controllers/api/project_controller_test.exs b/test/lightning_web/controllers/api/project_controller_test.exs index 8ed6589f330..67e741c03d2 100644 --- a/test/lightning_web/controllers/api/project_controller_test.exs +++ b/test/lightning_web/controllers/api/project_controller_test.exs @@ -140,11 +140,7 @@ defmodule LightningWeb.API.ProjectControllerTest do test "is refused with 403, not 409, on a project scheduled for deletion", %{conn: conn, user: user} do - project = - insert(:project, - scheduled_deletion: DateTime.utc_now() |> DateTime.truncate(:second), - project_users: [%{user: user, role: :owner}] - ) + project = scheduled_project([%{user: user, role: :owner}]) conn = get(conn, ~p"/api/projects/#{project.id}") @@ -178,13 +174,8 @@ defmodule LightningWeb.API.ProjectControllerTest do describe "show, for a service account with projects:write" do setup %{conn: conn} do - {account, _private_key} = service_account_with_key() - Mox.stub(Lightning.MockConfig, :service_account, fn -> account end) - - token = - Lightning.ServiceAccount.AccessToken.issue(account, ["projects:write"]) - - %{conn: put_req_header(conn, "authorization", "Bearer " <> token)} + {conn, _account} = with_service_account(conn, ["projects:write"]) + %{conn: conn} end test "shows a project it is not a member of, with its members", %{ @@ -271,9 +262,10 @@ defmodule LightningWeb.API.ProjectControllerTest do end end - defp scheduled_project do + defp scheduled_project(project_users \\ []) do insert(:project, - scheduled_deletion: DateTime.utc_now() |> DateTime.truncate(:second) + scheduled_deletion: DateTime.utc_now() |> DateTime.truncate(:second), + project_users: project_users ) end @@ -454,16 +446,9 @@ defmodule LightningWeb.API.ProjectControllerTest do owner, %{"email" => "nobody@example.com", "role" => "admin"} ], - same_user_twice: [ - owner, - admin, - %{admin | "email" => String.upcase(admin["email"]), "role" => "viewer"} - ], two_owners: [owner, %{admin | "role" => "owner"}], no_owner: [admin], no_members: [], - unknown_role: [owner, %{admin | "role" => "superuser"}], - missing_role: [owner, Map.delete(admin, "role")], not_a_list: %{"email" => owner["email"], "role" => "owner"} ] @@ -649,12 +634,7 @@ defmodule LightningWeb.API.ProjectControllerTest do test "is refused with 403, not 409, on a project scheduled for deletion", %{conn: conn} do user = insert(:user) - - project = - insert(:project, - scheduled_deletion: DateTime.utc_now() |> DateTime.truncate(:second), - project_users: [%{user: user, role: :owner}] - ) + project = scheduled_project([%{user: user, role: :owner}]) conn = conn From 9d2dd33c0a41c7d21bd7916b993899eb4f9247b9 Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Fri, 9 Oct 2026 17:14:13 +0200 Subject: [PATCH 31/31] Write the owner row last so an ownership handover never trips the one-owner index Children of a has_many cast are written in list order, and membership_params/2 lists existing members in whatever order the unordered preload returns them. When the new owner's row came first, its promotion hit the non-deferrable one-owner index before the old owner's demotion. project_with_users_changeset/2 now moves the row that ends up as owner to the end of the list, so every membership write path gets the same order. --- lib/lightning/projects/project.ex | 20 ++++++++++++ test/lightning/kickstart_test.exs | 6 ++-- test/lightning/projects_test.exs | 32 +++++++++++++++++++ test/lightning/setup_utils_test.exs | 8 ++--- test/lightning_web/live/project_live_test.exs | 14 ++++---- 5 files changed, 65 insertions(+), 15 deletions(-) diff --git a/lib/lightning/projects/project.ex b/lib/lightning/projects/project.ex index 24a6d87902b..da3c0d8a546 100644 --- a/lib/lightning/projects/project.ex +++ b/lib/lightning/projects/project.ex @@ -299,9 +299,29 @@ 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 changeset |> get_assoc(:project_users) 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/projects_test.exs b/test/lightning/projects_test.exs index 6b5f0caa058..06a5076bab4 100644 --- a/test/lightning/projects_test.exs +++ b/test/lightning/projects_test.exs @@ -190,6 +190,38 @@ defmodule Lightning.ProjectsTest do errors end + 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) 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/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",