Skip to content

Gia/#283 role based acct mgt - #289

Merged
ND68 merged 11 commits into
mainfrom
gia/role-based-acct-mgt
Mar 12, 2026
Merged

ND68 merged 11 commits into
mainfrom
gia/role-based-acct-mgt

Conversation

@giannacastr

@giannacastr giannacastr commented Feb 22, 2026 •

Copy link
Copy Markdown
Contributor

Description

Resolves ticket number: #283
Verifies correct role-based account management/permissions.

Explain what your code changes:
Implemented comprehensive API-level and UI-level enforcement for user account management to prevent unauthorized deactivations and permission escalation.

Self-Deactivation Protection: Added logic to the PATCH /api/users/[id] route to prevent any user (including Super Admins) from setting their own enabled status to false.

Permission Hierarchy Enforcement: Restricted users with userWrite from deactivating or modifying permissions for accounts that possess both isSuper and userWrite privileges.

Access Control: Verified that users without userWrite permission are strictly blocked from accessing any "deactivate" or "update" functionality on the user management page.

UI Hardening: Removed the deactivation option from the UI popup for self-targeting actions to provide a smoother user experience and prevent invalid requests.

List the steps you took to test your code:
All enforcement rules were validated via Postman using session cookies extracted from local Chrome DevTools.

Test 1: Logged in as Super Admin and attempted a PATCH request to my own ID with {"enabled": false}. Result: 403 Forbidden.

Test 2: Logged in as Staff Admin and attempted to deactivate the Super Admin (ID 1). Result: 403 Forbidden.

Test 3: Logged in as Staff Admin and attempted to strip permissions from a Super Admin. Result: 403 Forbidden.

Test 4: Logged in as Distribution Lead (no userWrite) and attempted to deactivate a different user. Result: 403 Forbidden.

Manual UI Check: Confirmed the "Deactivate" button is hidden/removed in the browser when viewing my own account profile.

Checklist

  • The ticket is mentioned above
  • The changes fulfill the success criteria of the ticket
  • The changes were self-reviewed
  • A review was requested

@netlify

netlify Bot commented Feb 22, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for hope-for-haiti ready!

Name Link
🔨 Latest commit 34fa48d
🔍 Latest deploy log https://app.netlify.com/projects/hope-for-haiti/deploys/699a8bf69cc20c0008c2eb0d
😎 Deploy Preview https://deploy-preview-289--hope-for-haiti.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@greptile-apps

greptile-apps Bot commented Feb 22, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR implements role-based access control enforcement for user account management, adding both API-level and UI-level guards against self-deactivation, unauthorized deactivation of privileged users, and permission escalation on super-admin accounts.

Key changes:

  • Self-deactivation protection: The API route correctly prevents users from modifying their own enabled status. The UI includes defensive guards in handleDeactivateAccount and confirmDeactivateAccount that toast errors for self-targeting and protected users.
  • Permission hierarchy enforcement: Backend guards restrict modifications to accounts with specific privilege combinations.
  • UI hardening gap: AccountDropdown was extended with a hideDeactivateOption prop to fully hide the deactivate button for self and protected users, but the prop is not forwarded at the call site in page.tsx. This means users see a toast error on click rather than the button being absent as intended.
  • Seed data: A staffAdmin seed user is added to facilitate local testing of permission rules—a clean, well-scoped addition.

Additional findings already tracked in prior reviews:

  • Accidentally committed Docker command files in src/schema/Untitled and src/types/Untitled need removal.
  • hasUserRead permission flag is computed in AdminDashboardScreen but not forwarded to AnalyticsSection, leaving the permission gate ineffective.
  • Backend deactivation guard uses OR logic while permission-modification guard uses AND logic, creating an asymmetry that should be reconciled with the frontend's AND-based checks.

Confidence Score: 2/5

  • Not safe to merge — UI hardening for deactivate button is incomplete, and the hasUserRead permission gate remains ineffective.
  • The PR implements important API-level permission checks and self-deactivation guards that are working correctly. However, two functional gaps remain: (1) the hideDeactivateOption prop is never forwarded, leaving the deactivate button visible for self and protected users despite the PR's stated goal of removing it from the UI, and (2) hasUserRead is computed but not passed to AnalyticsSection, defeating the permission gate. Additionally, the backend guards have an OR-vs-AND asymmetry and there are accidentally committed Docker CLI files that need cleanup.
  • src/app/accountManagement/page.tsx (missing hideDeactivateOption forwarding), src/screens/dashboard/AdminDashboardScreen.tsx (hasUserRead not passed to AnalyticsSection), src/services/userService.ts (OR vs AND guard asymmetry), src/schema/Untitled and src/types/Untitled (accidentally committed files)

Comments Outside Diff (1)

  1. src/app/accountManagement/page.tsx, line 373-381 (link)

    hideDeactivateOption prop not passed to AccountDropdown

    The hideDeactivateOption prop was added to AccountDropdown specifically to hide the deactivate/activate button for self-targeting and protected rows (per the PR description: "Removed the deactivation option from the UI popup for self-targeting actions"), but it is never set when the dropdown is rendered here. As a result, the deactivate option still appears in the dropdown for both the current user's own row and for protected (isSuper && userWrite) users — it only produces a toast error when clicked rather than being hidden as intended.

    Both conditions should be forwarded:

Last reviewed commit: e4437e1

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

8 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

Comment thread src/schema/Untitled Outdated
Comment thread src/types/Untitled Outdated
Comment thread src/app/accountManagement/page.tsx
@greptile-apps

greptile-apps Bot commented Feb 22, 2026

Copy link
Copy Markdown
Contributor
Additional Comments (1)

src/services/userService.ts
The logic here prevents deactivating ANY user with userWrite OR isSuper, but the API route at line 102-108 already blocks users from deactivating themselves. This means a Super Admin cannot deactivate a Staff Admin who only has userWrite (not isSuper), which seems overly restrictive.

According to the PR description, the intent was to prevent deactivating users with BOTH isSuper AND userWrite (actual Super Admins). The condition should be:

    if (
      data.enabled === false &&
      existingUser.userWrite &&
      existingUser.isSuper
    ) {
      throw new AuthorizationError(
        "Cannot deactivate users with isSuper and userWrite permissions"
      );
    }

@ND68 ND68 removed their assignment Feb 23, 2026
@ND68

ND68 commented Feb 23, 2026 •

Copy link
Copy Markdown
Collaborator

hey gia, overall great work. In my initial ticket I noticed I made a logical mistake pertaining to non-super accounts with user_write permissions, specifically, this line:

"If you DO have user_write permission, then you cannot deactivate or overwrite people with is_super and user_write. Again, thoroughly test this will all roles and accounts."

What I meant to say was users that only have user_write (no is_super) should NOT be able to deactivate users with is_super OR users with user_write. Essentially, you shouldn't be able to deactivate an adjacent role or a higher role. That's my bad, I should've been more clear.

Also, it looks like there are two docker command files that got caught in your commits, please remove those.

If you can make this change that would be great, everything else looks perfect. thanks

@ND68 ND68 linked an issue Feb 23, 2026 that may be closed by this pull request
@netlify

netlify Bot commented Mar 8, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for hope-for-haiti ready!

Name Link
🔨 Latest commit e1d250b
🔍 Latest deploy log https://app.netlify.com/projects/hope-for-haiti/deploys/69b30d3ae349720007e9f325
😎 Deploy Preview https://deploy-preview-289--hope-for-haiti.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Comment thread src/schema/Untitled Outdated
Comment thread src/types/Untitled Outdated
Comment thread src/screens/dashboard/AdminDashboardScreen.tsx Outdated
@ND68

ND68 commented Mar 9, 2026 •

Copy link
Copy Markdown
Collaborator

@greptileai

@ND68
ND68 merged commit ad51d2a into main Mar 12, 2026
5 checks passed
@ND68
ND68 deleted the gia/role-based-acct-mgt branch March 12, 2026 19:03
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.

Role Based Account Management

2 participants