fix: allow disabling accounts of managed users when user cannot be removed - allows owners of organizations with sso to remove users #3263
Conversation
…moved - allows owners of organizations with sso to remove users
WalkthroughController now uses OrganizationHolder for organization context and calls removeOrDeactivateUser; repository queries exclude disabled accounts; OrganizationRoleService adds removeOrDeactivateUser and helpers; e2e data supports managed members; a Cypress test covers removing a managed user. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin as Admin User
participant API as OrganizationController
participant Holder as OrganizationHolder
participant Svc as OrganizationRoleService
participant Repo as User/Org Repos
Admin->>API: DELETE /v2/organizations/{orgId}/users/{userId}
API->>Holder: get organization
Holder-->>API: organization (id)
API->>Svc: removeOrDeactivateUser(userId, organization.id)
alt Can remove user
Svc->>Svc: canRemoveUser(userId, organizationId)
Svc->>Repo: remove membership/roles
Repo-->>Svc: persisted
Svc-->>API: success (removed)
else Managed user (cannot remove)
Svc->>Repo: disable user account (disabledAt set)
Repo-->>Svc: deactivated
Svc-->>API: success (deactivated)
end
API-->>Admin: 200 OK + message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
…test for removing managed users from organization
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/development/src/main/kotlin/io/tolgee/controllers/internal/e2eData/OrganizationE2eDataController.kt (1)
55-55: Nice refactor usingassociate.The use of
associateis more idiomatic and concise than the previousmap { ... }.toMap()pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/organization/OrganizationController.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/repository/UserAccountRepository.kt(2 hunks)backend/data/src/main/kotlin/io/tolgee/service/organization/OrganizationRoleService.kt(2 hunks)backend/development/src/main/kotlin/io/tolgee/controllers/internal/e2eData/OrganizationE2eDataController.kt(3 hunks)e2e/cypress/e2e/organizations/organizationsMembers.cy.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
e2e/cypress/e2e/organizations/organizationsMembers.cy.ts (1)
e2e/cypress/common/shared.ts (3)
gcy(28-29)confirmStandard(74-76)assertMessage(78-80)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Frontend static check 🪲
- GitHub Check: Build backend 🏗️
🔇 Additional comments (10)
backend/data/src/main/kotlin/io/tolgee/repository/UserAccountRepository.kt (1)
203-203: LGTM! The disabled account filtering is correctly applied.The addition of
ua.disabledAt is nullto bothgetAllInOrganizationandgetAllInProjectqueries ensures that disabled accounts are excluded from organization and project member listings, which aligns with the new deactivation flow for managed users.Also applies to: 227-227
backend/development/src/main/kotlin/io/tolgee/controllers/internal/e2eData/OrganizationE2eDataController.kt (2)
43-47: LGTM! Managed members are correctly processed.The logic for processing managed members mirrors the existing member processing and correctly sets the managed status, which supports the new deactivation flow for managed users.
106-106: LGTM! Test data supports the new managed user removal flow.The addition of the
managedMembersfield and corresponding test data provides the necessary setup for testing the deactivation flow when removing users managed by an organization.Also applies to: 145-154
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/organization/OrganizationController.kt (2)
255-257: LGTM! Documentation accurately reflects the new behavior.The updated operation summary clearly communicates that managed users are disabled rather than removed, which sets appropriate expectations for API consumers.
264-264: LGTM! Delegation to the service layer is appropriate.The change to
removeOrDeactivateUserdelegates the removal vs. deactivation decision to the service layer, which is the correct architectural approach.e2e/cypress/e2e/organizations/organizationsMembers.cy.ts (1)
76-85: LGTM! Test validates the managed user removal flow.The test correctly validates that the UI flow for removing a managed user works as expected, with the same user experience as removing non-managed users (from the UI perspective). The test structure is consistent with the existing "Can remove other users" test.
backend/data/src/main/kotlin/io/tolgee/service/organization/OrganizationRoleService.kt (4)
31-31: LGTM! Lazy injection breaks circular dependencies.The
@Suppress("SelfReferenceConstructorParameter")and@param:Lazyannotations are correctly applied to resolve circular dependencies and enable transactional proxy calls through theselfreference.Also applies to: 36-42
264-264: LGTM! Self-reference ensures transactional behavior.The change to
self.removeUserensures that the transactional proxy is invoked, which is necessary for proper transaction management and cache eviction.
279-316: LGTM! The removal/deactivation logic is well-structured.The new
removeOrDeactivateUsermethod correctly implements the conditional logic:
- Deactivates users when they are managed by the organization
- Delegates to
removeUserfor standard removalThe private helpers
canRemoveUserandforceRemoveUserprovide clear separation of concerns.
272-277: No further callers rely on old removeUser behavior
Only the caching test and the controller endpoint invoke removeUser; neither expects the previous silent behavior.
| @Transactional | ||
| fun removeOrDeactivateUser( | ||
| userId: Long, | ||
| organizationId: Long, |
There was a problem hiding this comment.
It seems like passing any random organizationId (even of a non-existent org) will lead to successful deactivation.
Is it ok? Just asking/nitpicking, maybe that's fine
There was a problem hiding this comment.
The getManagedBy(userId) method must return the organization with the same ID as provided in organizationId for us to decide to disable the user. Therefore, the organization must exist, and the user must be managed by it for us to allow disabling the user account. I do assume that the organizationId has been validated by the organizationHolder, making sure it isn't disabled, and that the controller has checked the user permissions. So, an arbitrary organizationId should not reach this method anyway. However, even if it does, it shouldn't allow disabling the user. Or am I missing something? ^^
I was wondering recently if I should prefer passing entities to services instead of raw IDs. From an optimization perspective, IDs seem better. However, I've encountered multiple cases where using IDs would require duplicating checks, which usually happens when the entity is fetched from the database. So, I'm inclined to prefer using entities unless speed is important for that endpoint. What do you think @bdshadow @JanCizmar?
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/organization/OrganizationController.kt
Outdated
Show resolved
Hide resolved
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/organization/OrganizationController.kt
Outdated
Show resolved
Hide resolved
backend/data/src/main/kotlin/io/tolgee/service/organization/OrganizationRoleService.kt
Outdated
Show resolved
Hide resolved
| return managedBy == null || managedBy.id != organizationId | ||
| } | ||
|
|
||
| private fun forceRemoveUser( |
There was a problem hiding this comment.
Again, I am missing documentation comment. What are we forcing and why.
There was a problem hiding this comment.
Ye, I don't like the name of this method. It's more like "remove the user without checking if we are allowed to do it". I would rename it removeUserForReal, but that feels like a bit too much :D
| private val authenticationFacade: AuthenticationFacade, | ||
| private val userAccountService: UserAccountService, | ||
| @Lazy | ||
| @param:Lazy |
There was a problem hiding this comment.
Why we are adding this @param?
There was a problem hiding this comment.
IDE was complaining. "The annotation is only applied to the parameter. An explicit annotation use-site target is recommended." I agree. It reads better. This way, I know where the annotation will be, even if I don't know the defaults. Do you think it's redundant?
|
I am also curios what happens when the company would like add the user back. Maybe they left, but after some time they come back to the company again. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/organization/OrganizationController.kt (2)
241-254: Prefer using the resolved organization id consistently
setUserRolestill accepts@PathVariable organizationIdbut now ignores it and instead relies onorganizationHolder.organization.id. Either use the resolved value for validation (compare and fail if mismatched) or drop the unused path variable to avoid confusion and keep the signature tight.
314-348: Guard against holder drift in usage endpoints
getUsagenow sources everything fromorganizationHolder.organizationEntity. Please add a sanity check (or assertion) that the holder’s id matches theorganizationIdpath variable to avoid silent mismatches if the holder is mis-populated (especially across async dispatch or custom filters).backend/data/src/main/kotlin/io/tolgee/service/organization/OrganizationRoleService.kt (1)
292-312: Revisit canRemoveUser naming/documentation
canRemoveUseractually means “user is removable without disabling,” not “user is managed by this org.” Consider renaming (e.g.,isRemovableByOrganization) or expanding the KDoc to explain all cases (unmanaged, managed by another org, managed by current org) for future maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/organization/OrganizationController.kt(6 hunks)backend/data/src/main/kotlin/io/tolgee/service/organization/OrganizationRoleService.kt(2 hunks)e2e/cypress/e2e/organizations/organizationsMembers.cy.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/cypress/e2e/organizations/organizationsMembers.cy.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-05T15:20:50.722Z
Learnt from: Anty0
PR: tolgee/tolgee-platform#3003
File: ee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/controllers/glossary/GlossaryController.kt:64-68
Timestamp: 2025-06-05T15:20:50.722Z
Learning: In the Tolgee codebase, OrganizationHolder automatically uses the organizationId path variable from Spring MVC endpoints to populate the organization context. The organizationHolder.organization.id will always equal the organizationId path parameter because OrganizationHolder handles the extraction and validation internally through the dependency injection framework.
Applied to files:
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/organization/OrganizationController.kt
📚 Learning: 2025-06-19T16:43:01.945Z
Learnt from: Anty0
PR: tolgee/tolgee-platform#3060
File: backend/security/src/main/kotlin/io/tolgee/security/authorization/FeatureAuthorizationInterceptor.kt:48-48
Timestamp: 2025-06-19T16:43:01.945Z
Learning: In the Tolgee platform codebase, organizationHolder.organization throws an exception instead of returning null when the organization is not set, so null safety checks are not needed when accessing organizationHolder.organization.id.
Applied to files:
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/organization/OrganizationController.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build backend 🏗️
🔇 Additional comments (2)
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/organization/OrganizationController.kt (1)
96-97: Confirm OrganizationHolder lifecycleDouble-check that
OrganizationHolderis populated for every endpoint using it (e.g.,/set-role,/users/{userId},/set-base-permissions,/usage). It relies on request-scoped interception—if any of these routes bypass the interceptor, the calls toorganizationHolder.organization/.organizationEntitywill throw. Please verify via integration tests or existing configuration.backend/data/src/main/kotlin/io/tolgee/service/organization/OrganizationRoleService.kt (1)
263-277: Keep leave() behavior aligned with managed usersRouting
leave()throughself.removeUserpreserves the managed-user guard (throwingUSER_IS_MANAGED_BY_ORGANIZATION). Confirm this matches product expectations; previously managed users might have been able to leave differently. If intentionally restricting them, consider documenting to avoid regressions.
| @DeleteMapping("/{organizationId:[0-9]+}/users/{userId:[0-9]+}") | ||
| @Operation(summary = "Remove user from organization") | ||
| @Operation( | ||
| summary = "Remove user from organization", | ||
| description = ( | ||
| "Remove user from organization. " + | ||
| "If user is managed by the organization, their account is disabled instead." | ||
| ) | ||
| ) | ||
| @RequiresOrganizationRole(OrganizationRoleType.OWNER) | ||
| @RequiresSuperAuthentication | ||
| fun removeUser( | ||
| @PathVariable("organizationId") organizationId: Long, | ||
| @PathVariable organizationId: Long, | ||
| @PathVariable("userId") userId: Long, | ||
| ) { | ||
| organizationRoleService.removeUser(userId, organizationId) | ||
| organizationRoleService.removeOrDeactivateUser(userId, organizationHolder.organization.id) | ||
| } |
There was a problem hiding this comment.
Propagate organization-role cache eviction when deactivating managed users
When removeUser routes to removeOrDeactivateUser, managed accounts are disabled but their organization role and related caches remain untouched. This keeps stale entries in Caches.ORGANIZATION_ROLES, so disabled members can still appear where cached data is used (e.g., hasAnyOrganizationRole, seat counters). Please ensure the disable branch also evicts/cleans membership artifacts.
| @Transactional | ||
| fun removeOrDeactivateUser( | ||
| userId: Long, | ||
| organizationId: Long, | ||
| ) { | ||
| if (!canRemoveUser(userId, organizationId)) { | ||
| userAccountService.disable(userId) | ||
| return | ||
| } | ||
|
|
||
| removeUserForReal(userId, organizationId) | ||
| } |
There was a problem hiding this comment.
Disable branch needs the same cleanup as hard removal
In removeOrDeactivateUser, the managed-user branch calls userAccountService.disable(userId) and returns without removing organization roles, project permissions, or evicting caches. This leaves the managed member tied to the organization (blocking seat release and keeping stale cached roles). Align both branches by reusing removeUserForReal or introducing a complementary cleanup for the disable path.
🤖 Prompt for AI Agents
In
backend/data/src/main/kotlin/io/tolgee/service/organization/OrganizationRoleService.kt
around lines 279-290, the branch that calls userAccountService.disable(userId)
returns without performing the same cleanup as the hard-removal path (removing
organization roles, revoking project permissions, evicting caches, releasing
seats). Change the flow so the disable path performs identical cleanup: either
call removeUserForReal(userId, organizationId) after disabling, or extract the
cleanup steps from removeUserForReal into a shared private method and call that
from both branches (disable branch and hard-remove branch) to remove org roles,
revoke project permissions, evict related caches, and ensure the organization's
seat is freed.
There was a problem hiding this comment.
That's intentional—removing the user does that. Disabling the user should be reversible, so the user stays unmodified.
There was a problem hiding this comment.
@Anty0, understood—thank you for clarifying! I see now that the disable path is intentionally lightweight to support re-enabling users if they return to the organization, while hard removal performs the full cleanup. That design makes sense given the use case.
٩(◕‿◕)۶
✏️ Learnings added
Learnt from: Anty0
PR: tolgee/tolgee-platform#3263
File: backend/data/src/main/kotlin/io/tolgee/service/organization/OrganizationRoleService.kt:279-290
Timestamp: 2025-10-07T14:36:39.280Z
Learning: In OrganizationRoleService in backend/data/src/main/kotlin/io/tolgee/service/organization/OrganizationRoleService.kt, the `removeOrDeactivateUser` method intentionally has two distinct paths: if a user can be removed, it calls `removeUserForReal` (which removes organization roles, project permissions, and evicts caches); if a user is managed by the organization and cannot be removed, it calls `userAccountService.disable(userId)` without any cleanup. This design is intentional—disabling should be reversible so the user stays unmodified (keeping their organization roles and project permissions intact), allowing them to be re-enabled if they return to the organization.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Summary by CodeRabbit
New Features
Documentation
Tests