fix: dedupe dandiset owners on PUT /users/ (closes #2831) - #2835
Draft
yarikoptic wants to merge 4 commits into
Draft
fix: dedupe dandiset owners on PUT /users/ (closes #2831)#2835yarikoptic wants to merge 4 commits into
yarikoptic wants to merge 4 commits into
Conversation
…i#2831) The PUT /dandisets/{id}/users/ handler resolved each requested username twice -- once via User.username and once via SocialAccount.extra_data.login -- and concatenated the result lists without deduplication. When a single input string matched both queries (or matched more than one row in either), multiple distinct Django User rows were granted owner permission for one requested username. In dandi#2831 this surfaced as the same GitHub user appearing twice in the owners list of dandiset 001849 after a Manage Owners save. The fix resolves each username to at most one User, with explicit precedence: a matching SocialAccount.extra_data['login'] wins over a Django User.username (the latter is the user's email in the typical allauth-via-resonant-settings setup, but legacy/manually-created users may exist with usernames that look like GitHub logins). When two SocialAccount rows share a login (e.g. a freed GitHub handle was reused), pick the one whose user is most recently active. After resolution, the resulting users are also deduped by User.pk to handle the case where two distinct input strings resolve to the same Django user. Also harden the GET path: SocialAccount.objects.get(user=...) raised MultipleObjectsReturned if a user ever ended up with more than one social account; use .filter(...).first() instead. Tests added cover: - Django username colliding with another user's SocialAccount login. - Two SocialAccount rows sharing the same extra_data['login']. - Duplicate usernames in the PUT payload. Note: full pytest suite was not executed in the dev sandbox (no docker-compose stack); ruff check + ruff format --check pass on the modified files. Reviewer/CI should run the new test cases end-to-end. Co-Authored-By: Claude Code 2.1.138 / Claude Opus 4.7 <noreply@anthropic.com>
`DandisetOwnersDialog` previously synced `newOwners` from `store.owners` via `Object.assign(newOwners.value, store.owners)`. Two foot-guns: 1. `Object.assign` copies own enumerable properties; it does not reset `length`. If `store.owners` shrinks, the trailing local entries silently linger in `newOwners`. 2. If the backend ever returns duplicate-looking owners (same username appearing twice -- the symptom in dandi#2831 before the API fix), the dialog would show both cards as separate entries even though they are indistinguishable to the user. Replace the assignment with a fresh, by-username deduped copy. Also guard `addSelected()` against re-adding a user who is already in `newOwners` (the previous `.concat()` would happily duplicate). The backend is the source of truth -- the corresponding API fix already prevents the duplicate-owner scenario from occurring on writes. This commit is defense-in-depth for any pre-existing duplicate state or similar future regressions. Co-Authored-By: Claude Code 2.1.138 / Claude Opus 4.7 <noreply@anthropic.com>
The three new tests added in 8ff0f50 used `DandisetFactory.create(owners=[...])` and reached the full PUT happy path, which calls `send_ownership_change_emails(dandiset, ...)`. That accesses `dandiset.draft_version.name`, blowing up with `Version.DoesNotExist` because the dandiset had no draft version. Switch to the same `DraftVersionFactory.create(dandiset__owners=[...]).dandiset` idiom that `test_dandiset_rest_add_owner` and `test_dandiset_rest_remove_owner` already use for the happy path. Co-Authored-By: Claude Code 2.1.138 / Claude Opus 4.7 <noreply@anthropic.com>
`get_dandiset_owners()` orders by `date_joined`, so when the test's `real_user` is created before `requesting_user` the response lists "yarikoptic" before "matthew68", failing an order-sensitive assertion. Compare as sets — the bug under test is about *which* users are granted, not the order in which they are listed. Co-Authored-By: Claude Code 2.1.138 / Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression in the dandiset owners API where a single requested username could resolve to multiple User rows (via User.username and SocialAccount.extra_data['login']) and incorrectly grant duplicate owner permissions. It also adds frontend and backend hardening to prevent duplicate-looking owners from accumulating in the UI and to avoid 500s when multiple social accounts exist.
Changes:
- Backend: resolve each
PUT /api/dandisets/{id}/users/username to at most oneUser(preferSocialAccountlogin), and dedupe resolved owners byUser.pk. - Backend: make
GET /users/tolerate multipleSocialAccountrows per user by using.filter().first()instead of.get(). - Frontend: dedupe owners displayed/edited in the “Manage Owners” dialog and prevent adding duplicate selections.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
dandiapi/api/views/dandiset.py |
Reworks owner resolution/dedup on PUT and hardens GET owner serialization against multiple social accounts. |
dandiapi/api/tests/test_dandiset.py |
Adds regression tests covering username/login collisions, shared logins across social accounts, and duplicate usernames in payload. |
web/src/components/DLP/DandisetOwnersDialog.vue |
Re-syncs owners via a fresh deduped array and prevents adding duplicate owners client-side. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+557
to
+562
| social_by_login: dict[str, User] = { | ||
| acc.extra_data['login']: acc.user | ||
| for acc in SocialAccount.objects.select_related('user') | ||
| .filter(extra_data__login__in=usernames) | ||
| .order_by('-user__last_login', '-user__date_joined') | ||
| } |
Comment on lines
+596
to
+598
| # A user can in principle have more than one SocialAccount; use the first | ||
| # rather than .get() so we don't raise MultipleObjectsReturned here. | ||
| owner_account = SocialAccount.objects.filter(user=owner_user).first() |
| if (seen.has(u.username)) return false; | ||
| seen.add(u.username); | ||
| return true; | ||
| }); |
Comment on lines
533
to
535
| @action(methods=['GET', 'PUT'], detail=True) | ||
| def users(self, request, dandiset__pk): # noqa: C901 | ||
| def users(self, request, dandiset__pk): | ||
| dandiset: Dandiset = self.get_object() |
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
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 #2831.
Important
After this lands, an admin still needs to remove the duplicate owner row on dandiset 001849 — the backend can't tell which Yaroslav
Useris the "real" one. A one-offfind_duplicate_ownersaudit (see Test plan) would help find any other dandisets that hit the same bug before the fix.Summary
PUT /api/dandisets/{id}/users/was resolving each requested username through two parallel queries (User.usernameANDSocialAccount.extra_data['login']) and concatenating the results without deduplication. When a single input string matched both — or matched multipleSocialAccountrows sharing a login — multiple distinctUserrows were granted theownerpermission for what the user typed once. In #2831 this surfaced as the same GitHub user appearing twice in the owners list of dandiset 001849 right after a "Manage Owners" save.The regression was introduced in #1737 (commit 7eff07b, 2023-11), which replaced a precedence-aware
get_user_or_400(username)helper with bulkUser.objects.filter+SocialAccount.objects.filterqueries to cut N+1s — but lost the "exactly one user per username, SocialAccount wins" invariant.This PR restores that invariant on the backend, hardens the GET path against multiple social accounts per user, and adds defensive dedup in the Manage Owners dialog so this kind of state can't compound client-side.
Commits
fix(api): resolve each PUT /users/ username to at most one User (#2831)fix(web): defensively dedup owners in Manage Owners dialog (#2831)Per-commit details
8ff0f50
fix(api): resolve each PUT /users/ username to at most one User (#2831)dandiapi/api/views/dandiset.py:social_by_login(ordered by-user__last_login, -user__date_joinedso the choice is deterministic when two SocialAccount rows share a login — pick the user with the most recent activity) andfallback_by_username(DjangoUser.username, which is the user's email in this app). For each requested username, prefer the SocialAccount match; fall back to Django username; raiseValidationError("User X not found")if neither matches. Finally dedup the resolved list byUser.pkso two distinct input strings that resolve to the sameUserdon't double-up.SocialAccount.objects.get(user=...)with.filter(...).first()so a user with more than one social account doesn't 500 withMultipleObjectsReturned.dandiapi/api/tests/test_dandiset.py— three regression tests:test_dandiset_rest_add_owner_username_collides_with_other_user_login— a legacyUserwhoseusernamehappens to equal another user's GitHub login does not double-grant.test_dandiset_rest_add_owner_login_shared_by_two_social_accounts— twoSocialAccountrows sharingextra_data['login']resolve deterministically to the most-recently-active user, not both.test_dandiset_rest_add_owner_duplicate_usernames_in_payload— the same username repeated in the PUT body yields one ownership.ae38099
fix(web): defensively dedup owners in Manage Owners dialog (#2831)web/src/components/DLP/DandisetOwnersDialog.vue:Object.assign(newOwners.value, store.owners)with a fresh, by-username-deduped copy. The previous form did not resetlength, so a shrinkingstore.ownerswould leave stale trailing entries innewOwners, and duplicate-looking entries from the backend would render as separate cards.addSelected()now skips users already present innewOwners(the previous.concat()happily appended duplicates).The backend fix already prevents the duplicate-owner scenario on writes; the frontend changes are defense-in-depth for pre-existing duplicate state and future regressions.
Test plan
tox -e test -- dandiapi/api/tests/test_dandiset.py -k "owner"in CI/ locally(since requires manual startup/setup...) — the three new tests should pass and the existing owner tests should not regress. (The dev sandbox where this branch was prepared didn't have the docker-compose stack; onlyruff check+ruff format --checkwere run.)Userrow is the "real" one).find_duplicate_ownersmanagement command listing(dandiset_id, username, [user_pks])would help find other dandisets affected before the fix.