diff --git a/playwright/tests/import-managed-collection.spec.ts b/playwright/tests/import-managed-collection.spec.ts new file mode 100644 index 00000000..a338a2c6 --- /dev/null +++ b/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(); + }); +}); diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index 989ca47d..b375fe9d 100644 --- a/src/api/core/organizations.rs +++ b/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 = 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 = 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 { diff --git a/src/db/models/collection.rs b/src/db/models/collection.rs index 8aec90ea..ee235ba6 100644 --- a/src/db/models/collection.rs +++ b/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), }