feat: add editable project user roles for owner and admin - #4993
feat: add editable project user roles for owner and admin#4993Promist-Moon wants to merge 2 commits into
Conversation
|
Hey @Promist-Moon , thanks for this! Looks like a solid contribution. I'll give it a proper review later this week. |
|
Thanks Elias! Let me know when there's something you'd like me to fix! |
There was a problem hiding this comment.
Hey @Promist-Moon,
Thanks for picking this one up 👏🏽 The approach is right, and I really like that you re-checked that project_user_id belongs to the mounted project. I think we should try to keep that standard for other places in this page.
Three things in the set_role handler need fixing before this merges, written up inline. The role allowlist rejects nothing, so "owner" reaches the database write. A sandbox owner can demote a parent-project admin they are not allowed to remove. And a crafted project_user_id crashes the settings LiveView for any project member, viewers included. The rest of the comments are small.
Thanks again for the work 🙏
| changeset = | ||
| {%{role: project_user.role |> to_string()}, %{role: :string}} | ||
| |> Ecto.Changeset.cast(%{role: role}, [:role]) | ||
| |> Ecto.Changeset.validate_inclusion(:role, ~w(viewer editor admin)) | ||
|
|
||
| case Ecto.Changeset.get_change(changeset, :role) do |
There was a problem hiding this comment.
This is the one I'd most like fixed. validate_inclusion/4 only appends to changeset.errors, it never touches changeset.changes, and get_change/2 reads straight out of changes. So the case matches on any cast-able string including "owner", and it reaches Projects.update_project_user/2 unfiltered.
:owner is a legal RolesEnum value, so the only thing stopping it today is the project_owner_unique_index partial index, which is a data-integrity constraint rather than an authorisation control. On a project with no owner row the write succeeds and mints a new owner, and those are reachable: Accounts.purge_user/1 does a raw Repo.delete_all over the user's project_users with no ownership guard.
Could we gate on changeset.valid?, or just pattern-match the three allowed strings ?
| defp role_editable?( | ||
| project_user, | ||
| current_user, | ||
| can_edit_project_user_role | ||
| ) do | ||
| can_edit_project_user_role and project_user.role != :owner and | ||
| project_user.user_id != current_user.id | ||
| end |
There was a problem hiding this comment.
This is user_removable?/5 minus its sandbox clause, not (sandbox? and parent_admin?(project, project_user)), which delete_project_user!/1 also backs up by raising.
It matters because build_sandbox_project_attributes/5 copies the parent owner into a sandbox as :admin, and provisioning only needs :editor on the parent. I confirmed locally that a sandbox owner can't remove the parent owner's row but can demote it to viewer.
Could we thread project and sandbox? through and mirror that clause ? You'd want include: :user on the fetch so parent_admin?/2 can run.
| %{"project_user_id" => project_user_id, "role" => role}, | ||
| %{assigns: assigns} = socket | ||
| ) do | ||
| project_user = Projects.get_project_user!(project_user_id) |
There was a problem hiding this comment.
This runs before any of the authorisation branches, on a client-controlled hidden input. An unknown UUID raises Ecto.NoResultsError and a non-UUID raises Ecto.Query.CastError, either of which crashes the settings LiveView. Any member can fire it, viewers included.
The non-bang get_project_user/1, with a nil clause falling through to the unauthorised flash, would cover it.
| editable? = | ||
| assigns.can_edit_project_user_role and | ||
| assigns.project_user.role != :owner and | ||
| assigns.project_user.user_id != assigns.current_user.id |
There was a problem hiding this comment.
Non-blocking: these three conditions are also spelled out in role_editable?/3, and the sandbox drift I flagged there is what the duplication costs. Could we export one predicate and call it from both ?
| "role" => "owner" | ||
| }) | ||
|
|
||
| assert html =~ "Error when updating the project user" |
There was a problem hiding this comment.
This passes because the unique index collided, not because the allowlist rejected anything, which is what masked the changeset issue above. Asserting on an authorisation refusal would make it meaningful.
The crafted-event tests also all use a viewer as the actor. An admin firing set_role at the owner row or at their own row would be worth covering.
| - Project settings now allow `owner` and `admin` users to edit collaborator | ||
| roles inline (viewer/editor/admin) without remove-and-readd. Owner rows and | ||
| self-role edits remain non-editable. | ||
| [#4993](https://github.com/OpenFn/lightning/pull/4993) |
There was a problem hiding this comment.
Small one: new feature so I'd put this under Added rather than Changed, and link #3603 rather than the PR.
2 participants
Description
This PR adds project role editing for collaborators in Project Settings (#collaboration). Users with permissions
ownerandadmincan edit the collaborator permissions to be admin, editor, or viewer. Tests for the relevant permissions were also added.Closes #3603
Validation steps
Additional notes for the reviewer
AI Usage
Please disclose whether you've used AI anywhere in this PR (it's cool, we just want to know!):
You can read more details in our Responsible AI Policy
Pre-submission checklist
/reviewwith Claude Code)
(e.g.,
:owner,:admin,:editor,:viewer)