Skip to content
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

Org managers have full access to projects owned by their org #919

Merged
merged 23 commits into from
Jul 8, 2024

Conversation

rmunn
Copy link
Contributor

@rmunn rmunn commented Jul 3, 2024

Fixes #915.

This PR used to be on top of #888, but now that #888 is merged, I've rebased on top of develop. So now it's ready to merge once it's been reviewed and approved.

@rmunn
Copy link
Contributor Author

rmunn commented Jul 3, 2024

Commit 06311c7 contains an attempt to get the JWT orgs claim looking like it should (with role: "ADMIN" instead of Role: 1). Two problems:

  • First, the JSON serializer is producing Admin rather than ADMIN, and it's not obvious how to change that. Not a big deal, commit 06311c7 deals with that in the jwtToUser method. Not the best solution, but it works.
  • Second, the C# backend code is using the JSON serialization to get the claims, and it's expecting numbers rather than strings. This is a bigger problem, because it means code like this is failing:
if (User.Orgs.Any(o => o.OrgId == orgId && o.Role == OrgRole.Admin)) return true;

The LexAuthUser.GetClaims() method in the backend is seeing the string "Admin" instead of the number 1 that it expects, and so it's turning that role into OrgRole.Unknown.

I don't know how to solve that second problem. @hahn-kev, any ideas?

Update: Thanks, solved now.

frontend/src/lib/user.ts Outdated Show resolved Hide resolved
@rmunn rmunn self-assigned this Jul 4, 2024
@rmunn rmunn requested a review from hahn-kev July 4, 2024 02:15
@rmunn rmunn force-pushed the feat/org-managers-have-full-access branch from 3909e11 to 2dd4fb9 Compare July 4, 2024 05:47
rmunn and others added 11 commits July 4, 2024 13:58
Org managers can view, edit, and sync any projects owned by the org they
manage, even if they themselves are not explicitly listed as a member of
that project. This is similar to how site admins have full access to
anything, except restricted to projects owned by the org(s) in question.
This should be reverted once the JWTs are being serialized correctly
(proper camelCase prop names and enums as strings instead of numbers) in
the .NET backend code.
If they turn out to be useful elsewhere, we'll make them public and add
then to IPermissionService at that time.
@rmunn rmunn force-pushed the feat/org-managers-have-full-access branch from 2dd4fb9 to 3390063 Compare July 4, 2024 06:58
@rmunn rmunn changed the base branch from feat/org-page-improvements to develop July 4, 2024 06:58
Copy link

github-actions bot commented Jul 4, 2024

UI unit Tests

11 tests  ±0   11 ✅ ±0   0s ⏱️ ±0s
 3 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit 086173b. ± Comparison against base commit 00ec9a5.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jul 4, 2024

C# Unit Tests

52 tests   52 ✅  6s ⏱️
10 suites   0 💤
 1 files     0 ❌

Results for commit 086173b.

♻️ This comment has been updated with latest results.

rmunn added 3 commits July 4, 2024 15:00
First step is to teach integration fixture how to create confidential
projects.
Now we're ready to test Send/Receive of confidential projects by an org
manager who's not explicitly listed as a project member.
Copy link
Collaborator

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

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

looks good, but I would like to avoid running a query every permission check as those happen quite frequently, especially during resumable S&R, use a cache similar to what I did in the lookup of project id via code.

backend/LexBoxApi/Services/PermissionService.cs Outdated Show resolved Hide resolved
backend/LexBoxApi/Services/PermissionService.cs Outdated Show resolved Hide resolved
backend/LexCore/ServiceInterfaces/IPermissionService.cs Outdated Show resolved Hide resolved
rmunn added 4 commits July 8, 2024 12:42
This will greatly reduce the number of lookups that need to be made when
an organization manager are trying to manage a project that he's not an
explicit member of.
@rmunn rmunn force-pushed the feat/org-managers-have-full-access branch from 3fe6cf5 to 08bc5f8 Compare July 8, 2024 05:55
@rmunn rmunn requested a review from hahn-kev July 8, 2024 05:55
Copy link
Collaborator

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

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

nice work, some minor things as well as caching the lookup for project confidentiality.

backend/LexBoxApi/Services/ProjectService.cs Outdated Show resolved Hide resolved
backend/LexBoxApi/Services/ProjectService.cs Outdated Show resolved Hide resolved
backend/LexBoxApi/Services/PermissionService.cs Outdated Show resolved Hide resolved
@rmunn rmunn requested a review from hahn-kev July 8, 2024 06:56
Copy link
Collaborator

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

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

looks good to me.

@rmunn rmunn merged commit e99ed21 into develop Jul 8, 2024
14 checks passed
@rmunn rmunn deleted the feat/org-managers-have-full-access branch July 8, 2024 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update permissions so that an org admin can S&R and edit all projects in their org (add members etc)
2 participants