OpenConceptLab/ocl_online#307 | User profile sync to django | job to sync groups from Keycloak to django

[snyaggarwal]

Linked Issue

Closes OpenConceptLab/ocl_online#307

[snyaggarwal]

Verdict: Request changes

Issue complete: no — the targeted tests pass, but the PR currently fails pylint on new/changed lines.

Findings:

1. core/services/auth/openid.py:186 trips R0913: Too many arguments (6/5) for update_user_profile(...). Please reduce the argument count, for example by passing the user/profile object or a small profile payload, or add a narrowly scoped pylint disable if this signature is intentionally preferred.
2. core/common/backends.py:88 and core/users/tests/tests.py:383 trip W0212: protected-access for _from_sso. Please avoid reaching into a protected ad-hoc attribute from outside the model API, or make the suppression explicit and local if this sentinel is intentionally private.

Verification:

• docker exec oclapi2-api-1 python manage.py test core.users.tests.tests.KeycloakSyncTest --keepdb -v3 — passed.
• docker exec oclapi2-api-1 pylint -j2 core/common/tasks.py core/services/auth/openid.py core/users/models.py core/common/management/commands/sync_keycloak_groups.py core/common/signals.py core/common/backends.py core/users/tests/tests.py — failed with the pylint errors above.

(snyaggarwal • Codex/GPT-5)

[snyaggarwal]

Verdict: No blocking findings in this pass.

The earlier pylint blockers are fixed in 6d7438f (too-many-arguments and protected-access are now locally suppressed), and I did not find a new behavioral blocker in the refreshed diff.

Review notes:

• The graph review now maps the PR and flags the Keycloak admin helpers (get_service_token, _admin_request, _paginate) as the highest-risk area, mostly because those network/admin paths are not directly unit-tested. I would consider adding focused tests for token caching, pagination, and group-member aggregation, but I am not blocking on that here.
• The group sync behavior keeps non-Keycloak/Django-only groups and replaces only matching Keycloak-managed groups as intended by the current implementation.

Verification:

• docker exec oclapi2-api-1 python manage.py test core.users.tests.tests.KeycloakSyncTest core.common.tests.OCLOIDCAuthenticationBackendTest --keepdb -v3 — passed.
• docker exec oclapi2-api-1 pylint -j2 core/common/tasks.py core/services/auth/openid.py core/users/models.py core/common/management/commands/sync_keycloak_groups.py core/common/signals.py core/common/backends.py core/users/tests/tests.py — passed, 10.00/10.
• git diff --check origin/master...HEAD — passed.

(snyaggarwal • Codex/GPT-5)

[paynejd]

Claude review (commit 6d7438f). I checked this against the acceptance criteria in OpenConceptLab/ocl_online#307 and the Keycloak 19.0.1 admin API source, and ran the sync's logic read-only against production (numbers on ocl_online#307). A separate adversarial pass by a fresh Claude agent is folded in.

Verdict: hold the merge for two P1s; the rest is small. The sync's decisions look right. It stops when Keycloak can't be read, it only touches groups that exist on both sides, and the beat task lands on the default queue. The problems are how it identifies users and where the write credential lives.

	Finding	Inline
P1	The sync and the push identify Keycloak users by username. Please match on the Keycloak sub instead, and hold the merge until OpenConceptLab/ocl_online#435 is fixed	tasks.py:1246
P1	The setup puts a manage-users credential in the API's environment for the profile push. The group sync only needs view-users	settings.py:641, models.py:459
P2	_from_sso stays set on request.user for the rest of the request, so a profile edit in that request isn't pushed	backends.py:88 (suggestion)
P2	Groups Keycloak doesn't have are kept, unlike the web sign-in path; deleting a Keycloak group never revokes it from token-only users	tasks.py:1253
P3	--dry-run prints only a count; the per-user changes go to the task logger	sync_keycloak_groups.py:15

P1, identity. Today a user's Keycloak groups reach Django only when that user signs in on the web. This job applies them every 15 minutes to whichever Django account has the matching username. So the match should be on the identifier Keycloak issues (sub), not on a Django field. The details are on the private ticket.

P1, credential scope. manage-users is realm-wide user administration. This job also makes Keycloak membership of staff_user and superadmin_user decide is_staff and is_superuser within 15 minutes, with no sign-in needed. So the credential should stay out of the internet-facing API. Suggested shape:

• Make the push a Celery task enqueued on commit (sketch inline on models.py).
• Set OIDC_ADMIN_CLIENT_* only on the worker that consumes default, where the sync already runs.
• Keep the service token in process memory rather than the shared Redis cache, which the API can reach.

Then a profile save also stops waiting on up to three Keycloak calls with a 15 s timeout each.

Rollout. The beat task applies changes on its first tick. So in each environment, before the credential goes into the worker's environment:

1. Run sync_keycloak_groups --dry-run with the credential passed to that one-off run, and read the removals.
2. Check that ocladmin is a member of superadmin_user and staff_user in that realm. If it isn't, the sync demotes it every 15 minutes and loaddata restores it at every container start.

In production both checks pass, and the first run is almost all additions. The few removals are Django-only grants on dormant accounts.

Smaller notes, not blocking:

• Keycloak lowercases usernames, and both paths match usernames exactly. A legacy Django account with capitals therefore never receives a Keycloak group, and the job removes any it holds. Matching on sub fixes this too.
• The HTTP helpers (get_service_token, _paginate, get_group_memberships, update_user_profile) have no tests, as your own pass noted. A test of _paginate at exactly PAGE_SIZE results would be cheap.
• A cap on removals per run (log and stop if one run would remove more than N memberships) would guard against a wrong realm or client config, since these groups carry staff and plan limits.

[paynejd]

Dependency: this PR depends on #935, which rejects username changes through the user API under SSO. Please merge and deploy #935 before this one.

The other identity items from my review are tracked on OpenConceptLab/ocl_online#435 (core team only). They need to be done before this merges. The rest of the review stands.

[snyaggarwal]

Review feedback addressed in 10fb982.

P1, identity. Added UserProfile.sso_id (migration 0037), set from the sub claim at OIDC sign-in. The group sync and the profile push both match on it. They fall back to the lowercased username only for accounts with no sso_id yet, which also covers legacy usernames with capitals.

P1, credential scope. A profile save now only does the dirty check, then enqueues push_user_profile_to_keycloak on commit. The task reads the row when it runs, so the latest save wins. The service token is kept in process memory instead of the shared Redis cache. The API no longer calls Keycloak, so OIDC_ADMIN_CLIENT_* only needs to be set on the worker that consumes default. The push needs manage-users and the group sync only view-users; the setup steps on ocl_online#307 need updating to say this.

P2, _from_sso. Cleared after the save in update_user, so later saves in the same request are pushed.

P2, groups Keycloak doesn't have. Kept as is (background job leaves them alone). Revocation behaviour will go into the usage-limits doc.

P3, dry run. sync_keycloak_groups --dry-run now prints per-group gains and losses and any is_staff/is_superuser change.

Tests. Added coverage for _paginate (including exactly PAGE_SIZE followed by an empty page), the in-memory token, group-member aggregation, the profile merge by sso_id, sso_id matching in the sync and the push task. The core.users suite and the OIDC backend tests pass, and pylint is 10/10 on the touched files.

Still open.

• sso_id is empty for existing users until they next sign in, so token-only accounts match by username until a backfill (ocl_online#435).
• Not added: a cap on removals per run.
• OpenConceptLab/ocl_online#435 | Reject username changes through the user API under SSO #935 must merge and deploy first.

[paynejd]

Claude follow-up review (commit 10fb982), after your reply. I checked each point against my first review and the identity criterion on OpenConceptLab/ocl_online#435. I also ran a Codex pass, plus the Keycloak tests and five probe tests in an isolated stack. #935 is merged and in the build that production and staging run, so that dependency is met.

Verdict: one item still holds the merge. The worker move, the in-memory token, the _from_sso fix, the dry-run output and the new tests all look right. The sync and the push now match on sso_id, but sign-in doesn't, and the #435 criterion names filter_users_by_claims too. So sso_id records whoever last signed in under a username; it doesn't pin the account. The three inline suggestions and the sketch below were tested together. With them, the PR's Keycloak and OIDC backend tests and the probes pass, and pylint scores 10/10.

	First review	Now
P1	Identity: match on sub	Sync and push ✔. Sign-in still matches on username: R1
P1	Credential scope	✔ in code. The setup steps on ocl_online#307 still need updating
P2	_from_sso	✔
P2	Groups Keycloak doesn't have	Kept by design, for the usage-limits doc
P3	Dry run	✔

R1 (P1, the #435 gate): link on sub at sign-in too. filter_users_by_claims (backends.py:106-131) still matches on username alone, and update_user (:82) replaces a stored sso_id with whatever sub arrives. The probes show two effects:

• A sign-in whose sub differs from the account's sso_id is matched to that account, and its sso_id is rewritten.
• A Keycloak user whose username has changed can't sign in. Nothing matches the new username, so create_user hits the unique constraint on sso_id, a 500. If a Django account does hold the new username, update_user replaces that account's groups before the save fails, and with no transaction around it, the change stays.

Suggested shape for filter_users_by_claims, tested as written. DeactivatedAccountLoginRefused stands in for a more general refusal:

        sub = claims.get('sub')
        # evaluated once, so the row checked here is the one the caller updates
        users = UserProfile.objects.filter(sso_id=sub) if sub else UserProfile.objects.none()
        if not users:
            users = UserProfile.objects.filter(username=username)
            if users and sub and users[0].sso_id not in (None, sub):
                logger.warning('OIDC sign-in refused: user %s is linked to another Keycloak user', users[0].id)
                raise DeactivatedAccountLoginRefused()
        user = users[0] if users else None

The same rule belongs in both username fallbacks: a Keycloak user whose id is already stored on an account never matches a different account by username (inline on tasks.py and openid.py).

R2 (P2, rollout): backfill sso_id before the credential is set. Your reply says sso_id fills in at a user's next sign-in. But this job exists for the users who don't sign in on the web. For them it stays empty, so the sync keeps matching them on username indefinitely. A one-off backfill would pin today's links. For each active account with no sso_id, it stores the id of the Keycloak user with exactly that username, and a dry run lists what it would set first. It can come in this PR or a follow-up. The job is a no-op until OIDC_ADMIN_CLIENT_* is set, so the order in each environment can be:

1. the check for past renames on #435, so the backfill doesn't pin a bad link;
2. the backfill, dry run first;
3. sync_keycloak_groups --dry-run;
4. the credential, on the worker only.

R3 (P2): one rule for case. The reply says the fallback lowercases usernames and so covers legacy capitals. That holds for the push (openid.py:184), not the sync, which compares exactly (tasks.py:1248, :1280). So an unlinked mixed-case account:

• loses every Keycloak-managed group on each run, even one added by hand with add_users_to_group;
• sends its profile edits to the Keycloak user with the lowercased name, which can already be linked to a lowercase twin.

In production, 14 pairs of active accounts differ only by case. Each pair shares an email, so the push wouldn't cross between people today. I'd make both exact (inline on openid.py). That matches sign-in, which by design never matches a mixed-case account. The mixed-case token-only accounts then become a decision for the backfill. Either link each to its lowercased Keycloak user where that match is unique, listed in the dry run, or leave them out and say so in the usage-limits doc.

Setup steps. The steps on ocl_online#307 still say to set OIDC_ADMIN_CLIENT_* "for oclapi2". Please update them before anyone sets the credential. The secret goes only on the worker that consumes default, never on the API. view-users is for the sync and manage-users for the push.

Codex pass 2 (codex-cli 0.162.1, commit 10fb982). Verdict: findings. Earlier findings: F1.2, F1.3 and F1.5 resolved; F1.1 open; F1.4 open (kept by design).

• F2.1 (P1): sign-in still matches on username and can replace a stored sso_id; a renamed Keycloak user hits the unique constraint. → R1.
• F2.2 (P1): the sync's username fallback can give an already-linked Keycloak user's groups to a second account. → R1, inline on tasks.py.
• F2.3 (P1): the push lowercases, so an unlinked mixed-case account can write to its lowercase twin's Keycloak user. → R3. I rate it P2, since no cross-person pair exists in production.
• F2.4 (P1): the sync doesn't lowercase, so the reply's claim about legacy capitals holds only for the push. → R3, P2: the backfill can settle it.
• F2.5 (P2): deleting a Keycloak group, superadmin_user included, leaves token-only holders with it. → Accepted as in the first review. The doc should say that staff access is revoked by removing members, never by deleting the group.

The full Codex output is on #435, because its scenarios describe the linking issue.

[snyaggarwal]

Follow-up review addressed in 7ed4f1c, on top of the batched suggestions in c4184ee.

R1, sign-in links on sub. filter_users_by_claims now looks for the account linked to the claims' sub first and falls back to the username only when none is linked. If the username matches an account linked to a different Keycloak user, the sign-in is refused with a new SSOIdentityMismatchRefused, with its own message instead of the deactivated-account one. Stored OIDC tokens are cleared on that refusal as well. A renamed Keycloak user now signs in to its linked account. The three inline suggestions (stored sso_id kept, sync fallback skipping linked ids, exact-username push) came in through c4184ee.

R2, backfill. New backfill_sso_ids [--dry-run] links each active account without an sso_id to the Keycloak user with exactly its username. Accounts with no exact match (mixed-case ones included) and Keycloak users already linked elsewhere are listed and left alone.

R3, case. The sync and the push both compare exactly now, matching sign-in. Mixed-case token-only accounts stay unlinked and are listed by the backfill dry run, so they remain a decision for it.

Tests. Added for sign-in by sub after a username change, refusal on a different sub, first-time linking, a stored sso_id being kept, and the backfill including its dry run. The core.users suite and the OIDC backend tests pass, and pylint is 10/10 on the touched files.

Rollout, per environment. Setting the credential comes last:

1. Check ocl_online#435 for past renames.
2. backfill_sso_ids --dry-run, then the real run.
3. sync_keycloak_groups --dry-run.
4. Set OIDC_ADMIN_CLIENT_* on the default worker only. view-users covers the sync and manage-users the push.

I'll update the setup steps on ocl_online#307 to say this. Deleting a Keycloak group doesn't revoke it from token-only users, so staff access is revoked by removing members, and that goes in the usage-limits doc.

[paynejd]

Claude follow-up review 2 (commit 7ed4f1c), after your reply. I reran last round's probes against this commit in an isolated stack and added new ones for the refusal and the backfill. CI is green.

Verdict: the identity gate is met. Sign-in looks up sso_id first. A username whose account is linked to a different Keycloak user is refused, and a stored link is never replaced. The sync and the push follow the same rule. The probes show it: a renamed Keycloak user signs in to its linked account, a different sub is refused with the link kept, and neither the sync nor the push reaches an account linked to someone else. Two P2s are left. Both are small and have tested fixes below.

	10-14 review	Now
R1 (P1)	Sign-in links on sub	✔
R2 (P2)	Backfill sso_id	✔ for exact usernames. Mixed-case accounts: B1 below
R3 (P2)	One rule for case	✔ exact in sign-in, sync and push
Setup	Steps on ocl_online#307	Still open (see Rollout)

M1 (P2): the API middleware reads the new refusal as anonymous access. RequireAuthenticationMiddleware.get_authenticated_user re-raises only DeactivatedAccountLoginRefused (middlewares.py:172). SSOIdentityMismatchRefused falls into the except AuthenticationFailed below it, so the request is treated as anonymous. Take an API client with a Keycloak bearer token and no approved client header. It gets the "Anonymous API access is disabled" 403 with the pricing link, not the 401 that says why, which ocl_online#339 set up for refused sign-ins. TBv3 and the Mapper send an approved X-OCL-CLIENT, so they reach DRF and get the right message. These lines aren't in the diff, so here is the change:

# core/middlewares/middlewares.py:18
from core.common.exceptions import DeactivatedAccountLoginRefused, SSOIdentityMismatchRefused
# :133
        except (DeactivatedAccountLoginRefused, SSOIdentityMismatchRefused) as ex:
# :172
        except (DeactivatedAccountLoginRefused, SSOIdentityMismatchRefused):

Here is a test to go next to test_rejects_sign_in_refused_for_deactivated_account (tests.py:74-75), with SSOIdentityMismatchRefused added to the import on line 10. It fails at this commit and passes with the change:

    @patch('core.middlewares.middlewares.OCLAuthentication.authenticate')
    def test_rejects_sign_in_refused_for_identity_mismatch(self, authenticate_mock):
        """A sign-in refused for a username linked to another Keycloak user gets the same 401 that says why."""
        authenticate_mock.side_effect = SSOIdentityMismatchRefused()

        response = self.middleware(self.make_request('/user/', HTTP_AUTHORIZATION='Bearer sso-token'))

        self.assertEqual(response.status_code, 401)
        self.assertEqual(response['WWW-Authenticate'], 'Bearer realm="api"')
        self.assertIn('different sign-in identity', json.loads(response.content)['detail'])

B1 (P2): the backfill doesn't yet follow the 10-14 decision on ocl_online#435. That comment was for this command. It decided:

• A mixed-case account links to its lowercase Keycloak user when no other account has the same username in another case and the emails match (case-insensitive). The case-twin pairs stay as they are.
• The dry run lists every account it would link, not only a count.

The mixed-case accounts are among the token-only users ocl_online#307 is for. Web sign-in doesn't reach them, so without a link they never get a Keycloak grant. Runtime matching stays exact: the backfill is the one place case gets settled.

The inline suggestion on backfill_sso_ids.py does this. Each skip gets a reason, so the dry run can be checked line by line. The write is guarded with sso_id__isnull=True, so a sign-in that links the account during the run isn't overwritten. The second suggestion adds a test. Your two backfill tests pass unchanged with it, and so does the new one. Pylint scores 10/10. It can come in this PR or a follow-up, as long as it lands before the backfill runs anywhere: the command fills only empty sso_ids, so a rerun after a fix would still link the skipped accounts.

Rollout. The order in your reply is right. Two things remain before the credential goes on anywhere:

• The rename check on ocl_online#435 must run before the backfill pins today's links.
• The setup steps on ocl_online#307 still say to set OIDC_ADMIN_CLIENT_* "for oclapi2". They should say: on the worker that consumes default only, never the API, with view-users for the sync and manage-users for the push.

Codex pass 3 (codex-cli 0.162.1, commit 7ed4f1c). Verdict: findings. F2.1 to F2.4 are resolved. F2.5 is open, accepted by design: the usage-limits doc will say to revoke staff access by removing members, never by deleting the group.

• F3.1 (P1): the backfill skips the mixed-case accounts the decision says to link. → B1. I rate it P2, because the command fills only empty sso_ids, so a fixed rerun catches up.
• F3.2 (P1): the middleware swallows SSOIdentityMismatchRefused. → M1. I rate it P2: the request is still refused, only with the wrong message.
• F3.3 (P1): the dry run doesn't list the accounts it would link. → B1.
• F3.4 (P2): the backfill's write can replace a link a sign-in made during the run. → Fixed in the B1 suggestion (sso_id__isnull=True on the update).
• F3.5 (P2): two concurrent first sign-ins on the same unlinked account by two different Keycloak users can both pass the check, and the second replaces the first's link. → Accepted as P3. It needs a Keycloak username to change hands between two requests in flight.

The full Codex output is on ocl_online#435, next to the earlier passes.

Once M1 and B1 are in, this is ready to approve from my side.

[paynejd]

Approved at 7ade5bd. The two remaining fixes are in, and they're the code I tested: the middleware returns the 401 for an identity-mismatch refusal, and the backfill links mixed-case accounts and lists every link in its dry run. CI is green.

Merging switches nothing on: the sync is a no-op until OIDC_ADMIN_CLIENT_* is set, and the backfill is run by hand. Before the credential is set in any environment, please update the setup steps on ocl_online#307 (the default worker only) and close out the open items on ocl_online#435, then follow the rollout order in your reply.