Merge e5b7f37582 into 0cefa4cca7

playwright/tests/import-managed-collection.spec.ts

@@ -0,0 +1,80 @@
+import { test, expect, type TestInfo } from '@playwright/test';
+
+import * as utils from "../global-utils";
+import * as orgs from './setups/orgs';
+import { createAccount, logUser } from './setups/user';
+
+let users = utils.loadEnv();
+
+test.beforeAll('Setup', async ({ browser }, testInfo: TestInfo) => {
+    await utils.startVault(browser, testInfo);
+});
+
+test.afterAll('Teardown', async ({}) => {
+    utils.stopVault();
+});
+
+test('A member with Manage permission on a collection can import into it under the ownership policy', async ({ page }) => {
+    test.setTimeout(300_000);
+
+    // The member account has to exist before being invited (no SMTP configured for this suite).
+    await createAccount(test, page, users.user2);
+    await createAccount(test, page, users.user1);
+
+    await orgs.create(test, page, 'ImportOrg');
+
+    await test.step('Create a managed and a locked collection', async () => {
+        await page.getByRole('button', { name: 'New', exact: true }).click();
+        await page.getByRole('menuitem', { name: 'Collection' }).click();
+        await page.getByRole('textbox', { name: 'Name * (required)', exact: true }).fill('Managed');
+        await page.getByRole('button', { name: 'Save' }).click();
+        await utils.checkNotification(page, 'Created collection Managed');
+
+        await page.getByRole('button', { name: 'New', exact: true }).click();
+        await page.getByRole('menuitem', { name: 'Collection' }).click();
+        await page.getByRole('textbox', { name: 'Name * (required)', exact: true }).fill('Locked');
+        await page.getByRole('button', { name: 'Save' }).click();
+        await utils.checkNotification(page, 'Created collection Locked');
+    });
+
+    await test.step('Enable the organisation ownership policy', async () => {
+        await orgs.policies(test, page, 'ImportOrg');
+        await page.getByRole('button', { name: /^Centralise organisation ownership/ }).click();
+        await page.getByRole('checkbox', { name: 'Turn on' }).check();
+        await page.getByRole('button', { name: 'Save' }).click();
+    });
+
+    await orgs.members(test, page, 'ImportOrg');
+    await test.step(`Invite ${users.user2.email} with Manage collection on Managed only`, async () => {
+        await page.getByRole('button', { name: 'Invite member' }).click();
+        await page.getByRole('textbox', { name: 'Email * (required)', exact: true }).fill(users.user2.email);
+        await page.getByRole('tab', { name: 'Collections' }).click();
+        await page.getByRole('combobox', { name: 'Permission' }).click();
+        await page.getByRole('option', { name: 'Manage collection', exact: true }).click();
+        await page.getByRole('combobox', { name: 'Select collections' }).click();
+        await page.getByLabel('Options List').getByText('Managed', { exact: true }).click();
+        await page.getByRole('columnheader', { name: 'Collection', exact: true }).click();
+        await page.getByRole('button', { name: 'Save' }).click();
+        await utils.checkNotification(page, 'User(s) invited');
+    });
+
+    await orgs.confirm(test, page, 'ImportOrg', users.user2.email);
+
+    await logUser(test, page, users.user2);
+
+    await test.step('The import destination and file fields are enabled for the managed collection', async () => {
+        await page.goto('/#/tools/import');
+
+        const vaultSelect = page.getByRole('combobox', { name: /^Vault/ });
+        await expect(vaultSelect).toBeEnabled();
+        // The member has exactly one org they can manage a collection in, so it's preselected
+        // instead of "My vault".
+        await expect(page.getByText('ImportOrg', { exact: true })).toBeVisible();
+
+        const collectionSelect = page.getByRole('combobox', { name: 'Collection' });
+        await expect(collectionSelect).toBeEnabled();
+
+        await expect(page.getByRole('combobox', { name: /^File format/ })).toBeEnabled();
+        await expect(page.getByRole('textbox', { name: /or copy\/paste the import file contents/ })).toBeEnabled();
+    });
+});

src/api/core/organizations.rs

@@ -1837,9 +1837,12 @@ async fn post_org_import(
     for col in data.collections {
         let existing = col.id.as_ref().and_then(|col_id| existing_collections.get(col_id));
         let collection_uuid = if let Some(collection) = existing {
-            // When not an Owner or Admin, check if the member is allowed to write to the collection.
+            // When not an Owner or Admin, the member must have the "Manage collection" permission
+            // (directly or via a group) on the collection. Plain edit/write access is not enough:
+            // importing creates items in bulk on the member's behalf, so it requires the same
+            // permission level as manually managing the collection's contents.
             if headers.membership.atype < MembershipType::Admin
-                && !collection.is_writable_by_user(&headers.membership.user_uuid, &conn).await
+                && !collection.is_manageable_by_user(&headers.membership.user_uuid, &conn).await
             {
                 err!(Compact, "The current user isn't allowed to manage this collection")
             }
@@ -1865,6 +1868,17 @@ async fn post_org_import(
         relations.push((relation.key, relation.value));
     }
 
+    // Members without full organization access (e.g. importing into collections they manage
+    // under an active "Organization Data Ownership" policy) can only ever act within collections
+    // they're allowed to manage. Reject the whole import up-front if any cipher wouldn't end up
+    // assigned to at least one such collection, so we never create orphaned organization items.
+    if headers.membership.atype < MembershipType::Admin
+        && !headers.membership.has_full_access()
+        && !every_cipher_assigned_to_a_collection(data.ciphers.len(), &relations, !collections.is_empty())
+    {
+        err!(Compact, "Every imported item must be assigned to a collection you're allowed to manage")
+    }
+
     let headers: Headers = headers.into();
 
     let mut ciphers: Vec<CipherId> = Vec::with_capacity(data.ciphers.len());
@@ -1900,6 +1914,60 @@ async fn post_org_import(
     user.update_revision(&conn).await
 }
 
+/// Returns whether every cipher in `0..cipher_count` appears as the cipher-index side of at
+/// least one entry in `relations`. Used to reject an organization import up-front, before any
+/// ciphers are created, when the importing member doesn't have full org access and therefore
+/// must place every item into a collection they're allowed to manage.
+fn every_cipher_assigned_to_a_collection(
+    cipher_count: usize,
+    relations: &[(usize, usize)],
+    has_any_collection: bool,
+) -> bool {
+    if cipher_count == 0 {
+        return true;
+    }
+    if !has_any_collection {
+        return false;
+    }
+    let assigned: HashSet<usize> = relations.iter().map(|(cipher_idx, _)| *cipher_idx).collect();
+    (0..cipher_count).all(|i| assigned.contains(&i))
+}
+
+#[cfg(test)]
+mod tests {
+    use super::every_cipher_assigned_to_a_collection;
+
+    #[test]
+    fn no_ciphers_is_always_allowed() {
+        assert!(every_cipher_assigned_to_a_collection(0, &[], false));
+    }
+
+    #[test]
+    fn ciphers_without_any_collection_are_rejected() {
+        assert!(!every_cipher_assigned_to_a_collection(2, &[], false));
+    }
+
+    #[test]
+    fn every_cipher_mapped_to_a_collection_is_allowed() {
+        let relations = [(0, 0), (1, 0)];
+        assert!(every_cipher_assigned_to_a_collection(2, &relations, true));
+    }
+
+    #[test]
+    fn a_cipher_missing_from_relations_is_rejected() {
+        // Cipher index 1 has no entry in `relations`.
+        let relations = [(0, 0)];
+        assert!(!every_cipher_assigned_to_a_collection(2, &relations, true));
+    }
+
+    #[test]
+    fn a_cipher_assigned_to_multiple_collections_still_counts_as_assigned() {
+        // Cipher 0 is mapped to two collections, cipher 1 to one: both are covered.
+        let relations = [(0, 0), (0, 1), (1, 0)];
+        assert!(every_cipher_assigned_to_a_collection(2, &relations, true));
+    }
+}
+
 #[derive(Deserialize)]
 #[serde(rename_all = "camelCase")]
 struct BulkCollectionsData {

src/db/models/collection.rs

@@ -108,19 +108,22 @@ impl Collection {
                 // Owners and Admins always have true. Users are not able to have full access
                 Some(m) if m.has_full_access() => (false, false, m.atype >= MembershipType::Manager),
                 Some(m) => {
-                    // Only let a manager manage collections when the have full read/write access
+                    // The explicit per-collection "Manage" permission applies regardless of the
+                    // member's org-level role. Additionally, a Manager with full (non-restricted)
+                    // read/write access to a collection is also allowed to manage it, even without
+                    // that flag explicitly set.
                     let is_manager = m.atype == MembershipType::Manager;
                     if let Some(cu) = cipher_sync_data.user_collections.get(&self.uuid) {
                         (
                             cu.read_only,
                             cu.hide_passwords,
-                            is_manager && (cu.manage || (!cu.read_only && !cu.hide_passwords)),
+                            cu.manage || (is_manager && !cu.read_only && !cu.hide_passwords),
                         )
                     } else if let Some(cg) = cipher_sync_data.user_collections_groups.get(&self.uuid) {
                         (
                             cg.read_only,
                             cg.hide_passwords,
-                            is_manager && (cg.manage || (!cg.read_only && !cg.hide_passwords)),
+                            cg.manage || (is_manager && !cg.read_only && !cg.hide_passwords),
                         )
                     } else {
                         (false, false, false)
@@ -138,7 +141,9 @@ impl Collection {
                     let is_manager = m.atype == MembershipType::Manager;
                     let read_only = !self.is_writable_by_user(user_uuid, conn).await;
                     let hide_passwords = self.hide_passwords_for_user(user_uuid, conn).await;
-                    (read_only, hide_passwords, is_manager && !read_only && !hide_passwords)
+                    let manage = self.is_manageable_by_user(user_uuid, conn).await
+                        || (is_manager && !read_only && !hide_passwords);
+                    (read_only, hide_passwords, manage)
                 }
                 _ => (true, true, false),
             }