Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
aslilac merged 25 commits intomainfromlilac/fix-a-bunch-of-orgs-stuff
Feb 19, 2025

Conversation

aslilac
Copy link
Member

@aslilacaslilac commentedFeb 5, 2025
edited
Loading

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

  • Move several organizations permissions queries into one query fetched by theOrganizationSettingsLayout 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 unusedOrganizationSummaryPage, 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.

matifali reacted with heart emoji
@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
MemberAuthor

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.

@aslilacaslilac marked this pull request as ready for reviewFebruary 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
MemberAuthor

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 commentedFeb 18, 2025
edited
Loading

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
MemberAuthor

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
MemberAuthor

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 auditorsorganizations.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 commentedFeb 19, 2025
edited
Loading

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 commentedFeb 19, 2025
edited
Loading

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
MemberAuthor

aslilac commentedFeb 19, 2025
edited
Loading

@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

@jaaydenhjaaydenh 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.

@aslilacaslilac merged commitdeadac0 intomainFeb 19, 2025
33 checks passed
@aslilacaslilac deleted the lilac/fix-a-bunch-of-orgs-stuff branchFebruary 19, 2025 18:48
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsFeb 19, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

[8]ページ先頭

©2009-2025 Movatter.jp