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.

Sign up for GitHub

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

Jump to bottom

Fix project admin permission issue when connecting github integration #2209

Merged
merged 8 commits into from
Feb 23, 2024
Merged

Fix project admin permission issue when connecting github integration #2209

merged 8 commits into from
Feb 23, 2024

Conversation

sausage-todd
Copy link
Contributor

The fix itself:

  • We forgot to send a segment id when configuring github repo mapping

And a couple life improvements:

  • Simpler auth flow when in dev:
    • Doesn't depend on LFID
    • Uses local storage JWT if it's there without ever checking with LFID
    • Allows setting a local storage JWT with a url param: http://localhost:8081/project-groups?my-jwt=eyJ0eXAiOiJKV1QiLCJhbGci...
  • More granular permission errors
    • Throw 403 exception where the check didn't pass, allowing us to know which exactly check didn't pass
    • Even more info in exception messages for better debugging (look ma, no more console.logs)

@sausage-todd sausage-todd force-pushed the project-admin-permission-issue branch 2 times, most recently from 108ba6d to 5cf6c19 Compare February 22, 2024 15:37
Misha Savelyev added 8 commits February 23, 2024 12:42
Make it possible to circumvent auth flow when in dev
af2ed70
Make our middlewares work with all tenant routes
16c985f 
1. PermissionChecker depends on `req.currentSegments`
2. `/tenant/:id` route uses PermissionChecker
3. Since the url param is called `:id`, our `segmentMiddleware` isn't
   triggered
4. Hence, PermissionChecker fails to execute `currentSegments.some()` if
   the the user has a `projectAdmin` role

In a nutshell, this change fixes 500 on `/tenant/:id` for users with
only a `projectAdmin` role
Remove unused method
bc6763b
Add visibility to methods for better understanding of what's used where
f9e3543
Allow arbitrary 403 error messages while in dev
7b1ff1c
Make 403 errors more granular for better dev-ex
cb6aff6
Send segment id when mapping github repos
b1a28e2
Fix permission checker tests
16d1b9e
@sausage-todd sausage-todd force-pushed the project-admin-permission-issue branch from 5cf6c19 to 16d1b9e Compare February 23, 2024 12:43
@sausage-todd sausage-todd merged commit 707aee9 into crowd-linux Feb 23, 2024
10 checks passed
@sausage-todd sausage-todd deleted the project-admin-permission-issue branch February 23, 2024 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joanagmaia joanagmaia joanagmaia approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sausage-todd @joanagmaia