Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion mobile/lib/presentation/widgets/album/album_selector.widget.dart
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import 'package:immich_mobile/models/albums/album_search.model.dart';
import 'package:immich_mobile/presentation/widgets/album/album_tile.dart';
import 'package:immich_mobile/presentation/widgets/album/new_album_name_modal.widget.dart';
import 'package:immich_mobile/presentation/widgets/images/thumbnail.widget.dart';
import 'package:immich_mobile/utils/album_permissions.dart';
import 'package:immich_mobile/providers/album/album_sort_by_options.provider.dart';
import 'package:immich_mobile/providers/asset_viewer/asset_viewer.provider.dart';
import 'package:immich_mobile/providers/infrastructure/album.provider.dart';
Expand Down Expand Up @@ -45,13 +46,21 @@ class AlbumSelector extends ConsumerStatefulWidget {
/// picker can put another section under the shared search box. Null for every upstream call site.
final Widget? sliverAfterSearch;

/// Fork hook: hide albums the user cannot add assets to (see `album_permissions.dart`).
///
/// True for the add-to-collection *pickers*, where a viewer-role album is a dead target the
/// server rejects outright. False for album *browsers*, where such an album is perfectly
/// valid to open — which is why this is opt-in rather than always on.
final bool writableOnly;

const AlbumSelector({
super.key,
required this.onAlbumSelected,
this.onKeyboardExpanded,
this.onSearchChanged,
this.searchHint,
this.sliverAfterSearch,
this.writableOnly = false,
});

@override
Expand Down Expand Up @@ -144,9 +153,15 @@ class _AlbumSelectorState extends ConsumerState<AlbumSelector> {
}

Future<void> sortAlbums() async {
// Fork: a picker must not offer albums the user cannot add to. Filter here, at the single
// source both the searched and unsearched `shownAlbums` paths derive from, so a hidden
// album cannot reappear by typing its name.
final albums = ref.read(remoteAlbumProvider).albums;
final candidates = widget.writableOnly ? albumsUserCanAddTo(albums) : albums;

final sorted = await ref
.read(remoteAlbumProvider.notifier)
.sortAlbums(ref.read(remoteAlbumProvider).albums, sort.mode, isReverse: sort.isReverse);
.sortAlbums(candidates, sort.mode, isReverse: sort.isReverse);

if (!mounted) {
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ class _PartnerDetailBottomSheetState extends ConsumerState<PartnerDetailBottomSh
],
slivers: [
const AddToAlbumHeader(),
AlbumSelector(onAlbumSelected: addToAlbum, onKeyboardExpanded: onKeyboardExpand),
AlbumSelector(onAlbumSelected: addToAlbum, onKeyboardExpanded: onKeyboardExpand, writableOnly: true),
],
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,9 @@ class _CollectionPickerState extends ConsumerState<CollectionPicker> {
onKeyboardExpanded: widget.onKeyboardExpanded,
onSearchChanged: (query) => setState(() => _searchQuery = query),
searchHint: 'search_albums_and_spaces'.t(context: context),
// A viewer-role album is a dead target: the server rejects the whole request on the
// album id, so the sheet could only report a generic error.
writableOnly: true,
sliverAfterSearch: SliverToBoxAdapter(
child: SpaceCollectionSection(
onTargetSelected: _addToTarget,
Expand Down
31 changes: 31 additions & 0 deletions mobile/lib/utils/album_permissions.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import 'package:immich_mobile/domain/models/album/album.model.dart';

/// Role predicates for albums, shared by every surface that gates on them.
///
/// Companion to `space_permissions.dart`, which does the same job for shared spaces.

/// Whether the current user can add assets to [album].
///
/// Mirrors the server's `Permission.AlbumAssetCreate`, which grants
/// owner ∪ shared-with-[AlbumUserRole.editor] ∪ space-linked (`server/src/utils/access.ts`).
/// That check runs on the album id *before* any asset is touched, so a viewer's request is
/// rejected wholesale rather than per-asset — the client cannot report anything more useful
/// than a generic error. A viewer-role album offered as a target is therefore a dead end.
///
/// **Deliberately fails open.** A null [RemoteAlbum.currentUserRole] means "not known" — the
/// album is not in the role table, or the row has not synced — and is treated as usable. Only
/// a role we positively know to be [AlbumUserRole.viewer] is refused. Hiding an album we are
/// merely unsure about would make a legitimate target vanish with no explanation, which is a
/// worse failure than offering one the server then declines; the server is the real enforcer
/// either way. Same posture as `driftSpaceEditableProvider` for space people.
///
/// Space-linked albums are not a concern here: they carry no `album_user` row for the caller,
/// so they never appear in the personal album list to begin with — which is exactly why they
/// need their own section in the picker.
bool canAddAssetsToAlbum(RemoteAlbum album) => album.currentUserRole != AlbumUserRole.viewer;

/// [albums] with the ones the current user cannot add to removed.
///
/// Used by the add-to-collection pickers. Album *browsers* must not use this — a viewer-role
/// album is perfectly valid to open and look at; it is only invalid as an add target.
List<RemoteAlbum> albumsUserCanAddTo(List<RemoteAlbum> albums) => albums.where(canAddAssetsToAlbum).toList();
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import 'package:immich_mobile/presentation/widgets/collection/space_collection_s
import 'package:immich_mobile/presentation/widgets/album/album_selector.widget.dart';
import 'package:immich_mobile/providers/infrastructure/action.provider.dart';
import 'package:immich_mobile/providers/infrastructure/album.provider.dart';
import 'package:immich_mobile/providers/album/album_sort_by_options.provider.dart';
import 'package:immich_mobile/providers/infrastructure/remote_album.provider.dart';
import 'package:immich_mobile/providers/infrastructure/settings.provider.dart';
import 'package:immich_mobile/providers/infrastructure/space_album.provider.dart';
Expand All @@ -22,8 +23,9 @@ import 'package:immich_mobile/providers/timeline/multiselect.provider.dart';
import 'package:immich_mobile/providers/user.provider.dart';
import 'package:immich_mobile/widgets/common/search_field.dart';
import 'package:mocktail/mocktail.dart';
import 'package:openapi/api.dart';
import 'package:openapi/api.dart' hide AlbumUserRole;

import '../../../unit/factories/remote_album_factory.dart';
import '../../../fixtures/user.stub.dart';
import '../../../widget_tester_extensions.dart';

Expand Down Expand Up @@ -60,6 +62,46 @@ class _StubRemoteAlbumNotifier extends RemoteAlbumNotifier {
]) => albums;
}

/// Records the album list `AlbumSelector` passes into `searchAlbums`.
///
/// This is the only way to observe the picker's album filtering from a test: `AlbumTile`
/// needs `driftProvider` overridden and throws, so no row ever renders. Feeding the picker
/// only albums it must hide keeps `shownAlbums` empty, so nothing tries to build a tile.
class _CapturingRemoteAlbumNotifier extends RemoteAlbumNotifier {
_CapturingRemoteAlbumNotifier(this._albums);

final List<RemoteAlbum> _albums;
List<RemoteAlbum>? searchedOver;

// Starts empty: the widget only sorts from a `ref.listen` on `state.albums`, which fires on
// CHANGE, so publishing from `refresh()` is what actually drives the pipeline.
@override
RemoteAlbumState build() => const RemoteAlbumState(albums: []);

@override
Future<void> refresh() async {
state = RemoteAlbumState(albums: _albums);
}

@override
Future<List<RemoteAlbum>> sortAlbums(
List<RemoteAlbum> albums,
AlbumSortMode sortMode, {
bool isReverse = false,
}) async => albums;

@override
List<RemoteAlbum> searchAlbums(
List<RemoteAlbum> albums,
String query,
String? userId, [
QuickFilterMode filterMode = QuickFilterMode.all,
]) {
searchedOver = albums;
return albums;
}
}

/// Captures which [ActionSource] the picker dispatched against, and lets a test make the
/// dispatch fail, without standing up the real action plumbing.
class _RecordingActionNotifier extends ActionNotifier {
Expand Down Expand Up @@ -92,6 +134,8 @@ class _RecordingActionNotifier extends ActionNotifier {
}
}

void _noop(RemoteAlbum _) {}

void main() {
Future<void> pumpPicker(
WidgetTester tester, {
Expand Down Expand Up @@ -192,6 +236,63 @@ void main() {
expect(headerY, lessThan(albumsY));
});

// The album rows themselves cannot be asserted on here: `AlbumTile` needs `driftProvider`
// overridden and throws in this harness, and `find.text` on an album name is confounded by
// the search field's own text. So assert the WIRING -- that the picker asks `AlbumSelector`
// to hide albums the user cannot add to -- the same way the bottom-sheet tests assert on
// `BaseBottomSheet.slivers` rather than on rows below the fold. The rule itself is covered
// exhaustively in test/utils/album_permissions_test.dart.
testWidgets('V1: the picker asks the album selector to hide albums the user cannot add to', (tester) async {
await pumpPicker(tester);

expect(tester.widget<AlbumSelector>(find.byType(AlbumSelector)).writableOnly, isTrue);
});

testWidgets('V2: writableOnly is opt-in, so album browsers are unaffected', (tester) async {
// drift_album.page.dart mounts AlbumSelector as a browser, where a viewer-role album is
// perfectly valid to open. It passes nothing, so the default is what protects it.
const browser = AlbumSelector(onAlbumSelected: _noop);

expect(browser.writableOnly, isFalse);
});

testWidgets('V3: a hidden album is already gone before the search runs', (tester) async {
// The filter sits at sortAlbums()'s input rather than on the rendered list, so a hidden
// album cannot come back by typing its name. Observed by capturing what the widget hands
// to searchAlbums -- rows themselves cannot render (AlbumTile needs driftProvider).
final viewerAlbum = RemoteAlbumFactory.create(
id: 'al1',
name: 'ViewerAlbum',
currentUserRole: AlbumUserRole.viewer,
);
final notifier = _CapturingRemoteAlbumNotifier([viewerAlbum]);

final userService = _MockUserService();
final user = UserStub.user1;
when(() => userService.tryGetMyUser()).thenReturn(user);
when(() => userService.watchMyUser()).thenAnswer((_) => const Stream.empty());

await tester.pumpConsumerWidgetRaw(
const CustomScrollView(slivers: [CollectionPicker()]),
overrides: [
currentUserProvider.overrideWith((ref) => _StubCurrentUserNotifier(userService, user)),
remoteAlbumProvider.overrideWith(() => notifier),
appConfigProvider.overrideWithValue(const AppConfig()),
sharedSpacesProvider.overrideWith((ref) async => const []),
multiSelectProvider.overrideWith(
() => MultiSelectNotifier(const MultiSelectState(selectedAssets: {}, lockedSelectionAssets: {})),
),
],
);
await tester.pumpAndSettle();

await tester.enterText(find.byType(SearchField), 'ViewerAlbum');
await tester.pumpAndSettle();

expect(notifier.searchedOver, isNotNull, reason: 'the search path never ran, so this proves nothing');
expect(notifier.searchedOver, isEmpty);
});

testWidgets('L1: spaces render above albums, and both below the search field', (tester) async {
await pumpPicker(tester, spaces: [space('s1', 'Family')]);

Expand Down
2 changes: 2 additions & 0 deletions mobile/test/unit/factories/remote_album_factory.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@ class RemoteAlbumFactory {
int assetCount = 0,
String? ownerName,
bool isShared = false,
AlbumUserRole? currentUserRole,
}) {
id = TestUtils.uuid(id);
return RemoteAlbum(
currentUserRole: currentUserRole,
id: id,
name: name ?? 'remote_album_$id',
ownerId: TestUtils.uuid(ownerId),
Expand Down
55 changes: 55 additions & 0 deletions mobile/test/utils/album_permissions_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import 'package:flutter_test/flutter_test.dart';
import 'package:immich_mobile/domain/models/album/album.model.dart';
import 'package:immich_mobile/utils/album_permissions.dart';

import '../unit/factories/remote_album_factory.dart';

void main() {
RemoteAlbum album(String name, AlbumUserRole? role) =>
RemoteAlbumFactory.create(id: name, name: name, currentUserRole: role);

group('canAddAssetsToAlbum', () {
test('refuses a viewer-role album', () {
// The server's AlbumAssetCreate is owner ∪ editor ∪ space-linked, so a viewer's add
// is rejected on the album id before any asset is touched.
expect(canAddAssetsToAlbum(album('Viewer', AlbumUserRole.viewer)), isFalse);
});

test('allows an editor-role album', () {
expect(canAddAssetsToAlbum(album('Editor', AlbumUserRole.editor)), isTrue);
});

test('allows an owner-role album', () {
expect(canAddAssetsToAlbum(album('Owner', AlbumUserRole.owner)), isTrue);
});

test('allows an album whose role is unknown, rather than hiding it', () {
// Fails open on purpose: null means "not known", not "viewer". Hiding an album we are
// unsure about would make a legitimate target vanish silently.
expect(canAddAssetsToAlbum(album('Unknown', null)), isTrue);
});
});

group('albumsUserCanAddTo', () {
test('drops only the viewer-role albums and preserves order', () {
final albums = [
album('Owned', AlbumUserRole.owner),
album('Viewer', AlbumUserRole.viewer),
album('Editor', AlbumUserRole.editor),
album('Unknown', null),
];

expect(albumsUserCanAddTo(albums).map((a) => a.name), ['Owned', 'Editor', 'Unknown']);
});

test('returns an empty list when every album is viewer-role', () {
final albums = [album('V1', AlbumUserRole.viewer), album('V2', AlbumUserRole.viewer)];

expect(albumsUserCanAddTo(albums), isEmpty);
});

test('returns an empty list unchanged', () {
expect(albumsUserCanAddTo(const []), isEmpty);
});
});
}
Loading