feat: add organization-scoped permission checks to deployment settings #14063

code-asher · 2024-07-31T00:53:17Z

You should be able to see the all the links on the settings page to which you have access, and those pages should react appropriately (like groups should only have the create group button if you have permission, etc).

This only affects multi-org. There should be no change if not multi-org.

alwaysmeticulous · 2024-07-31T00:55:39Z

🤖 Meticulous spotted visual differences in 321 of 735 screens tested: view and approve differences detected.

_{Last updated for commit 5a547fc. This comment will update as new commits are pushed.}

site/src/pages/DeploySettingsPage/DeploySettingsLayout.tsx

site/src/pages/ManagementSettingsPage/GroupsPage/GroupsPage.tsx

site/src/pages/ManagementSettingsPage/OrganizationSettingsPage.tsx

BrunoQuaresma

I left a few comments, and I see there are a few TODOs in the codebase. Do you plan to finish these TODOs during the current work or address them later? If later, would it make sense to transform the important ones into issues?

PS: I'm going to QA this at some point today.

BrunoQuaresma · 2024-07-31T14:40:06Z

I QAed the changes and they look good, but I think it could be better if you have specific areas where I should look for issues. Since it is a "draft" PR I'm not approving it for now, but feel free to request another review at any time.

code-asher · 2024-07-31T18:26:12Z

@BrunoQuaresma Thank you for giving it an early look! I still need to add/update some tests/Storybooks and then I think it will be good to go. I will make some notes on the kinds of things that need to be QA'd.

As for the TODOs, I will check with Kira and make issues for the ones she thinks makes sense.

code-asher · 2024-08-01T01:48:43Z

All right, I updated the tests and added stories but I had to break out into some views to do that. The code is mostly unchanged but the diff will look a a bit large.

So generally here is what I have been testing:

A site-wide admin should see and be able to edit all the deployment settings and organizations.
A site-wide auditor should see the audit logs for the default org. And the deployment settings apparently, but not the licenses or appearance. They can also see the members and groups for the default orgs but not edit either.
An org admin should only be able to edit the settings, members, and groups of orgs of which they are an admin, and should see the audit logs of their orgs as well.
All of the above should be able to view users site-wide. But only the admin can add new users.
An org auditor should only be able to see the audit logs for their own orgs. But no access to the members or groups of the org.
Plain members will not see any deployment dropdown at all (just like before).
If you have multi-org disabled or are not licensed for multi-org then everything should look and work exactly the way it was before.

NOTE: The audit log will not work right now for org admins/auditors, but they will once Steven's PR is merged so I will leave this in a draft until that is done then I will mark ready and request a review. <3

Seems like all the other frontend variables use the `view` syntax. Arguably we should use `read` to match the backend, but `view` does seem more UI-like.

All the checks now require both the experiment and license. I also renamed the variable canViewOrganizations everywhere for consistency.

You will only see the button if you can create a group in that org. Now that the old site-wide group permission is unused, remove it.

Since in addition to deployment settings this page now also includes users, audit logs, groups, and orgs. Since you might not be able to fetch deployment values, move all the loaders to the individual pages instead of in the wrapping layout.

We only use this layout when multi-org is enabled, so no need to run the check a second time.

An auditor for example appears to have the view deployment values permission but cannot view licenses.

code-asher · 2024-08-01T20:11:48Z

Should be good to go now!

BrunoQuaresma · 2024-08-05T13:38:20Z

I completed the QA and I found a few bugs. I think these bugs are not blockers so I'm approving the PR. Would be nice, and safe, to have someone else QAing this PR since it is quite large and touches some important places. Good work!

✅ A site-wide admin should see and be able to edit all the deployment settings and organizations.
❌ A site-wide auditor should see the audit logs for the default org. And the deployment settings apparently, but not the licenses or appearance. They can also see the members and groups for the default orgs but not edit either.
⚠️ An org admin should only be able to edit the settings, members, and groups of orgs of which they are an admin, and should see the audit logs of their orgs as well.
✅ All of the above should be able to view users site-wide. But only the admin can add new users.
✅ An org auditor should only be able to see the audit logs for their own orgs. But no access to the members or groups of the org.
✅ Plain members will not see any deployment dropdown at all (just like before).
✅ If you have multi-org disabled or are not licensed for multi-org then everything should look and work exactly the way it was before.

As an auditor, I was able to see the licenses.

As an auditor, I'm getting stuck on the audit logs page. I can't see the other org options.

Screen.Recording.2024-08-05.at.10.28.03.mov

As org admin, I was able to remove the owner from the members. I don't think admins should have the right to remove admins or any other higher user level.

code-asher · 2024-08-05T22:21:51Z

Thank you for the thorough testing!!

As an auditor, I was able to see the licenses.

This should be fixed! Looks like you were on an older commit.

As an auditor, I'm getting stuck on the audit logs page. I can't see the other org options

~~Huh, weird I was not able to reproduce with a site-wide admin. I will poke into it more.~~ Actually, could be due to being on an older commit, I did make some auditor changes. But see below, I removed the redirect entirely, but I think we need to design a better page for when the user has no edit permissions eventually.

As org admin, I was able to remove the owner from the members. I don't think admins should have the right to remove admins or any other higher user level.

~~That does seem weird, let me check with the backend folks. I will follow up with another PR if this needs to change.~~ Looks like this is OK, owners can still do everything even if removed from an org, so it is mostly just up to the org admin if they want the owner in the member list or not.

code-asher · 2024-08-05T23:00:58Z

I changed the automatic redirect for auditors, so now it just says "you have no permissions to edit" if there are no permissions instead of trying to redirect to the audit log. I think we will probably want to consider a better design for this though. I updated the ticket to reflect needing a decision on this.

The 0000 syntax does not work, it has to be the actual org ID. Technically this is probably not necessary since it means only admins can create groups which was already the case, but I think being explicit about the org we are checking is a good idea, even if these pages will eventually go away in favor of the multi-org ones.

I think this needs a proper design/decision.

Originally I thought I should update these to use the org-based permissions query but I think maybe I should scope my changes to just the multi-org settings page and leave the old pages alone for now.

I think the "you do not have permission to edit" message might make it sound like you can do nothing at all with the org, but it might be that you can edit members and groups, just not the org settings. So, instead, show the org settings but disable the form if you cannot edit. Probably there is still a better design but maybe this is OK for now.

code-asher · 2024-08-06T01:42:36Z

Just FYI I reverted two changes to the old groups and users pages, I felt like they were out of scope for this PR and I did not directly call them out in the list of stuff to test.

I also changed the org settings page to show an ineditable form instead of a big "you cannot edit this org" message but I will get a proper design for this page when a user has no permissions later.

github-actions bot assigned code-asher Jul 31, 2024

code-asher force-pushed the asher/org-permissions branch 2 times, most recently from ad74fcf to f6dfce0 Compare July 31, 2024 01:49