Skip to content

fix: fix loading states and permissions checks in organization settings #16465

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 25 commits into from
Feb 19, 2025

Conversation

aslilac
Copy link
Member

@aslilac aslilac commented Feb 5, 2025

Closes coder/internal#331
Closes coder/internal#346
Closes coder/internal#347
Closes coder/internal#348
Closes coder/internal#349
Closes coder/internal#350
Closes #16379

  • Move several organizations permissions queries into one query fetched by the OrganizationSettingsLayout component

  • Update permissions checks for whether the organizations settings should be accessible by a user

We used to just check if the user could edit any organization. The problem with that, is that by "organization" we meant the organization table, which just holds metadata, and didn't take into account the ability to edit members, groups, roles, or any other resource. We now check if the user can edit any of these resources individually in any organization.

  • Fix a bunch of loading states, and spinners

There are a ton of places where if data was missing we just assumed it was loading. A lot of these have been cleaned up.

  • Remove unused code, lots of general hygiene improvements.

Remove the unused OrganizationSummaryPage, move the /organizations redirect logic into its own component instead of leaving it in the settings page (which is no longer even the default)

  • Update individual page permission checks, and make sure editing is checked separately from viewing

More pages are now viewable if the user has permission to view the information on them, even if they can't edit them.

@jaaydenh
Copy link
Contributor

When testing the permissions for each Role, how do you know what the permissions should be for each user other than looking at the backend code? Having documentation of the permissions as they relate to all orgs features for all roles will be helpful for reviewing this PR especially any tests, for future testing and for documentation for users

For all roles including:
Owner, User Admin, Template Admins, Auditor, Organization Admin, Organization User Admin, Organization Template Admin, Organization Auditor

@aslilac
Copy link
Member Author

aslilac commented Feb 11, 2025

I don't think it really makes sense to focus on "roles", because custom roles exist. I just focused on "what permissions make sense for enabling x". a "create/new thing" button should only show up for people with the proper permissions to create a that type of thing.

@aslilac aslilac requested a review from jaaydenh February 18, 2025 18:54
@aslilac aslilac marked this pull request as ready for review February 18, 2025 18:54
@jaaydenh

This comment was marked as resolved.

@jaaydenh

This comment was marked as resolved.

@jaaydenh

This comment was marked as resolved.

@jaaydenh

This comment was marked as resolved.

@aslilac
Copy link
Member Author

aslilac commented Feb 18, 2025

A user with site wide user admin, template admin and auditor is able to successfully access an org they are not a member of without any errors. This does not seem correct.

it is correct though, that's how site wide permissions are implemented

@jaaydenh
Copy link
Contributor

jaaydenh commented Feb 18, 2025

A user with site wide user admin, template admin and auditor is able to successfully access an org they are not a member of without any errors. This does not seem correct.

it is correct though, that's how site wide permissions are implemented

The part that feels incorrect is that a user is only able access an org they are not a member if they have all 3 permissions combined (site wide user admin, template admin and auditor) if they have just 1 or 2 of them then they do not have access. It feels like that should only be for an owner role. Behavior that only happens for combinations of roles feels confusing.

@aslilac
Copy link
Member Author

aslilac commented Feb 18, 2025

after looking at the code, the new provisioners page has some deeper rooted permissions issues that probably need to be dealt with separately.

@aslilac
Copy link
Member Author

aslilac commented Feb 19, 2025

The part that feels incorrect is that a user is only able access an org they are not a member if they have all 3 permissions combined (site wide user admin, template admin and auditor) if they have just 1 or 2 of them then they do not have access. It feels like that should only be for an owner role. Behavior that only happens for combinations of roles feels confusing.

I think the real issue here is that I gave site auditors organizations.read to fix another issue, but site user admin and site template admin both need it as well. All of the site wide roles now have that permission, which equals them out.

@jaaydenh
Copy link
Contributor

jaaydenh commented Feb 19, 2025

Why would a site wide template admin or auditor have access to custom roles read only but not idp sync? Whereas a user admin has access to both? I would expect the template admin and auditor does not need even read access to custom roles.

@jaaydenh
Copy link
Contributor

jaaydenh commented Feb 19, 2025

For a site wide template admin or site wide auditor I am still seeing this error message for an org they are not a member of. I am expecting them to have access to members in the same way they do for an org they are a member of.

Screenshot 2025-02-19 at 16 51 30

@jaaydenh
Copy link
Contributor

It may help to update the roles popover with information about access to provisioners, custom roles and idp sync and to be clear about ability to manage users and groups. FYI, its because of the text here that I dont think site wide template admins or auditors should be able to access members or groups in organizations.

Screenshot 2025-02-19 at 16 57 43

@aslilac
Copy link
Member Author

aslilac commented Feb 19, 2025

@jaaydenh this PR is already enormous and fixes a ton of bugs. I appreciate that you want to find more bugs, but this code needs to be merged. this PR is almost two weeks old, if you have a problem with any of the changes I have already made then we can talk about those, but any further fixes need to be follow ups. I'm not gonna keep adding more and more bug fixes in one PR forever. 😅

this is not the right venue to be discussing what permissions a role should have, or to discuss if we describe built-in roles with sufficient detail, the goal here it to make sure that the correct permission is being checked in more places.

@jaaydenh
Copy link
Contributor

@aslilac Totally fine moving more fixes to another PR. I was imaging the goal of this PR was to correct the frontend permissions for all the built-in site wide and organizations roles.

Copy link
Contributor

@jaaydenh jaaydenh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving this to lock in the all the fixes so far. I will create additional issues for the other bugs that are out of scope.

@aslilac aslilac merged commit deadac0 into main Feb 19, 2025
33 checks passed
@aslilac aslilac deleted the lilac/fix-a-bunch-of-orgs-stuff branch February 19, 2025 18:48
@github-actions github-actions bot locked and limited conversation to collaborators Feb 19, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.