feat: allow organization owners to disable and re-enable managed users (SSO users)#3759
Open
Anty0 wants to merge 3 commits into
Open
feat: allow organization owners to disable and re-enable managed users (SSO users)#3759Anty0 wants to merge 3 commits into
Anty0 wants to merge 3 commits into
Conversation
- Add owner-only PUT endpoints to disable/enable accounts of users managed by the organization - Revert the #3263 hotfix that overloaded the remove-user DELETE endpoint to disable managed users - Keep disabled managed users visible in the org member list (grayed out) with enable/disable controls - Make UserAccountService.disable/enable idempotent (no event/state change on a no-op; admin endpoints inherit this) - Add backend, service-event, and e2e tests
Contributor
📝 WalkthroughWalkthroughAdds organization-owner-scoped ChangesManaged User Disable/Enable Feature
Sequence Diagram(s)sequenceDiagram
participant OrgOwner as Org Owner (Browser)
participant MemberItem as MemberItem / DisableUserButton
participant OrgController as OrganizationController
participant OrgRoleService as OrganizationRoleService
participant UserAccountService as UserAccountService
participant DB as UserAccountRepository
OrgOwner->>MemberItem: click Disable
MemberItem->>MemberItem: open confirmation dialog
OrgOwner->>MemberItem: confirm
MemberItem->>OrgController: PUT /v2/organizations/{orgId}/users/{userId}/disable
OrgController->>OrgController: guard: reject if userId == authUser.id
OrgController->>OrgRoleService: disableUser(userId, orgId)
OrgRoleService->>OrgRoleService: isManagedBy(userId, orgId)
alt not managed by this org
OrgRoleService-->>OrgController: ValidationException (USER_IS_NOT_MANAGED_BY_ORGANIZATION)
OrgController-->>MemberItem: 400 Bad Request
else is managed
OrgRoleService->>UserAccountService: disable(userId)
UserAccountService->>DB: findActiveOrDisabled(userId)
DB-->>UserAccountService: UserAccount
UserAccountService->>UserAccountService: early-return if already disabled
UserAccountService->>DB: save(disabledAt = now)
UserAccountService->>UserAccountService: publish OnUserCountChanged(decrease)
OrgRoleService-->>OrgController: ok
OrgController-->>MemberItem: 200 OK
MemberItem->>MemberItem: invalidate /v2/organizations cache
MemberItem-->>OrgOwner: show success toast + disabled label
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
webapp/src/views/organizations/members/DisableUserButton.tsx (1)
8-9: ⚡ Quick winUse Tolgee path alias instead of a relative import.
Please switch this new import to the
tg.viewsalias to keep import style consistent with repo conventions.Suggested patch
-import { useOrganization } from '../useOrganization'; +import { useOrganization } from 'tg.views/organizations/useOrganization';As per coding guidelines, “Use Tolgee custom TypeScript path aliases (tg.component, tg.service, tg.hooks, tg.views, tg.globalContext) instead of relative imports.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/views/organizations/members/DisableUserButton.tsx` around lines 8 - 9, The import statement for useOrganization is using a relative path import (../useOrganization) instead of the Tolgee path alias convention. Replace the relative import of useOrganization with the tg.views path alias to maintain consistency with repository conventions. Specifically, change the import statement that currently uses '../useOrganization' to use the tg.views alias instead.Source: Coding guidelines
webapp/src/views/organizations/members/EnableUserButton.tsx (1)
8-9: ⚡ Quick winUse Tolgee path alias instead of a relative import.
Please switch this new import to the
tg.viewsalias for consistency with the webapp import convention.Suggested patch
-import { useOrganization } from '../useOrganization'; +import { useOrganization } from 'tg.views/organizations/useOrganization';As per coding guidelines, “Use Tolgee custom TypeScript path aliases (tg.component, tg.service, tg.hooks, tg.views, tg.globalContext) instead of relative imports.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/views/organizations/members/EnableUserButton.tsx` around lines 8 - 9, The import statement for useOrganization is using a relative path import instead of following the Tolgee path alias convention. Replace the relative import path for useOrganization with the tg.views path alias to be consistent with other imports in the codebase and align with the webapp import conventions. The messageService import on line 9 already correctly uses the tg.service alias as a reference for the proper pattern to follow.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/administration/AdministrationControllerTest.kt`:
- Line 160: In the AdministrationControllerTest class, the assertion on line 160
uses `.isNotNull` without parentheses, which is inconsistent with the method
call syntax used on line 156 with `.isNull()`. Add parentheses to the isNotNull
assertion to make it a proper method call: change `.isNotNull` to `.isNotNull()`
to match the consistent assertion pattern throughout the test.
In
`@backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/organizationController/OrganizationControllerManagedUsersTest.kt`:
- Around line 25-30: The test class prepareManagedUsersTestData method uses
`@BeforeEach` to create and save test data but lacks a corresponding `@AfterEach`
cleanup method, which violates test isolation guidelines. Add an `@AfterEach`
annotated method that uses testDataService to clean up the testData that was
saved in prepareManagedUsersTestData, ensuring each test starts with a clean
state and no test data persists between test executions.
In
`@backend/app/src/test/kotlin/io/tolgee/service/UserAccountDisableEnableEventTest.kt`:
- Around line 56-60: The saveTestData() helper method persists TestData but
lacks a corresponding cleanup hook, violating the backend test convention. Add
an `@AfterEach` annotated cleanup method that disposes of or clears the saved
TestData. Store the DisableManagedUserTestData instance returned from
saveTestData() as a class-level property so it can be accessed in the cleanup
method, then invoke the appropriate testDataService cleanup method in `@AfterEach`
to ensure proper test isolation.
In
`@backend/data/src/main/kotlin/io/tolgee/service/security/UserAccountService.kt`:
- Around line 646-650: The disable/enable user transitions in this code have a
race condition where concurrent requests can both pass the idempotency check and
emit OnUserCountChanged events. Make these operations atomic by either
implementing optimistic locking with a `@Version` field on the User entity or by
refactoring the logic in the repository to use an atomic UPDATE query that only
modifies the user if the disabledAt/enabledAt field has not already changed.
This ensures that the read-check-write-publish sequence in both the method that
sets disabledAt (around line 646) and the corresponding method that resets it
(around line 656) cannot be interrupted by concurrent requests, preserving true
idempotency.
In `@e2e/cypress/e2e/organizations/organizationsMembers.cy.ts`:
- Around line 77-85: Replace the text-dependent cy.contains('Lonely Developer')
calls with data-cy attribute-based selection using gcy() or cy.gcy(). Instead of
locating the organization-member-item row via text content, use a deterministic
data-cy selector to identify the specific member row, then use that hook to
target the organization-members-remove-user-button and
organization-members-disable-user-button elements. If the member row does not
have a suitable data-cy attribute, add one in the UI component to make it
identifiable without relying on text content. Apply this same fix to all
affected test blocks at lines 91-98, 104-111, and 117-127.
In `@webapp/src/translationTools/useErrorTranslation.ts`:
- Around line 177-178: The t() function call for the
'user_is_not_managed_by_organization' case is missing a defaultValue parameter,
which means the UI will display untranslated text before the Tolgee translations
are synced. Add a defaultValue parameter to the t() function call with a
meaningful fallback string that describes the error when the translation is not
yet available. This ensures the UI remains readable even before translations are
uploaded to Tolgee.
---
Nitpick comments:
In `@webapp/src/views/organizations/members/DisableUserButton.tsx`:
- Around line 8-9: The import statement for useOrganization is using a relative
path import (../useOrganization) instead of the Tolgee path alias convention.
Replace the relative import of useOrganization with the tg.views path alias to
maintain consistency with repository conventions. Specifically, change the
import statement that currently uses '../useOrganization' to use the tg.views
alias instead.
In `@webapp/src/views/organizations/members/EnableUserButton.tsx`:
- Around line 8-9: The import statement for useOrganization is using a relative
path import instead of following the Tolgee path alias convention. Replace the
relative import path for useOrganization with the tg.views path alias to be
consistent with other imports in the codebase and align with the webapp import
conventions. The messageService import on line 9 already correctly uses the
tg.service alias as a reference for the proper pattern to follow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7e9dc91f-f7c2-47df-97ae-23fd50508d54
📒 Files selected for processing (19)
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/organization/OrganizationController.ktbackend/api/src/main/kotlin/io/tolgee/hateoas/organization/UserAccountWithOrganizationRoleModel.ktbackend/api/src/main/kotlin/io/tolgee/hateoas/organization/UserAccountWithOrganizationRoleModelAssembler.ktbackend/app/src/test/kotlin/io/tolgee/api/v2/controllers/administration/AdministrationControllerTest.ktbackend/app/src/test/kotlin/io/tolgee/api/v2/controllers/organizationController/OrganizationControllerManagedUsersTest.ktbackend/app/src/test/kotlin/io/tolgee/service/UserAccountDisableEnableEventTest.ktbackend/data/src/main/kotlin/io/tolgee/constants/Message.ktbackend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/data/DisableManagedUserTestData.ktbackend/data/src/main/kotlin/io/tolgee/model/views/UserAccountWithOrganizationRoleView.ktbackend/data/src/main/kotlin/io/tolgee/repository/UserAccountRepository.ktbackend/data/src/main/kotlin/io/tolgee/service/organization/OrganizationRoleService.ktbackend/data/src/main/kotlin/io/tolgee/service/security/UserAccountService.kte2e/cypress/e2e/organizations/organizationsMembers.cy.tse2e/cypress/support/dataCyType.d.tswebapp/src/service/apiSchema.generated.tswebapp/src/translationTools/useErrorTranslation.tswebapp/src/views/organizations/members/DisableUserButton.tsxwebapp/src/views/organizations/members/EnableUserButton.tsxwebapp/src/views/organizations/members/MemberItem.tsx
- Use the Kotlin `.assert` form for isNotNull/isNull in the touched and surrounding organization tests - Add @AfterEach testData cleanup to the disable/enable test classes (and AdministrationControllerTest) - Fix cleanTestData to delete disabled users too (findActiveOrDisabled), so disabled test accounts don't leak across the shared test DB
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/administration/AdministrationControllerTest.kt (1)
160-168:⚠️ Potential issue | 🟡 Minor | 💤 Low valueAdd parentheses to
isNotNullon line 167 for consistency.Line 163 uses
.isNull()with parentheses, but line 167 uses.isNotNullwithout parentheses. This inconsistency within the same test should be resolved by adding()to match method call syntax.- userAccountService.findActive(testData.user.id).assert.isNotNull + userAccountService.findActive(testData.user.id).assert.isNotNull()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/administration/AdministrationControllerTest.kt` around lines 160 - 168, In the test function disable and enable are idempotent, add parentheses to the isNotNull method call to make it isNotNull() to match the syntax used in the isNull() call earlier in the same test. This ensures consistent method call syntax throughout the test function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/administration/AdministrationControllerTest.kt`:
- Around line 160-168: In the test function disable and enable are idempotent,
add parentheses to the isNotNull method call to make it isNotNull() to match the
syntax used in the isNull() call earlier in the same test. This ensures
consistent method call syntax throughout the test function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 59b6ae2c-e6d8-4984-9344-01f7b12708cf
📒 Files selected for processing (7)
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/administration/AdministrationControllerTest.ktbackend/app/src/test/kotlin/io/tolgee/api/v2/controllers/organizationController/OrganizationControllerLeavingTest.ktbackend/app/src/test/kotlin/io/tolgee/api/v2/controllers/organizationController/OrganizationControllerManagedUsersTest.ktbackend/app/src/test/kotlin/io/tolgee/api/v2/controllers/organizationController/OrganizationControllerMembersTest.ktbackend/app/src/test/kotlin/io/tolgee/api/v2/controllers/organizationController/OrganizationControllerTest.ktbackend/app/src/test/kotlin/io/tolgee/service/UserAccountDisableEnableEventTest.ktbackend/data/src/main/kotlin/io/tolgee/development/testDataBuilder/TestDataService.kt
✅ Files skipped from review due to trivial changes (1)
- backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/organizationController/OrganizationControllerLeavingTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/organizationController/OrganizationControllerManagedUsersTest.kt
- backend/app/src/test/kotlin/io/tolgee/service/UserAccountDisableEnableEventTest.kt
Anty0
changed the title
dkrizan
reviewed
Jun 19, 2026
dkrizan
left a comment
dkrizan
left a comment
Member
There was a problem hiding this comment.
@Anty0, General question - I thought the purpose of disabling a user was to restrict it only to Admins. This means we can no longer disable a user from the Admin section because any organization manager or owner can re-enable them. Am I correct? If this is the intended functionality, I’m fine with it.
Member
Author
|
Yes, for managed users, the organization manages their accounts, including whether they are disabled. We can still disable the organization owner's account - doing so would prevent the organization from re-enabling its users, so there is still a way. I don't expect us to selectively disable individual accounts within an SSO organization. If we want it separate, I could probably add a |
Member
|
My only concern is the admin moderation path, which this currently regresses. Maybe, keep What do you think ? |
Member
Author
|
Sure, I can do that. I didn't think it was worth the effort :3 |
Contributor
|
This PR is stale because it has been open for 30 days with no activity. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3275.
TLDR: This allows owners of SSO-enabled organizations to enable or disable their users. Because SSO accounts can't leave the organization that manages them, this is the only way to "get rid" of existing users.
The current implementation disables the managed account when the organization owner attempts to remove it from the organization. This behavior wasn't transparent to users, and it didn't allow the organization owner to re-enable accounts disabled this way.
Design
Remove user (existing option):
Disable user:
Re-enable user:
Summary
PUT /v2/organizations/{organizationId}/users/{userId}/disableand/enableendpoints to disable and re-enable accounts of users managed by the organization.DELETEendpoint: removing a managed user is rejected again (USER_IS_MANAGED_BY_ORGANIZATION); disabling/enabling is now done via the dedicated endpoints.disabledAt is null or managed), so admin-disabled non-managed accounts stay hidden as before.managed/disabledflags on the organization member model.UserAccountService.disable/enableidempotent (no duplicateOnUserCountChangedevent or state change on a no-op, cleanNotFoundfor a missing user). This also changes the existing admin disable/enable endpoints from erroring to a no-op on repeat calls.Summary by CodeRabbit
Summary
New Features
Behavior Changes
Tests