fix(mobile): stop offering albums you cannot add to in the collection picker - #985
Merged
Conversation
… picker The picker listed every album the user is a member of, including ones where their role is viewer. `Permission.AlbumAssetCreate` is owner union shared-with-editor union space-linked, and that check runs on the album id before any asset is touched, so a viewer's request is rejected wholesale -- the sheet can only surface a generic "an error occurred". A viewer-role album was therefore a dead target. Adds `albumsUserCanAddTo` beside the existing space_permissions helpers and an opt-in `writableOnly` flag on AlbumSelector, applied at the single source both the searched and unsearched list paths derive from, so a hidden album cannot reappear by typing its name. Both pickers opt in; the album browser does not, since a viewer-role album is perfectly valid to open. Fails open by design: a null role means "not known", not "viewer", and stays listed -- hiding an album we are unsure about would make a legitimate target vanish silently, and the server is the real enforcer either way.
V2 pins that writableOnly is opt-in, which is what keeps the album browser (drift_album.page) showing viewer-role albums -- they are valid to open, only invalid as an add target. Nothing else guarded the default. V3 pins the load-bearing claim that a hidden album cannot reappear by typing its name. It captures the list handed to searchAlbums rather than looking for rendered rows, because AlbumTile needs driftProvider and throws in this harness; feeding only an album that must be hidden keeps the list empty so no tile is ever built. Both V1 and V3 were confirmed red with writableOnly temporarily flipped to false.
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.
Follow-up to #984, from a question asked while testing it: are all the albums/spaces the picker offers actually valid targets?
For spaces, yes. For albums, no.
The gap
The spaces half is properly gated — rows are filtered by
spaceIsWritable, and space-linked albums are covered server-side byAlbumAssetCreate'scheckSpaceLinkedAlbumAccessarm.The album half is not.
AlbumSelectorlists every album the user is a member of, including ones where their role is viewer.RemoteAlbum.currentUserRolealready carries that fact, but the widget only used it to gate swipe-to-delete.Server-side,
Permission.AlbumAssetCreateis owner ∪ shared-with-Editor ∪ space-linked (server/src/utils/access.ts:196-205), andAlbumService.addAssetsruns that check on the album id before touching any asset (album.service.ts:255). So a viewer's add is rejected wholesale rather than per-asset, and the sheet can only report the genericscaffold_body_error_occurred. The album was a dead target that failed with an unhelpful message.This predates #984 — the listing behaviour is upstream's. #984 widened the exposure by removing the
ownsAlbumgate, so a viewer of an album now sees the picker at all. That change was still right: rights over the destination are what matter. It just assumed the destination list was trustworthy.The fix
albumsUserCanAddToin a newutils/album_permissions.dart, beside the existingspace_permissions.dartthat does the same job for spaces.AlbumSelectorgains an opt-inwritableOnlyflag (+1 field, +1 ctor param, +4 lines in one method — additive, defaultfalse, so upstream call sites are untouched).Applied at the input to
sortAlbums(), which is the single source bothshownAlbumspaths derive from. Filtering only the unsearched list would let a hidden album reappear the moment its name is typed.Opt-in per call site, because the distinction is real:
writableOnlycollection_picker.widget.dartpartner_detail_bottom_sheet.widget.dartdrift_album.page.dartFails open deliberately. A null
currentUserRolemeans "not known" (not in the role table, or unsynced) and stays listed. Only a role positively known to bevieweris dropped. Hiding an album we're merely unsure about would make a legitimate target vanish with no explanation — worse than offering one the server declines, and the server enforces either way. Same posture asdriftSpaceEditableProviderfor space people.Space-linked albums are unaffected: they carry no
album_userrow for the caller, so they never enter this list — which is exactly why they need their own section.Tests
The rule is covered exhaustively as a pure function (7 cases: viewer refused; editor, owner and unknown allowed; order preserved; all-viewer and empty lists), plus a widget test that the picker actually passes
writableOnly.The list rows themselves are deliberately not asserted on, and the test says why:
AlbumTilethrowsdriftProvider must be overriddenin that harness, andfind.texton an album name is confounded by the search field's own text — an earlier draft of this test typed the album's name to search for it and "passed" by matching the text box. So the widget test asserts the wiring, mirroring how the bottom-sheet tests assert onBaseBottomSheet.sliversrather than on rows below the fold.Verification
mobile 3139 tests pass,
dart analyze --fatal-infosclean,dart formatoverlib/clean (0 of 808 changed).Not fixed here
The generic error message itself. Mapping the 400 to something like "You can't add to this album" would help the cases this filter can't prevent (a role that changed since sync). Worth doing, but it's a separate change.