Skip to content

fix: sync user removal with external auth provider#46

Open
abhizipstack wants to merge 3 commits intomainfrom
fix/user-deletion-scalekit-sync
Open

fix: sync user removal with external auth provider#46
abhizipstack wants to merge 3 commits intomainfrom
fix/user-deletion-scalekit-sync

Conversation

@abhizipstack
Copy link
Copy Markdown
Contributor

@abhizipstack abhizipstack commented Apr 7, 2026

Summary

  • After deleting a user's OrganizationMember DB record, the controller now calls auth_service.remove_users_from_organization() to sync the removal with the external auth provider
  • Uses the provider's user_id instead of Django integer PK
  • Aligns OSS AuthenticationService stub signature with the cloud service interface
  • Adds continue after "No membership found" to skip provider sync on failed DB deletions

Test plan

  • Remove a user from an organization via UI
  • Verify user is removed from both local DB and external auth provider
  • Verify OSS mode still works (stub is a no-op)
  • Verify removing a non-existent user returns proper error without calling auth provider

🤖 Generated with Claude Code

When removing users from an organization, the controller now calls
the auth service to delete the membership from the external provider
(e.g., Scalekit) after the local DB record is deleted. Uses the
Scalekit user_id instead of Django PK. Also aligns OSS stub signature
with the cloud ScalekitService interface.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Apr 7, 2026

Greptile Summary

This PR adds external auth provider sync to user removal — after a successful OrganizationMember DB deletion the controller reads user.user_id (the Scalekit-side ID) and calls auth_service.remove_users_from_organization(), surfacing any sync failure as a "partial" entry in failed_removals. A continue guard prevents the provider call when the DB deletion found no record. The OSS AuthenticationService stub is updated to match the new three-argument interface.

Key changes:

  • authentication_controller.py: Adds continue after "No membership found", then syncs removal with external auth provider via user_id, and appends "partial" to failed_removals on sync failure
  • authentication_service.py: Updates OSS stub signature from (admin, org_id, user_emails) to (org_id, user, user_id) returning bool, aligning with cloud service interface

Confidence Score: 5/5

This PR is safe to merge — implementation is correct and both previous review concerns are fully resolved

Both prior P1 concerns (silent partial failure and Django PK fallback) are now addressed. No new issues found. The user_id empty-string edge case is correctly handled via falsy check.

No files require special attention

Important Files Changed

Filename Overview
backend/backend/account/authentication_controller.py Adds auth provider sync after DB removal, with correct guard on user_id and partial-failure reporting
backend/backend/account/authentication_service.py Updates OSS stub signature to match new 3-arg interface (org_id, user, user_id) returning bool no-op

Sequence Diagram

sequenceDiagram
    participant Caller
    participant Controller as AuthController
    participant DB
    participant AuthService as auth_service

    Caller->>Controller: remove_users_from_organization(admin, org_id, emails)
    loop for each email
        Controller->>DB: User.objects.get(email)
        alt User not found
            DB-->>Controller: DoesNotExist
            Controller->>Controller: append failed("User not found"), continue
        else User found
            DB-->>Controller: user
            Controller->>DB: OrganizationMember.filter(...).delete()
            alt No membership
                DB-->>Controller: deleted_count=0
                Controller->>Controller: append failed("No membership found"), continue
            else Membership deleted
                DB-->>Controller: deleted_count>0
                Controller->>Controller: user_id = getattr(user, "user_id", None)
                alt user_id is falsy
                    Controller->>Controller: skip provider sync
                else user_id is truthy
                    Controller->>AuthService: remove_users_from_organization(org_id, user, user_id)
                    alt Success
                        AuthService-->>Controller: True
                    else Exception
                        AuthService-->>Controller: raises
                        Controller->>Controller: append partial("auth provider sync failed")
                    end
                end
            end
        end
    end
    Controller-->>Caller: failed_removals list
Loading

Reviews (3): Last reviewed commit: "fix: address Greptile review — report pa..." | Re-trigger Greptile

When removing users from an organization, the controller now calls
the auth service to delete the membership from the external auth
provider after the local DB record is deleted. Uses the provider's
user_id instead of Django PK. Also aligns OSS stub signature with
the cloud service interface.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@abhizipstack abhizipstack changed the title fix: sync user removal with auth provider (Scalekit) fix: sync user removal with external auth provider Apr 7, 2026
…hout provider ID

- Append to failed_removals when auth provider sync fails (prevents
  silent split-brain state)
- Skip provider sync entirely when user has no provider user_id
  (avoids guaranteed-to-fail API call in OSS mode)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@abhizipstack
Copy link
Copy Markdown
Contributor Author

Both Greptile comments addressed in 8c7e316:

P1 (split-brain): Partial failures now appended to failed_removals so the caller surfaces the error.

P2 (fallback to Django PK): Now guards with if user_id and skips the provider call entirely when no provider ID is present (OSS mode).

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.

2 participants