From a5db6cea6fe8c4c720b6793847b52601d97b0ba6 Mon Sep 17 00:00:00 2001 From: sshrushanth-ks Date: Tue, 16 Jun 2026 14:26:26 +0530 Subject: [PATCH 1/9] Fix share-folder record expiration removing owner records and breaking re-shares When share-folder was used with -r and --expire-in, expiration was applied to SharedFolderUpdateRecord, which caused the record to be removed from the owner's vault when the timer expired. Route per-record expiration and -roe through the record share API (revoke then re-grant) instead, keep folder user updates for access only, sync before granting, and skip redundant user updates when sharing additional records to the same recipient. --- keepercommander/commands/register.py | 146 ++++++++++++++++++++++++++- unit-tests/test_command_register.py | 131 +++++++++++++++++++++++- 2 files changed, 271 insertions(+), 6 deletions(-) diff --git a/keepercommander/commands/register.py b/keepercommander/commands/register.py index 5aac059db..c51e72055 100644 --- a/keepercommander/commands/register.py +++ b/keepercommander/commands/register.py @@ -321,6 +321,8 @@ def get_share_admin_obj_uids(obj_names, obj_type): if action == 'grant': share_expiration = get_share_expiration( kwargs.get('expire_at'), kwargs.get('expire_in'), cmd_name='share-folder') + if isinstance(share_expiration, int) and (kwargs.get('user') or kwargs.get('record')): + SyncDownCommand().execute(params, force=True) rotate_on_expiration = bool(kwargs.get('rotate_on_expiration')) if rotate_on_expiration: @@ -451,6 +453,128 @@ def prep_rq(recs, users, curr_sf): group_idx += 1 self.send_requests(params, rq_groups) + if (action == 'grant' and isinstance(share_expiration, int) and record_uids and as_users): + api.load_user_public_keys(params, list(as_users), send_invites=False) + SyncDownCommand().execute(params, force=True) + record_share_requests = [] + for sf_uid in shared_folder_uids: + sh_fol = params.shared_folder_cache.get(sf_uid, {'shared_folder_uid': sf_uid}) + rs_rqs = ShareFolderCommand.prepare_record_share_request( + params, kwargs, sf_uid, list(as_users), list(record_uids), + curr_sf=sh_fol, share_expiration=share_expiration, + rotate_on_expiration=rotate_on_expiration) + if rs_rqs: + record_share_requests.extend(rs_rqs) + if record_share_requests: + ShareRecordCommand.send_requests(params, record_share_requests) + + @staticmethod + def prepare_record_share_request(params, kwargs, shared_folder_uid, users, rec_uids, *, + curr_sf, share_expiration=None, rotate_on_expiration=False): + """Build RecordShareUpdateRequest(s) for per-record share expiration in a shared folder. + + Expiration must not be set on SharedFolderUpdateRecord — that removes the record + from the shared folder (and the owner's vault) when it expires. Per-user record + expiration is applied through the record share API instead. + + When setting a positive expiration, the server only inserts expiration rows on + create, so we revoke the existing share and re-grant in separate requests. + """ + if not rec_uids or not users or not isinstance(share_expiration, int): + return None + if (kwargs.get('action') or 'grant') != 'grant': + return None + + ce = kwargs.get('can_edit') + cs = kwargs.get('can_share') + sf_uid_bytes = utils.base64_url_decode(shared_folder_uid) + regrant_for_expiration = share_expiration > 0 + + def apply_share_expiration(ro): # type: (record_pb2.SharedRecord) -> None + if share_expiration > 0: + ro.expiration = share_expiration * 1000 + ro.timerNotificationType = record_pb2.NOTIFY_OWNER + if rotate_on_expiration: + ro.rotateOnExpiration = True + elif share_expiration < 0: + ro.expiration = -1 + + def build_shared_record(email, record_uid, rec, existing=None): + # type: (str, str, dict, Optional[dict]) -> Optional[record_pb2.SharedRecord] + ro = record_pb2.SharedRecord() + ro.toUsername = email + ro.recordUid = utils.base64_url_decode(record_uid) + ro.sharedFolderUid = sf_uid_bytes + record_key = rec.get('record_key_unencrypted') + keys = params.key_cache.get(email) + if not record_key or not keys or not (keys.rsa or keys.ec): + return None + if params.forbid_rsa and keys.ec: + ec_key = crypto.load_ec_public_key(keys.ec) + ro.recordKey = crypto.encrypt_ec(record_key, ec_key) + ro.useEccKey = True + elif not params.forbid_rsa and keys.rsa: + rsa_key = crypto.load_rsa_public_key(keys.rsa) + ro.recordKey = crypto.encrypt_rsa(record_key, rsa_key) + ro.useEccKey = False + if existing: + ro.editable = ce == 'on' if ce is not None else existing.get('editable', False) + ro.shareable = cs == 'on' if cs is not None else existing.get('shareable', False) + else: + default_ce = curr_sf.get('default_can_edit') is True + default_cs = curr_sf.get('default_can_share') is True + ro.editable = ce == 'on' if ce is not None else default_ce + ro.shareable = cs == 'on' if cs is not None else default_cs + apply_share_expiration(ro) + return ro + + rq_remove = record_pb2.RecordShareUpdateRequest() + rq_add = record_pb2.RecordShareUpdateRequest() + rq_update = record_pb2.RecordShareUpdateRequest() + for record_uid in rec_uids: + if record_uid not in params.record_cache: + continue + rec = params.record_cache[record_uid] + existing_shares = {} + shares = rec.get('shares') or {} + for po in shares.get('user_permissions') or []: + existing_shares[po['username'].lower()] = po + + for email in users: + email_key = email.lower() + existing = existing_shares.get(email_key) + if regrant_for_expiration: + ro_remove = record_pb2.SharedRecord() + ro_remove.toUsername = email + ro_remove.recordUid = utils.base64_url_decode(record_uid) + ro_remove.sharedFolderUid = sf_uid_bytes + rq_remove.removeSharedRecord.append(ro_remove) + ro_add = build_shared_record(email, record_uid, rec, existing) + if ro_add: + rq_add.addSharedRecord.append(ro_add) + elif existing: + ro = record_pb2.SharedRecord() + ro.toUsername = email + ro.recordUid = utils.base64_url_decode(record_uid) + ro.sharedFolderUid = sf_uid_bytes + ro.editable = ce == 'on' if ce is not None else existing.get('editable', False) + ro.shareable = cs == 'on' if cs is not None else existing.get('shareable', False) + apply_share_expiration(ro) + rq_update.updateSharedRecord.append(ro) + else: + ro_add = build_shared_record(email, record_uid, rec) + if ro_add: + rq_add.addSharedRecord.append(ro_add) + + result = [] + if rq_remove.removeSharedRecord: + result.append(rq_remove) + if rq_add.addSharedRecord: + result.append(rq_add) + if rq_update.updateSharedRecord: + result.append(rq_update) + return result or None + @staticmethod def prepare_request(params, kwargs, curr_sf, users, teams, rec_uids, *, default_record=False, default_account=False, @@ -464,9 +588,17 @@ def prepare_request(params, kwargs, curr_sf, users, teams, rec_uids, *, action = kwargs.get('action') or 'grant' mr = kwargs.get('manage_records') mu = kwargs.get('manage_users') + per_record_timer = bool(rec_uids) and isinstance(share_expiration, int) def apply_share_expiration(target): - """Set expiration / timer / rotateOnExpiration on a User/Team/Record update proto.""" + """Set expiration / timer / rotateOnExpiration on a User/Team share update proto. + + When -r and --expire-in are used together, the timer belongs on the per-user + record share (records_share_update), not the folder user share. The server + enforces a longer minimum (e.g. 5 minutes) on folder-level ROE timers. + """ + if per_record_timer: + return if not isinstance(share_expiration, int): return if share_expiration > 0: @@ -495,6 +627,17 @@ def apply_share_expiration(target): apply_share_expiration(uo) if email in existing_users: if action == 'grant': + if rec_uids: + current_user = next( + (x for x in curr_sf.get('users', []) if x['username'] == email), None) + if current_user: + mr_unchanged = (mr is None + or (current_user.get('manage_records') is True) == (mr == 'on')) + mu_unchanged = (mu is None + or (current_user.get('manage_users') is True) == (mu == 'on')) + if mr_unchanged and mu_unchanged and ( + per_record_timer or not isinstance(share_expiration, int)): + continue uo.manageRecords = folder_pb2.BOOLEAN_NO_CHANGE if mr is None else folder_pb2.BOOLEAN_TRUE if mr == 'on' else folder_pb2.BOOLEAN_FALSE uo.manageUsers = folder_pb2.BOOLEAN_NO_CHANGE if mu is None else folder_pb2.BOOLEAN_TRUE if mu == 'on' else folder_pb2.BOOLEAN_FALSE rq.sharedFolderUpdateUser.append(uo) @@ -584,7 +727,6 @@ def apply_share_expiration(target): for record_uid in rec_uids: ro = folder_pb2.SharedFolderUpdateRecord() ro.recordUid = utils.base64_url_decode(record_uid) - apply_share_expiration(ro) if record_uid in existing_records: if action == 'grant': diff --git a/unit-tests/test_command_register.py b/unit-tests/test_command_register.py index 3074af5a7..803cc86a4 100644 --- a/unit-tests/test_command_register.py +++ b/unit-tests/test_command_register.py @@ -309,10 +309,9 @@ def test_share_folder(self): self.assertEqual(len(TestRegister.expected_commands), 0) def test_share_folder_prepare_request_sets_rotate_on_expiration(self): - """SharedFolderUpdateUser/Team/Record all carry rotateOnExpiration when -roe is on.""" + """Folder-wide expiration/ROE applies to user/team protos, not record protos.""" params = get_synced_params() shared_folder_uid = next(iter(params.shared_folder_cache.keys())) - record_uid = next(iter([x['record_uid'] for x in params.meta_data_cache.values() if x['can_share']])) team_uid = utils.base64_url_encode(b'a' * 16) curr_sf = dict(params.shared_folder_cache[shared_folder_uid]) @@ -331,7 +330,7 @@ def test_share_folder_prepare_request_sets_rotate_on_expiration(self): curr_sf=curr_sf, users=['user2@keepersecurity.com'], teams=[team_uid], - rec_uids=[record_uid], + rec_uids=[], share_expiration=future_ts, rotate_on_expiration=True, ) @@ -340,7 +339,7 @@ def test_share_folder_prepare_request_sets_rotate_on_expiration(self): team_msgs = list(rq.sharedFolderAddTeam) + list(rq.sharedFolderUpdateTeam) record_msgs = list(rq.sharedFolderAddRecord) + list(rq.sharedFolderUpdateRecord) - for msgs, label in [(user_msgs, 'user'), (team_msgs, 'team'), (record_msgs, 'record')]: + for msgs, label in [(user_msgs, 'user'), (team_msgs, 'team')]: self.assertTrue(msgs, f'expected at least one {label} proto on the wire') for m in msgs: self.assertTrue(m.rotateOnExpiration, @@ -348,6 +347,130 @@ def test_share_folder_prepare_request_sets_rotate_on_expiration(self): self.assertGreater(m.expiration, 0) self.assertEqual(m.timerNotificationType, record_pb2.NOTIFY_OWNER) + self.assertFalse(record_msgs, 'record protos must not carry folder-wide expiration') + + def test_share_folder_prepare_request_skips_record_expiration_when_records_specified(self): + """Per-record expiration is routed through the record share API, not SharedFolderUpdateRecord.""" + params = get_synced_params() + shared_folder_uid = next(iter(params.shared_folder_cache.keys())) + record_uid = next(iter([x['record_uid'] for x in params.meta_data_cache.values() if x['can_share']])) + + curr_sf = dict(params.shared_folder_cache[shared_folder_uid]) + curr_sf.setdefault('users', []) + curr_sf.setdefault('records', [{'record_uid': record_uid, 'can_edit': True, 'can_share': True}]) + future_ts = int(datetime.datetime.now().timestamp()) + 86_400 + + rq = register.ShareFolderCommand.prepare_request( + params, + kwargs={'action': 'grant', 'can_edit': 'on', 'can_share': 'on'}, + curr_sf=curr_sf, + users=['user2@keepersecurity.com'], + teams=[], + rec_uids=[record_uid], + share_expiration=future_ts, + rotate_on_expiration=True, + ) + + user_msgs = list(rq.sharedFolderAddUser) + list(rq.sharedFolderUpdateUser) + record_msgs = list(rq.sharedFolderAddRecord) + list(rq.sharedFolderUpdateRecord) + + for m in user_msgs: + self.assertEqual(m.expiration, 0) + self.assertFalse(m.rotateOnExpiration) + + self.assertTrue(record_msgs, 'expected record permission update') + for m in record_msgs: + self.assertEqual(m.expiration, 0) + self.assertFalse(m.rotateOnExpiration) + + def test_share_folder_prepare_record_share_request_sets_expiration(self): + params = get_synced_params() + shared_folder_uid = next(iter(params.shared_folder_cache.keys())) + record_uid = next(iter([x['record_uid'] for x in params.meta_data_cache.values() if x['can_share']])) + curr_sf = dict(params.shared_folder_cache[shared_folder_uid]) + future_ts = int(datetime.datetime.now().timestamp()) + 86_400 + + params.key_cache['user2@keepersecurity.com'] = mock.MagicMock( + rsa=utils.base64_url_decode(vault_env.encoded_public_key), ec=None) + + rq = register.ShareFolderCommand.prepare_record_share_request( + params, + kwargs={'action': 'grant', 'can_edit': 'on', 'can_share': 'on'}, + shared_folder_uid=shared_folder_uid, + users=['user2@keepersecurity.com'], + rec_uids=[record_uid], + curr_sf=curr_sf, + share_expiration=future_ts, + rotate_on_expiration=True, + ) + + self.assertIsNotNone(rq) + self.assertEqual(len(rq), 2, 'positive expiration must revoke then re-grant') + self.assertTrue(rq[0].removeSharedRecord) + self.assertTrue(rq[1].addSharedRecord) + shared_records = list(rq[1].addSharedRecord) + self.assertTrue(shared_records) + for sr in shared_records: + self.assertGreater(sr.expiration, 0) + self.assertTrue(sr.rotateOnExpiration) + self.assertEqual(sr.timerNotificationType, record_pb2.NOTIFY_OWNER) + self.assertEqual(utils.base64_url_encode(sr.sharedFolderUid), shared_folder_uid) + + def test_share_folder_prepare_request_skips_redundant_user_update_for_record_only(self): + """When sharing another record without expiration, skip redundant folder user update.""" + params = get_synced_params() + shared_folder_uid = next(iter(params.shared_folder_cache.keys())) + record_uid = next(iter([x['record_uid'] for x in params.meta_data_cache.values() if x['can_share']])) + + curr_sf = dict(params.shared_folder_cache[shared_folder_uid]) + curr_sf['users'] = [{ + 'username': 'user2@keepersecurity.com', + 'manage_records': True, + 'manage_users': True, + }] + curr_sf.setdefault('records', [{'record_uid': record_uid, 'can_edit': True, 'can_share': True}]) + + rq = register.ShareFolderCommand.prepare_request( + params, + kwargs={'action': 'grant', 'manage_records': 'on', 'manage_users': 'on', + 'can_edit': 'on', 'can_share': 'on'}, + curr_sf=curr_sf, + users=['user2@keepersecurity.com'], + teams=[], + rec_uids=[record_uid], + share_expiration=None, + ) + + self.assertFalse(list(rq.sharedFolderUpdateUser)) + self.assertTrue(list(rq.sharedFolderUpdateRecord)) + + def test_share_folder_prepare_request_updates_user_when_folder_wide_expiration(self): + """Folder-wide --expire-in (no -r) sets expiration on the folder user share.""" + params = get_synced_params() + shared_folder_uid = next(iter(params.shared_folder_cache.keys())) + future_ts = int(datetime.datetime.now().timestamp()) + 86_400 + + curr_sf = dict(params.shared_folder_cache[shared_folder_uid]) + curr_sf['users'] = [{ + 'username': 'user2@keepersecurity.com', + 'manage_records': True, + 'manage_users': True, + }] + + rq = register.ShareFolderCommand.prepare_request( + params, + kwargs={'action': 'grant', 'manage_records': 'on', 'manage_users': 'on'}, + curr_sf=curr_sf, + users=['user2@keepersecurity.com'], + teams=[], + rec_uids=[], + share_expiration=future_ts, + ) + + user_msgs = list(rq.sharedFolderUpdateUser) + self.assertTrue(user_msgs) + self.assertGreater(user_msgs[0].expiration, 0) + def test_share_folder_rotate_on_expiration_rejects_folder_without_pam_user(self): params = get_synced_params() shared_folder_uid = next(iter(params.shared_folder_cache.keys())) From 65487880d7eb8e68220eb3962106e1e46eef12a6 Mon Sep 17 00:00:00 2001 From: sshrushanth-ks Date: Wed, 24 Jun 2026 15:57:38 +0530 Subject: [PATCH 2/9] Share-folder: expire folder and record access together; log expiry in output --- keepercommander/commands/register.py | 70 ++++++++++++++++++++-------- unit-tests/test_command_register.py | 13 ++++-- 2 files changed, 59 insertions(+), 24 deletions(-) diff --git a/keepercommander/commands/register.py b/keepercommander/commands/register.py index c51e72055..dd003307f 100644 --- a/keepercommander/commands/register.py +++ b/keepercommander/commands/register.py @@ -20,7 +20,7 @@ import re import time import urllib.parse -from typing import Optional, Dict, Iterable, Any, Set, List, Union +from typing import Optional, Dict, Iterable, Any, Set, List, Union, Tuple from urllib.parse import urlunparse from tabulate import tabulate @@ -256,6 +256,35 @@ def get_share_expiration(expire_at, expire_in, cmd_name='share-record'): # ( return expiration_seconds +def format_share_expiration_ms(expiration_ms): # type: (int) -> str + if expiration_ms > 0: + return str(datetime.datetime.fromtimestamp(expiration_ms / 1000)) + if expiration_ms < 0: + return 'never' + return '' + + +def _folder_share_expiration_lookups(rq): # type: (folder_pb2.SharedFolderUpdateV3Request) -> Tuple[Dict[str, int], Dict[str, int]] + user_exp = {} + for uo in list(rq.sharedFolderAddUser) + list(rq.sharedFolderUpdateUser): + if uo.expiration: + user_exp[uo.username.lower()] = uo.expiration + team_exp = {} + for to in list(rq.sharedFolderAddTeam) + list(rq.sharedFolderUpdateTeam): + if to.expiration: + team_exp[utils.base64_url_encode(to.teamUid)] = to.expiration + return user_exp, team_exp + + +def _record_share_expiration_lookup(rq): # type: (record_pb2.RecordShareUpdateRequest) -> Dict[Tuple[str, str], int] + lookup = {} + for attr in ('addSharedRecord', 'updateSharedRecord'): + for ro in getattr(rq, attr): + if ro.expiration: + lookup[(utils.base64_url_encode(ro.recordUid), ro.toUsername.lower())] = ro.expiration + return lookup + + class ShareFolderCommand(Command): def get_parser(self): return share_folder_parser @@ -475,7 +504,8 @@ def prepare_record_share_request(params, kwargs, shared_folder_uid, users, rec_u Expiration must not be set on SharedFolderUpdateRecord — that removes the record from the shared folder (and the owner's vault) when it expires. Per-user record - expiration is applied through the record share API instead. + expiration is applied through the record share API; folder user/team expiration + is set separately via shared_folder_update_v3 in prepare_request. When setting a positive expiration, the server only inserts expiration rows on create, so we revoke the existing share and re-grant in separate requests. @@ -588,17 +618,8 @@ def prepare_request(params, kwargs, curr_sf, users, teams, rec_uids, *, action = kwargs.get('action') or 'grant' mr = kwargs.get('manage_records') mu = kwargs.get('manage_users') - per_record_timer = bool(rec_uids) and isinstance(share_expiration, int) - def apply_share_expiration(target): - """Set expiration / timer / rotateOnExpiration on a User/Team share update proto. - - When -r and --expire-in are used together, the timer belongs on the per-user - record share (records_share_update), not the folder user share. The server - enforces a longer minimum (e.g. 5 minutes) on folder-level ROE timers. - """ - if per_record_timer: - return + """Set expiration / timer / rotateOnExpiration on a User/Team share update proto.""" if not isinstance(share_expiration, int): return if share_expiration > 0: @@ -635,8 +656,7 @@ def apply_share_expiration(target): or (current_user.get('manage_records') is True) == (mr == 'on')) mu_unchanged = (mu is None or (current_user.get('manage_users') is True) == (mu == 'on')) - if mr_unchanged and mu_unchanged and ( - per_record_timer or not isinstance(share_expiration, int)): + if mr_unchanged and mu_unchanged and not isinstance(share_expiration, int): continue uo.manageRecords = folder_pb2.BOOLEAN_NO_CHANGE if mr is None else folder_pb2.BOOLEAN_TRUE if mr == 'on' else folder_pb2.BOOLEAN_FALSE uo.manageUsers = folder_pb2.BOOLEAN_NO_CHANGE if mu is None else folder_pb2.BOOLEAN_TRUE if mu == 'on' else folder_pb2.BOOLEAN_FALSE @@ -764,7 +784,8 @@ def send_requests(params, partitioned_requests): try: rss = api.communicate_rest(params, rqs, 'vault/shared_folder_update_v3', payload_version=1, rs_type=folder_pb2.SharedFolderUpdateV3ResponseV2) - for rs in rss.sharedFoldersUpdateV3Response: + for rq_item, rs in zip(chunk, rss.sharedFoldersUpdateV3Response): + user_exp, team_exp = _folder_share_expiration_lookups(rq_item) team_cache = params.available_team_cache or [] for attr in ( 'sharedFolderAddTeamStatus', 'sharedFolderUpdateTeamStatus', @@ -776,11 +797,13 @@ def send_requests(params, partitioned_requests): team = next((x for x in team_cache if x.get('team_uid') == team_uid), None) if team: status = t.status + exp_text = format_share_expiration_ms(team_exp.get(team_uid, 0)) + exp_suffix = f', folder access expires {exp_text}' if exp_text else '' if status == 'success': - logging.info('Team share \'%s\' %s', team['team_name'], + logging.info('Team share \'%s\' %s%s', team['team_name'], 'added' if attr == 'sharedFolderAddTeamStatus' else 'updated' if attr == 'sharedFolderUpdateTeamStatus' else - 'removed') + 'removed', exp_suffix) else: logging.warning('Team share \'%s\' failed', team['team_name']) @@ -792,11 +815,13 @@ def send_requests(params, partitioned_requests): for s in statuses: username = s.username status = s.status + exp_text = format_share_expiration_ms(user_exp.get(username.lower(), 0)) + exp_suffix = f', folder access expires {exp_text}' if exp_text else '' if status == 'success': - logging.info('User share \'%s\' %s', username, + logging.info('User share \'%s\' %s%s', username, 'added' if attr == 'sharedFolderAddUserStatus' else 'updated' if attr == 'sharedFolderUpdateUserStatus' else - 'removed') + 'removed', exp_suffix) elif status == 'invited': logging.info('User \'%s\' invited', username) else: @@ -1219,6 +1244,7 @@ def send_requests(params, requests): left -= added rs = api.communicate_rest(params, rq1, 'vault/records_share_update', rs_type=record_pb2.RecordShareUpdateResponse) + record_exp = _record_share_expiration_lookup(rq1) for attr in ['addSharedRecordStatus', 'updateSharedRecordStatus', 'removeSharedRecordStatus']: if hasattr(rs, attr): statuses = getattr(rs, attr) @@ -1228,7 +1254,11 @@ def send_requests(params, requests): email = status_rs.username if status == 'success': verb = 'granted to' if attr == 'addSharedRecordStatus' else 'changed for' if attr == 'updateSharedRecordStatus' else 'revoked from' - logging.info('Record \"%s\" access permissions has been %s user \'%s\'', record_uid, verb, email) + exp_text = format_share_expiration_ms( + record_exp.get((record_uid, email.lower()), 0)) + exp_suffix = f', record access expires {exp_text}' if exp_text else '' + logging.info('Record \"%s\" access permissions has been %s user \'%s\'%s', + record_uid, verb, email, exp_suffix) else: verb = 'grant' if attr == 'addSharedRecordStatus' else 'change' if attr == 'updateSharedRecordStatus' else 'revoke' diff --git a/unit-tests/test_command_register.py b/unit-tests/test_command_register.py index 803cc86a4..164416cb0 100644 --- a/unit-tests/test_command_register.py +++ b/unit-tests/test_command_register.py @@ -349,8 +349,8 @@ def test_share_folder_prepare_request_sets_rotate_on_expiration(self): self.assertFalse(record_msgs, 'record protos must not carry folder-wide expiration') - def test_share_folder_prepare_request_skips_record_expiration_when_records_specified(self): - """Per-record expiration is routed through the record share API, not SharedFolderUpdateRecord.""" + def test_share_folder_prepare_request_sets_folder_and_record_expiration_when_records_specified(self): + """With -r and --expire-in, folder user and record share both get timers; not SharedFolderUpdateRecord.""" params = get_synced_params() shared_folder_uid = next(iter(params.shared_folder_cache.keys())) record_uid = next(iter([x['record_uid'] for x in params.meta_data_cache.values() if x['can_share']])) @@ -360,6 +360,9 @@ def test_share_folder_prepare_request_skips_record_expiration_when_records_speci curr_sf.setdefault('records', [{'record_uid': record_uid, 'can_edit': True, 'can_share': True}]) future_ts = int(datetime.datetime.now().timestamp()) + 86_400 + params.key_cache['user2@keepersecurity.com'] = mock.MagicMock( + rsa=utils.base64_url_decode(vault_env.encoded_public_key), ec=None) + rq = register.ShareFolderCommand.prepare_request( params, kwargs={'action': 'grant', 'can_edit': 'on', 'can_share': 'on'}, @@ -374,9 +377,11 @@ def test_share_folder_prepare_request_skips_record_expiration_when_records_speci user_msgs = list(rq.sharedFolderAddUser) + list(rq.sharedFolderUpdateUser) record_msgs = list(rq.sharedFolderAddRecord) + list(rq.sharedFolderUpdateRecord) + self.assertTrue(user_msgs, 'expected folder user share with expiration') for m in user_msgs: - self.assertEqual(m.expiration, 0) - self.assertFalse(m.rotateOnExpiration) + self.assertGreater(m.expiration, 0) + self.assertTrue(m.rotateOnExpiration) + self.assertEqual(m.timerNotificationType, record_pb2.NOTIFY_OWNER) self.assertTrue(record_msgs, 'expected record permission update') for m in record_msgs: From 8a868d5d4ea974fc07712a4a2560a674a47b4136 Mon Sep 17 00:00:00 2001 From: sshrushanth-ks Date: Wed, 24 Jun 2026 16:13:05 +0530 Subject: [PATCH 3/9] updated test file --- unit-tests/test_command_register.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/unit-tests/test_command_register.py b/unit-tests/test_command_register.py index 164416cb0..0adb151b5 100644 --- a/unit-tests/test_command_register.py +++ b/unit-tests/test_command_register.py @@ -447,7 +447,9 @@ def test_share_folder_prepare_request_skips_redundant_user_update_for_record_onl ) self.assertFalse(list(rq.sharedFolderUpdateUser)) - self.assertTrue(list(rq.sharedFolderUpdateRecord)) + self.assertTrue(list(rq.sharedFolderAddRecord), + 'new record grant should update folder record permissions via add') + self.assertFalse(list(rq.sharedFolderUpdateRecord)) def test_share_folder_prepare_request_updates_user_when_folder_wide_expiration(self): """Folder-wide --expire-in (no -r) sets expiration on the folder user share.""" @@ -480,9 +482,10 @@ def test_share_folder_rotate_on_expiration_rejects_folder_without_pam_user(self) params = get_synced_params() shared_folder_uid = next(iter(params.shared_folder_cache.keys())) cmd = register.ShareFolderCommand() - with self.assertRaises(CommandError) as ctx: - cmd.execute(params, action='grant', user=['user2@keepersecurity.com'], - folder=shared_folder_uid, expire_in='1d', rotate_on_expiration=True) + with mock.patch('keepercommander.commands.register.SyncDownCommand.execute'): + with self.assertRaises(CommandError) as ctx: + cmd.execute(params, action='grant', user=['user2@keepersecurity.com'], + folder=shared_folder_uid, expire_in='1d', rotate_on_expiration=True) self.assertIn('pamUser', str(ctx.exception)) @staticmethod From 8166b54b4c9c884db1a5a565abd90779807b0712 Mon Sep 17 00:00:00 2001 From: sshrushanth-ks Date: Sun, 28 Jun 2026 22:52:15 +0530 Subject: [PATCH 4/9] Fix share-folder remove vault deletion; clean up access grant/remove logs --- keepercommander/commands/register.py | 248 +++++++++++++++++---------- unit-tests/test_command_register.py | 45 +++++ 2 files changed, 201 insertions(+), 92 deletions(-) diff --git a/keepercommander/commands/register.py b/keepercommander/commands/register.py index dd003307f..757f92809 100644 --- a/keepercommander/commands/register.py +++ b/keepercommander/commands/register.py @@ -256,7 +256,9 @@ def get_share_expiration(expire_at, expire_in, cmd_name='share-record'): # ( return expiration_seconds -def format_share_expiration_ms(expiration_ms): # type: (int) -> str +def format_share_expiration_ms(expiration_ms): + # type: (int) -> str + """Format a share expiration timestamp (milliseconds) for log output.""" if expiration_ms > 0: return str(datetime.datetime.fromtimestamp(expiration_ms / 1000)) if expiration_ms < 0: @@ -264,24 +266,29 @@ def format_share_expiration_ms(expiration_ms): # type: (int) -> str return '' -def _folder_share_expiration_lookups(rq): # type: (folder_pb2.SharedFolderUpdateV3Request) -> Tuple[Dict[str, int], Dict[str, int]] +def _folder_share_expiration_lookups(rq): + # type: (folder_pb2.SharedFolderUpdateV3Request) -> Tuple[Dict[str, int], Dict[str, int]] + """Map folder user/team names to expiration values from an outgoing folder update request.""" user_exp = {} - for uo in list(rq.sharedFolderAddUser) + list(rq.sharedFolderUpdateUser): - if uo.expiration: - user_exp[uo.username.lower()] = uo.expiration + for folder_user in list(rq.sharedFolderAddUser) + list(rq.sharedFolderUpdateUser): + if folder_user.expiration: + user_exp[folder_user.username.lower()] = folder_user.expiration team_exp = {} - for to in list(rq.sharedFolderAddTeam) + list(rq.sharedFolderUpdateTeam): - if to.expiration: - team_exp[utils.base64_url_encode(to.teamUid)] = to.expiration + for folder_team in list(rq.sharedFolderAddTeam) + list(rq.sharedFolderUpdateTeam): + if folder_team.expiration: + team_exp[utils.base64_url_encode(folder_team.teamUid)] = folder_team.expiration return user_exp, team_exp -def _record_share_expiration_lookup(rq): # type: (record_pb2.RecordShareUpdateRequest) -> Dict[Tuple[str, str], int] +def _record_share_expiration_lookup(rq): + # type: (record_pb2.RecordShareUpdateRequest) -> Dict[Tuple[str, str], int] + """Map (record_uid, username) pairs to expiration values from an outgoing record share request.""" lookup = {} for attr in ('addSharedRecord', 'updateSharedRecord'): - for ro in getattr(rq, attr): - if ro.expiration: - lookup[(utils.base64_url_encode(ro.recordUid), ro.toUsername.lower())] = ro.expiration + for shared_record in getattr(rq, attr): + if shared_record.expiration: + lookup[(utils.base64_url_encode(shared_record.recordUid), + shared_record.toUsername.lower())] = shared_record.expiration return lookup @@ -482,9 +489,12 @@ def prep_rq(recs, users, curr_sf): group_idx += 1 self.send_requests(params, rq_groups) - if (action == 'grant' and isinstance(share_expiration, int) and record_uids and as_users): - api.load_user_public_keys(params, list(as_users), send_invites=False) - SyncDownCommand().execute(params, force=True) + if record_uids and as_users and ( + action == 'remove' + or (action == 'grant' and isinstance(share_expiration, int))): + if action == 'grant': + api.load_user_public_keys(params, list(as_users), send_invites=False) + SyncDownCommand().execute(params, force=True) record_share_requests = [] for sf_uid in shared_folder_uids: sh_fol = params.shared_folder_cache.get(sf_uid, {'shared_folder_uid': sf_uid}) @@ -495,68 +505,85 @@ def prep_rq(recs, users, curr_sf): if rs_rqs: record_share_requests.extend(rs_rqs) if record_share_requests: - ShareRecordCommand.send_requests(params, record_share_requests) + ShareRecordCommand.send_requests( + params, record_share_requests, share_folder_access_log=action) @staticmethod def prepare_record_share_request(params, kwargs, shared_folder_uid, users, rec_uids, *, curr_sf, share_expiration=None, rotate_on_expiration=False): - """Build RecordShareUpdateRequest(s) for per-record share expiration in a shared folder. + """Build RecordShareUpdateRequest(s) for shared-folder record access changes. - Expiration must not be set on SharedFolderUpdateRecord — that removes the record - from the shared folder (and the owner's vault) when it expires. Per-user record - expiration is applied through the record share API; folder user/team expiration - is set separately via shared_folder_update_v3 in prepare_request. + Grant with expiration: per-user record timers via records_share_update (not + SharedFolderUpdateRecord, which removes the record from the folder when it + expires). Positive expiration uses revoke-then-grant because the server only + inserts expiration rows on create. - When setting a positive expiration, the server only inserts expiration rows on - create, so we revoke the existing share and re-grant in separate requests. + Remove with user and record: removeSharedRecord for that user only; does not + remove the record from the folder or the user from the folder. """ - if not rec_uids or not users or not isinstance(share_expiration, int): + if not rec_uids or not users: return None - if (kwargs.get('action') or 'grant') != 'grant': + action = kwargs.get('action') or 'grant' + sf_uid_bytes = utils.base64_url_decode(shared_folder_uid) + + if action == 'remove': + rq_remove = record_pb2.RecordShareUpdateRequest() + for record_uid in rec_uids: + for email in users: + shared_record = record_pb2.SharedRecord() + shared_record.toUsername = email + shared_record.recordUid = utils.base64_url_decode(record_uid) + shared_record.sharedFolderUid = sf_uid_bytes + rq_remove.removeSharedRecord.append(shared_record) + return [rq_remove] if rq_remove.removeSharedRecord else None + + if not isinstance(share_expiration, int): return None ce = kwargs.get('can_edit') cs = kwargs.get('can_share') - sf_uid_bytes = utils.base64_url_decode(shared_folder_uid) regrant_for_expiration = share_expiration > 0 - def apply_share_expiration(ro): # type: (record_pb2.SharedRecord) -> None + def apply_share_expiration(shared_record): + # type: (record_pb2.SharedRecord) -> None + """Set expiration / ROE fields on a SharedRecord proto.""" if share_expiration > 0: - ro.expiration = share_expiration * 1000 - ro.timerNotificationType = record_pb2.NOTIFY_OWNER + shared_record.expiration = share_expiration * 1000 + shared_record.timerNotificationType = record_pb2.NOTIFY_OWNER if rotate_on_expiration: - ro.rotateOnExpiration = True + shared_record.rotateOnExpiration = True elif share_expiration < 0: - ro.expiration = -1 + shared_record.expiration = -1 def build_shared_record(email, record_uid, rec, existing=None): # type: (str, str, dict, Optional[dict]) -> Optional[record_pb2.SharedRecord] - ro = record_pb2.SharedRecord() - ro.toUsername = email - ro.recordUid = utils.base64_url_decode(record_uid) - ro.sharedFolderUid = sf_uid_bytes + """Build a SharedRecord proto for addSharedRecord with keys and permissions.""" + shared_record = record_pb2.SharedRecord() + shared_record.toUsername = email + shared_record.recordUid = utils.base64_url_decode(record_uid) + shared_record.sharedFolderUid = sf_uid_bytes record_key = rec.get('record_key_unencrypted') keys = params.key_cache.get(email) if not record_key or not keys or not (keys.rsa or keys.ec): return None if params.forbid_rsa and keys.ec: ec_key = crypto.load_ec_public_key(keys.ec) - ro.recordKey = crypto.encrypt_ec(record_key, ec_key) - ro.useEccKey = True + shared_record.recordKey = crypto.encrypt_ec(record_key, ec_key) + shared_record.useEccKey = True elif not params.forbid_rsa and keys.rsa: rsa_key = crypto.load_rsa_public_key(keys.rsa) - ro.recordKey = crypto.encrypt_rsa(record_key, rsa_key) - ro.useEccKey = False + shared_record.recordKey = crypto.encrypt_rsa(record_key, rsa_key) + shared_record.useEccKey = False if existing: - ro.editable = ce == 'on' if ce is not None else existing.get('editable', False) - ro.shareable = cs == 'on' if cs is not None else existing.get('shareable', False) + shared_record.editable = ce == 'on' if ce is not None else existing.get('editable', False) + shared_record.shareable = cs == 'on' if cs is not None else existing.get('shareable', False) else: default_ce = curr_sf.get('default_can_edit') is True default_cs = curr_sf.get('default_can_share') is True - ro.editable = ce == 'on' if ce is not None else default_ce - ro.shareable = cs == 'on' if cs is not None else default_cs - apply_share_expiration(ro) - return ro + shared_record.editable = ce == 'on' if ce is not None else default_ce + shared_record.shareable = cs == 'on' if cs is not None else default_cs + apply_share_expiration(shared_record) + return shared_record rq_remove = record_pb2.RecordShareUpdateRequest() rq_add = record_pb2.RecordShareUpdateRequest() @@ -574,27 +601,27 @@ def build_shared_record(email, record_uid, rec, existing=None): email_key = email.lower() existing = existing_shares.get(email_key) if regrant_for_expiration: - ro_remove = record_pb2.SharedRecord() - ro_remove.toUsername = email - ro_remove.recordUid = utils.base64_url_decode(record_uid) - ro_remove.sharedFolderUid = sf_uid_bytes - rq_remove.removeSharedRecord.append(ro_remove) - ro_add = build_shared_record(email, record_uid, rec, existing) - if ro_add: - rq_add.addSharedRecord.append(ro_add) + remove_shared_record = record_pb2.SharedRecord() + remove_shared_record.toUsername = email + remove_shared_record.recordUid = utils.base64_url_decode(record_uid) + remove_shared_record.sharedFolderUid = sf_uid_bytes + rq_remove.removeSharedRecord.append(remove_shared_record) + add_shared_record = build_shared_record(email, record_uid, rec, existing) + if add_shared_record: + rq_add.addSharedRecord.append(add_shared_record) elif existing: - ro = record_pb2.SharedRecord() - ro.toUsername = email - ro.recordUid = utils.base64_url_decode(record_uid) - ro.sharedFolderUid = sf_uid_bytes - ro.editable = ce == 'on' if ce is not None else existing.get('editable', False) - ro.shareable = cs == 'on' if cs is not None else existing.get('shareable', False) - apply_share_expiration(ro) - rq_update.updateSharedRecord.append(ro) + shared_record = record_pb2.SharedRecord() + shared_record.toUsername = email + shared_record.recordUid = utils.base64_url_decode(record_uid) + shared_record.sharedFolderUid = sf_uid_bytes + shared_record.editable = ce == 'on' if ce is not None else existing.get('editable', False) + shared_record.shareable = cs == 'on' if cs is not None else existing.get('shareable', False) + apply_share_expiration(shared_record) + rq_update.updateSharedRecord.append(shared_record) else: - ro_add = build_shared_record(email, record_uid, rec) - if ro_add: - rq_add.addSharedRecord.append(ro_add) + add_shared_record = build_shared_record(email, record_uid, rec) + if add_shared_record: + rq_add.addSharedRecord.append(add_shared_record) result = [] if rq_remove.removeSharedRecord: @@ -618,6 +645,8 @@ def prepare_request(params, kwargs, curr_sf, users, teams, rec_uids, *, action = kwargs.get('action') or 'grant' mr = kwargs.get('manage_records') mu = kwargs.get('manage_users') + skip_folder_record_remove = action == 'remove' and bool(rec_uids) and bool(users) + def apply_share_expiration(target): """Set expiration / timer / rotateOnExpiration on a User/Team share update proto.""" if not isinstance(share_expiration, int): @@ -745,31 +774,31 @@ def apply_share_expiration(target): if len(rec_uids) > 0: existing_records = {x['record_uid'] for x in curr_sf.get('records', [])} for record_uid in rec_uids: - ro = folder_pb2.SharedFolderUpdateRecord() - ro.recordUid = utils.base64_url_decode(record_uid) + folder_record_update = folder_pb2.SharedFolderUpdateRecord() + folder_record_update.recordUid = utils.base64_url_decode(record_uid) if record_uid in existing_records: if action == 'grant': - ro.canEdit = folder_pb2.BOOLEAN_NO_CHANGE if ce is None else folder_pb2.BOOLEAN_TRUE if ce == 'on' else folder_pb2.BOOLEAN_FALSE - ro.canShare = folder_pb2.BOOLEAN_NO_CHANGE if cs is None else folder_pb2.BOOLEAN_TRUE if cs == 'on' else folder_pb2.BOOLEAN_FALSE - rq.sharedFolderUpdateRecord.append(ro) - elif action == 'remove': - rq.sharedFolderRemoveRecord.append(ro.recordUid) + folder_record_update.canEdit = folder_pb2.BOOLEAN_NO_CHANGE if ce is None else folder_pb2.BOOLEAN_TRUE if ce == 'on' else folder_pb2.BOOLEAN_FALSE + folder_record_update.canShare = folder_pb2.BOOLEAN_NO_CHANGE if cs is None else folder_pb2.BOOLEAN_TRUE if cs == 'on' else folder_pb2.BOOLEAN_FALSE + rq.sharedFolderUpdateRecord.append(folder_record_update) + elif action == 'remove' and not skip_folder_record_remove: + rq.sharedFolderRemoveRecord.append(folder_record_update.recordUid) else: if action == 'grant': default_ce = folder_pb2.BOOLEAN_TRUE if curr_sf.get('default_can_edit') is True else folder_pb2.BOOLEAN_FALSE default_cs = folder_pb2.BOOLEAN_TRUE if curr_sf.get('default_can_share') is True else folder_pb2.BOOLEAN_FALSE - ro.canEdit = default_ce if ce is None else folder_pb2.BOOLEAN_TRUE if ce == 'on' else folder_pb2.BOOLEAN_FALSE - ro.canShare = default_cs if cs is None else folder_pb2.BOOLEAN_TRUE if cs == 'on' else folder_pb2.BOOLEAN_FALSE + folder_record_update.canEdit = default_ce if ce is None else folder_pb2.BOOLEAN_TRUE if ce == 'on' else folder_pb2.BOOLEAN_FALSE + folder_record_update.canShare = default_cs if cs is None else folder_pb2.BOOLEAN_TRUE if cs == 'on' else folder_pb2.BOOLEAN_FALSE sf_key = curr_sf.get('shared_folder_key_unencrypted') if sf_key: rec = params.record_cache[record_uid] rec_key = rec['record_key_unencrypted'] if rec.get('version', 0) < 3: - ro.encryptedRecordKey = crypto.encrypt_aes_v1(rec_key, sf_key) + folder_record_update.encryptedRecordKey = crypto.encrypt_aes_v1(rec_key, sf_key) else: - ro.encryptedRecordKey = crypto.encrypt_aes_v2(rec_key, sf_key) - rq.sharedFolderAddRecord.append(ro) + folder_record_update.encryptedRecordKey = crypto.encrypt_aes_v2(rec_key, sf_key) + rq.sharedFolderAddRecord.append(folder_record_update) return rq @staticmethod @@ -816,12 +845,21 @@ def send_requests(params, partitioned_requests): username = s.username status = s.status exp_text = format_share_expiration_ms(user_exp.get(username.lower(), 0)) - exp_suffix = f', folder access expires {exp_text}' if exp_text else '' if status == 'success': - logging.info('User share \'%s\' %s%s', username, - 'added' if attr == 'sharedFolderAddUserStatus' else - 'updated' if attr == 'sharedFolderUpdateUserStatus' else - 'removed', exp_suffix) + if exp_text and attr in ( + 'sharedFolderAddUserStatus', 'sharedFolderUpdateUserStatus'): + logging.info( + 'Folder access granted to user \'%s\', expires %s', + username, exp_text) + elif attr == 'sharedFolderRemoveUserStatus': + logging.info( + 'Folder access removed from user \'%s\'', username) + else: + exp_suffix = f', folder access expires {exp_text}' if exp_text else '' + logging.info('User share \'%s\' %s%s', username, + 'added' if attr == 'sharedFolderAddUserStatus' else + 'updated' if attr == 'sharedFolderUpdateUserStatus' else + 'removed', exp_suffix) elif status == 'invited': logging.info('User \'%s\' invited', username) else: @@ -840,10 +878,15 @@ def send_requests(params, partitioned_requests): else: title = record_uid if status == 'success': - logging.info('Record share \'%s\' %s', title, - 'added' if attr == 'sharedFolderAddRecordStatus' else - 'updated' if attr == 'sharedFolderUpdateRecordStatus' else - 'removed') + if user_exp and attr in ( + 'sharedFolderAddRecordStatus', + 'sharedFolderUpdateRecordStatus'): + pass + else: + logging.info('Record share \'%s\' %s', title, + 'added' if attr == 'sharedFolderAddRecordStatus' else + 'updated' if attr == 'sharedFolderUpdateRecordStatus' else + 'removed') else: logging.warning('Record share \'%s\' failed', title) except KeeperApiError as kae: @@ -1221,7 +1264,7 @@ def get_contact(user, contacts): rq and ShareRecordCommand.send_requests(params, [rq]) @staticmethod - def send_requests(params, requests): + def send_requests(params, requests, share_folder_access_log=None): requests = iter(requests) rq = next(requests, None) while rq and (len(rq.addSharedRecord) > 0 or len(rq.updateSharedRecord) > 0 or len(rq.removeSharedRecord) > 0): @@ -1253,12 +1296,33 @@ def send_requests(params, requests): status = status_rs.status email = status_rs.username if status == 'success': - verb = 'granted to' if attr == 'addSharedRecordStatus' else 'changed for' if attr == 'updateSharedRecordStatus' else 'revoked from' - exp_text = format_share_expiration_ms( - record_exp.get((record_uid, email.lower()), 0)) - exp_suffix = f', record access expires {exp_text}' if exp_text else '' - logging.info('Record \"%s\" access permissions has been %s user \'%s\'%s', - record_uid, verb, email, exp_suffix) + if share_folder_access_log == 'grant': + if attr in ('addSharedRecordStatus', 'updateSharedRecordStatus'): + exp_text = format_share_expiration_ms( + record_exp.get((record_uid, email.lower()), 0)) + if exp_text: + logging.info( + 'Record access granted to user \'%s\' for record \'%s\', ' + 'expires %s', email, record_uid, exp_text) + else: + logging.info( + 'Record access granted to user \'%s\' for record \'%s\'', + email, record_uid) + elif share_folder_access_log == 'remove': + if attr == 'removeSharedRecordStatus': + logging.info( + 'Record access removed from user \'%s\' for record \'%s\'', + email, record_uid) + elif not share_folder_access_log: + verb = ('granted to' if attr == 'addSharedRecordStatus' + else 'changed for' if attr == 'updateSharedRecordStatus' + else 'revoked from') + exp_text = format_share_expiration_ms( + record_exp.get((record_uid, email.lower()), 0)) + exp_suffix = f', record access expires {exp_text}' if exp_text else '' + logging.info( + 'Record \"%s\" access permissions has been %s user \'%s\'%s', + record_uid, verb, email, exp_suffix) else: verb = 'grant' if attr == 'addSharedRecordStatus' else 'change' if attr == 'updateSharedRecordStatus' else 'revoke' diff --git a/unit-tests/test_command_register.py b/unit-tests/test_command_register.py index 0adb151b5..6cef6596b 100644 --- a/unit-tests/test_command_register.py +++ b/unit-tests/test_command_register.py @@ -421,6 +421,51 @@ def test_share_folder_prepare_record_share_request_sets_expiration(self): self.assertEqual(sr.timerNotificationType, record_pb2.NOTIFY_OWNER) self.assertEqual(utils.base64_url_encode(sr.sharedFolderUid), shared_folder_uid) + def test_share_folder_prepare_request_remove_user_record_keeps_record_in_folder(self): + """Removing user access to one record removes the user from the folder but not the record.""" + params = get_synced_params() + shared_folder_uid = next(iter(params.shared_folder_cache.keys())) + record_uid = next(iter(params.shared_folder_cache[shared_folder_uid]['records']))['record_uid'] + + curr_sf = dict(params.shared_folder_cache[shared_folder_uid]) + curr_sf['users'] = [{ + 'username': 'user2@keepersecurity.com', + 'manage_records': True, + 'manage_users': True, + }] + + rq = register.ShareFolderCommand.prepare_request( + params, + kwargs={'action': 'remove'}, + curr_sf=curr_sf, + users=['user2@keepersecurity.com'], + teams=[], + rec_uids=[record_uid], + ) + + self.assertTrue(list(rq.sharedFolderRemoveUser)) + self.assertFalse(list(rq.sharedFolderRemoveRecord)) + + def test_share_folder_prepare_record_share_request_remove_user_record(self): + params = get_synced_params() + shared_folder_uid = next(iter(params.shared_folder_cache.keys())) + record_uid = next(iter(params.shared_folder_cache[shared_folder_uid]['records']))['record_uid'] + curr_sf = dict(params.shared_folder_cache[shared_folder_uid]) + + rq_list = register.ShareFolderCommand.prepare_record_share_request( + params, + kwargs={'action': 'remove'}, + shared_folder_uid=shared_folder_uid, + users=['user2@keepersecurity.com'], + rec_uids=[record_uid], + curr_sf=curr_sf, + ) + + self.assertIsNotNone(rq_list) + self.assertEqual(len(rq_list), 1) + self.assertTrue(rq_list[0].removeSharedRecord) + self.assertFalse(rq_list[0].addSharedRecord) + def test_share_folder_prepare_request_skips_redundant_user_update_for_record_only(self): """When sharing another record without expiration, skip redundant folder user update.""" params = get_synced_params() From d5b9b3111973d8cb6d3158a247ccd51e35ca6796 Mon Sep 17 00:00:00 2001 From: amangalampalli-ks Date: Tue, 30 Jun 2026 20:56:09 +0530 Subject: [PATCH 5/9] Fix share folder expire in and -r combination --- keepercommander/commands/register.py | 141 ++++----------------------- unit-tests/test_command_register.py | 20 +--- 2 files changed, 25 insertions(+), 136 deletions(-) diff --git a/keepercommander/commands/register.py b/keepercommander/commands/register.py index 757f92809..4f4d8de0d 100644 --- a/keepercommander/commands/register.py +++ b/keepercommander/commands/register.py @@ -115,9 +115,12 @@ def register_command_info(aliases, command_info): 'initial sharing action') expiration = share_folder_parser.add_mutually_exclusive_group() expiration.add_argument('--expire-at', dest='expire_at', action='store', metavar='TIMESTAMP', - help='share expiration: never or ISO datetime (yyyy-MM-dd[ hh:mm:ss])') + help='folder access expiration: never or ISO datetime (yyyy-MM-dd[ hh:mm:ss]). ' + 'Records inherit folder timing; -r sets record permissions only.') expiration.add_argument('--expire-in', dest='expire_in', action='store', metavar='PERIOD', - help='share expiration: never or period ([(y)ears|(mo)nths|(d)ays|(h)ours(mi)nutes]') + help='folder access expiration: never or period ' + '([(y)ears|(mo)nths|(d)ays|(h)ours|(mi)nutes]). ' + 'Records inherit folder timing; -r sets record permissions only.') share_folder_parser.add_argument('-roe', '--rotate-on-expiration', dest='rotate_on_expiration', action='store_true', help='rotate the password when the share access expires. ' 'Only valid on grant; requires a positive --expire-at/--expire-in ' @@ -489,19 +492,13 @@ def prep_rq(recs, users, curr_sf): group_idx += 1 self.send_requests(params, rq_groups) - if record_uids and as_users and ( - action == 'remove' - or (action == 'grant' and isinstance(share_expiration, int))): - if action == 'grant': - api.load_user_public_keys(params, list(as_users), send_invites=False) - SyncDownCommand().execute(params, force=True) + if record_uids and as_users and action == 'remove': record_share_requests = [] for sf_uid in shared_folder_uids: sh_fol = params.shared_folder_cache.get(sf_uid, {'shared_folder_uid': sf_uid}) rs_rqs = ShareFolderCommand.prepare_record_share_request( params, kwargs, sf_uid, list(as_users), list(record_uids), - curr_sf=sh_fol, share_expiration=share_expiration, - rotate_on_expiration=rotate_on_expiration) + curr_sf=sh_fol) if rs_rqs: record_share_requests.extend(rs_rqs) if record_share_requests: @@ -510,127 +507,31 @@ def prep_rq(recs, users, curr_sf): @staticmethod def prepare_record_share_request(params, kwargs, shared_folder_uid, users, rec_uids, *, - curr_sf, share_expiration=None, rotate_on_expiration=False): - """Build RecordShareUpdateRequest(s) for shared-folder record access changes. + curr_sf): + """Build RecordShareUpdateRequest(s) to revoke per-user record access on remove. - Grant with expiration: per-user record timers via records_share_update (not - SharedFolderUpdateRecord, which removes the record from the folder when it - expires). Positive expiration uses revoke-then-grant because the server only - inserts expiration rows on create. + --expire-in/--expire-at apply to folder user access only (via + shared_folder_update_v3). Records in the folder inherit folder timing; -r sets + record permissions only (can_edit/can_share) on SharedFolderUpdateRecord. Remove with user and record: removeSharedRecord for that user only; does not - remove the record from the folder or the user from the folder. + remove the record from the folder or the owner's vault. """ if not rec_uids or not users: return None - action = kwargs.get('action') or 'grant' - sf_uid_bytes = utils.base64_url_decode(shared_folder_uid) - - if action == 'remove': - rq_remove = record_pb2.RecordShareUpdateRequest() - for record_uid in rec_uids: - for email in users: - shared_record = record_pb2.SharedRecord() - shared_record.toUsername = email - shared_record.recordUid = utils.base64_url_decode(record_uid) - shared_record.sharedFolderUid = sf_uid_bytes - rq_remove.removeSharedRecord.append(shared_record) - return [rq_remove] if rq_remove.removeSharedRecord else None - - if not isinstance(share_expiration, int): + if (kwargs.get('action') or 'grant') != 'remove': return None - - ce = kwargs.get('can_edit') - cs = kwargs.get('can_share') - regrant_for_expiration = share_expiration > 0 - - def apply_share_expiration(shared_record): - # type: (record_pb2.SharedRecord) -> None - """Set expiration / ROE fields on a SharedRecord proto.""" - if share_expiration > 0: - shared_record.expiration = share_expiration * 1000 - shared_record.timerNotificationType = record_pb2.NOTIFY_OWNER - if rotate_on_expiration: - shared_record.rotateOnExpiration = True - elif share_expiration < 0: - shared_record.expiration = -1 - - def build_shared_record(email, record_uid, rec, existing=None): - # type: (str, str, dict, Optional[dict]) -> Optional[record_pb2.SharedRecord] - """Build a SharedRecord proto for addSharedRecord with keys and permissions.""" - shared_record = record_pb2.SharedRecord() - shared_record.toUsername = email - shared_record.recordUid = utils.base64_url_decode(record_uid) - shared_record.sharedFolderUid = sf_uid_bytes - record_key = rec.get('record_key_unencrypted') - keys = params.key_cache.get(email) - if not record_key or not keys or not (keys.rsa or keys.ec): - return None - if params.forbid_rsa and keys.ec: - ec_key = crypto.load_ec_public_key(keys.ec) - shared_record.recordKey = crypto.encrypt_ec(record_key, ec_key) - shared_record.useEccKey = True - elif not params.forbid_rsa and keys.rsa: - rsa_key = crypto.load_rsa_public_key(keys.rsa) - shared_record.recordKey = crypto.encrypt_rsa(record_key, rsa_key) - shared_record.useEccKey = False - if existing: - shared_record.editable = ce == 'on' if ce is not None else existing.get('editable', False) - shared_record.shareable = cs == 'on' if cs is not None else existing.get('shareable', False) - else: - default_ce = curr_sf.get('default_can_edit') is True - default_cs = curr_sf.get('default_can_share') is True - shared_record.editable = ce == 'on' if ce is not None else default_ce - shared_record.shareable = cs == 'on' if cs is not None else default_cs - apply_share_expiration(shared_record) - return shared_record + sf_uid_bytes = utils.base64_url_decode(shared_folder_uid) rq_remove = record_pb2.RecordShareUpdateRequest() - rq_add = record_pb2.RecordShareUpdateRequest() - rq_update = record_pb2.RecordShareUpdateRequest() for record_uid in rec_uids: - if record_uid not in params.record_cache: - continue - rec = params.record_cache[record_uid] - existing_shares = {} - shares = rec.get('shares') or {} - for po in shares.get('user_permissions') or []: - existing_shares[po['username'].lower()] = po - for email in users: - email_key = email.lower() - existing = existing_shares.get(email_key) - if regrant_for_expiration: - remove_shared_record = record_pb2.SharedRecord() - remove_shared_record.toUsername = email - remove_shared_record.recordUid = utils.base64_url_decode(record_uid) - remove_shared_record.sharedFolderUid = sf_uid_bytes - rq_remove.removeSharedRecord.append(remove_shared_record) - add_shared_record = build_shared_record(email, record_uid, rec, existing) - if add_shared_record: - rq_add.addSharedRecord.append(add_shared_record) - elif existing: - shared_record = record_pb2.SharedRecord() - shared_record.toUsername = email - shared_record.recordUid = utils.base64_url_decode(record_uid) - shared_record.sharedFolderUid = sf_uid_bytes - shared_record.editable = ce == 'on' if ce is not None else existing.get('editable', False) - shared_record.shareable = cs == 'on' if cs is not None else existing.get('shareable', False) - apply_share_expiration(shared_record) - rq_update.updateSharedRecord.append(shared_record) - else: - add_shared_record = build_shared_record(email, record_uid, rec) - if add_shared_record: - rq_add.addSharedRecord.append(add_shared_record) - - result = [] - if rq_remove.removeSharedRecord: - result.append(rq_remove) - if rq_add.addSharedRecord: - result.append(rq_add) - if rq_update.updateSharedRecord: - result.append(rq_update) - return result or None + shared_record = record_pb2.SharedRecord() + shared_record.toUsername = email + shared_record.recordUid = utils.base64_url_decode(record_uid) + shared_record.sharedFolderUid = sf_uid_bytes + rq_remove.removeSharedRecord.append(shared_record) + return [rq_remove] if rq_remove.removeSharedRecord else None @staticmethod def prepare_request(params, kwargs, curr_sf, users, teams, rec_uids, *, diff --git a/unit-tests/test_command_register.py b/unit-tests/test_command_register.py index 6cef6596b..29f81cfe2 100644 --- a/unit-tests/test_command_register.py +++ b/unit-tests/test_command_register.py @@ -350,7 +350,7 @@ def test_share_folder_prepare_request_sets_rotate_on_expiration(self): self.assertFalse(record_msgs, 'record protos must not carry folder-wide expiration') def test_share_folder_prepare_request_sets_folder_and_record_expiration_when_records_specified(self): - """With -r and --expire-in, folder user and record share both get timers; not SharedFolderUpdateRecord.""" + """With -r and --expire-in, only folder user gets a timer; record protos are permissions-only.""" params = get_synced_params() shared_folder_uid = next(iter(params.shared_folder_cache.keys())) record_uid = next(iter([x['record_uid'] for x in params.meta_data_cache.values() if x['can_share']])) @@ -388,12 +388,12 @@ def test_share_folder_prepare_request_sets_folder_and_record_expiration_when_rec self.assertEqual(m.expiration, 0) self.assertFalse(m.rotateOnExpiration) - def test_share_folder_prepare_record_share_request_sets_expiration(self): + def test_share_folder_prepare_record_share_request_ignores_grant_expiration(self): + """Grant with --expire-in does not create per-record share timers; folder timing only.""" params = get_synced_params() shared_folder_uid = next(iter(params.shared_folder_cache.keys())) record_uid = next(iter([x['record_uid'] for x in params.meta_data_cache.values() if x['can_share']])) curr_sf = dict(params.shared_folder_cache[shared_folder_uid]) - future_ts = int(datetime.datetime.now().timestamp()) + 86_400 params.key_cache['user2@keepersecurity.com'] = mock.MagicMock( rsa=utils.base64_url_decode(vault_env.encoded_public_key), ec=None) @@ -405,21 +405,9 @@ def test_share_folder_prepare_record_share_request_sets_expiration(self): users=['user2@keepersecurity.com'], rec_uids=[record_uid], curr_sf=curr_sf, - share_expiration=future_ts, - rotate_on_expiration=True, ) - self.assertIsNotNone(rq) - self.assertEqual(len(rq), 2, 'positive expiration must revoke then re-grant') - self.assertTrue(rq[0].removeSharedRecord) - self.assertTrue(rq[1].addSharedRecord) - shared_records = list(rq[1].addSharedRecord) - self.assertTrue(shared_records) - for sr in shared_records: - self.assertGreater(sr.expiration, 0) - self.assertTrue(sr.rotateOnExpiration) - self.assertEqual(sr.timerNotificationType, record_pb2.NOTIFY_OWNER) - self.assertEqual(utils.base64_url_encode(sr.sharedFolderUid), shared_folder_uid) + self.assertIsNone(rq) def test_share_folder_prepare_request_remove_user_record_keeps_record_in_folder(self): """Removing user access to one record removes the user from the folder but not the record.""" From b445c56e1d243bf860e8f88b785dbac586a0fe22 Mon Sep 17 00:00:00 2001 From: amangalampalli-ks Date: Thu, 2 Jul 2026 17:39:51 +0530 Subject: [PATCH 6/9] Separate folder and record flag usage to fix multiple remove related issue --- keepercommander/commands/register.py | 348 +++++++++++++++------------ unit-tests/test_command_register.py | 147 ++++++++--- 2 files changed, 313 insertions(+), 182 deletions(-) diff --git a/keepercommander/commands/register.py b/keepercommander/commands/register.py index 4f4d8de0d..17dfbe714 100644 --- a/keepercommander/commands/register.py +++ b/keepercommander/commands/register.py @@ -93,40 +93,58 @@ def register_command_info(aliases, command_info): '(not "never") and a pamUser record with rotation configured.') share_record_parser.add_argument('record', nargs='?', type=str, action='store', help='record/shared folder path/UID') -share_folder_parser = argparse.ArgumentParser(prog='share-folder', description='Change the permissions of a shared folder') -share_folder_parser.add_argument('-a', '--action', dest='action', choices=['grant','remove'], - default='grant', action='store', help='shared folder action. \'grant\' if omitted') -share_folder_parser.add_argument('-e', '--email', dest='user', action='append', - help='account email, team, @existing for all users and teams in the folder, ' - 'or \'*\' as default folder permission') -share_folder_parser.add_argument('-r', '--record', dest='record', action='append', - help='record name, record UID, @existing for all records in the folder,' - ' or \'*\' as default folder permission') -share_folder_parser.add_argument('-p', '--manage-records', dest='manage_records', action='store', - choices=['on', 'off'], help='account permission: can manage records.') -share_folder_parser.add_argument('-o', '--manage-users', dest='manage_users', action='store', - choices=['on', 'off'], help='account permission: can manage users.') -share_folder_parser.add_argument('-s', '--can-share', dest='can_share', action='store', - choices=['on', 'off'], help='record permission: can be shared') -share_folder_parser.add_argument('-d', '--can-edit', dest='can_edit', action='store', - choices=['on', 'off'], help='record permission: can be modified.') -share_folder_parser.add_argument('-f', '--force', dest='force', action='store_true', - help='Apply permission changes ignoring default folder permissions. Used on the ' - 'initial sharing action') -expiration = share_folder_parser.add_mutually_exclusive_group() -expiration.add_argument('--expire-at', dest='expire_at', action='store', metavar='TIMESTAMP', - help='folder access expiration: never or ISO datetime (yyyy-MM-dd[ hh:mm:ss]). ' - 'Records inherit folder timing; -r sets record permissions only.') -expiration.add_argument('--expire-in', dest='expire_in', action='store', metavar='PERIOD', - help='folder access expiration: never or period ' - '([(y)ears|(mo)nths|(d)ays|(h)ours|(mi)nutes]). ' - 'Records inherit folder timing; -r sets record permissions only.') -share_folder_parser.add_argument('-roe', '--rotate-on-expiration', dest='rotate_on_expiration', action='store_true', - help='rotate the password when the share access expires. ' - 'Only valid on grant; requires a positive --expire-at/--expire-in ' - '(not "never") and at least one pamUser record with rotation ' - 'configured in the folder.') -share_folder_parser.add_argument('folder', nargs='+', type=str, action='store', help='shared folder path or UID') +share_folder_parser = argparse.ArgumentParser( + prog='share-folder', + description='Manage shared folder access for users and teams, and record permissions in the folder.') + +folder_access = share_folder_parser.add_argument_group( + 'folder access', 'Who can use the folder (grant or remove with -a)') +folder_access.add_argument( + '-a', '--action', dest='action', choices=['grant', 'remove'], + default='grant', action='store', + help='folder access action for -e users/teams: grant (default) or remove') +folder_access.add_argument( + '-e', '--email', dest='user', action='append', + help='account email, team, @existing for all users and teams in the folder, ' + 'or \'*\' as default user permission') +folder_access.add_argument( + '-p', '--manage-records', dest='manage_records', action='store', + choices=['on', 'off'], help='account permission: can manage records. Requires -e.') +folder_access.add_argument( + '-o', '--manage-users', dest='manage_users', action='store', + choices=['on', 'off'], help='account permission: can manage users. Requires -e.') +expiration = folder_access.add_mutually_exclusive_group() +expiration.add_argument( + '--expire-at', dest='expire_at', action='store', metavar='TIMESTAMP', + help='folder access expiration for -e: never or ISO datetime (yyyy-MM-dd[ hh:mm:ss]). Requires -e.') +expiration.add_argument( + '--expire-in', dest='expire_in', action='store', metavar='PERIOD', + help='folder access expiration for -e: never or period ' + '([(y)ears|(mo)nths|(d)ays|(h)ours|(mi)nutes]). Requires -e.') +folder_access.add_argument( + '-roe', '--rotate-on-expiration', dest='rotate_on_expiration', action='store_true', + help='rotate the password when folder access expires. Grant only; requires a positive ' + '--expire-at/--expire-in (not "never") and a pamUser record with rotation configured. ' + 'Requires -a grant, -e, and --expire-at or --expire-in.') +folder_access.add_argument( + '-f', '--force', dest='force', action='store_true', + help='skip confirmation prompts') + +record_access = share_folder_parser.add_argument_group( + 'record permissions', 'Can edit and can share for records in the folder') +record_access.add_argument( + '-r', '--record', dest='record', action='append', + help='record name or UID, @existing for all records in the folder, ' + 'or \'*\' as default record permission') +record_access.add_argument( + '-s', '--can-share', dest='can_share', action='store', + choices=['on', 'off'], help='record permission: can be shared. Requires -r.') +record_access.add_argument( + '-d', '--can-edit', dest='can_edit', action='store', + choices=['on', 'off'], help='record permission: can be modified. Requires -r.') + +share_folder_parser.add_argument( + 'folder', nargs='+', type=str, action='store', help='shared folder path or UID') share_report_parser = argparse.ArgumentParser(prog='share-report', description='Generates a report of shared records', parents=[base.report_output_parser]) @@ -247,7 +265,6 @@ def get_share_expiration(expire_at, expire_in, cmd_name='share-record'): # ( raise CommandError( cmd_name, 'Share expiration must be at least 1 minute.', - ) dt = datetime.datetime.now() + td if dt is None: @@ -259,6 +276,18 @@ def get_share_expiration(expire_at, expire_in, cmd_name='share-record'): # ( return expiration_seconds +def _as_append_list(value): + # type: (Any) -> List[Any] + if not value: + return [] + return value if isinstance(value, list) else [value] + + +def _folder_has_record_permission_target(record_uids, default_record, all_records): + # type: (Set[str], bool, bool) -> bool + return bool(record_uids or default_record or all_records) + + def format_share_expiration_ms(expiration_ms): # type: (int) -> str """Format a share expiration timestamp (milliseconds) for log output.""" @@ -295,6 +324,39 @@ def _record_share_expiration_lookup(rq): return lookup +def _record_share_log_title(params, record_uid): + # type: (KeeperParams, str) -> str + if record_uid in params.record_cache: + return api.get_record(params, record_uid).title + return record_uid + + +def _is_shared_folder_owner(params, shared_folder): + # type: (KeeperParams, dict) -> bool + owner_uid = shared_folder.get('owner_account_uid') + if owner_uid: + return owner_uid == utils.base64_url_encode(params.account_uid_bytes) + owner_username = shared_folder.get('owner_username') + if owner_username and params.user: + return owner_username.lower() == params.user.lower() + return False + + +def _folder_has_full_manager_excluding(shared_folder, exclude_usernames=()): + # type: (dict, Iterable[str]) -> bool + exclude = {x.lower() for x in exclude_usernames if x} + for user in shared_folder.get('users', []): + username = (user.get('username') or '').lower() + if username in exclude: + continue + if user.get('manage_records') and user.get('manage_users'): + return True + for team in shared_folder.get('teams', []): + if team.get('manage_records') and team.get('manage_users'): + return True + return False + + class ShareFolderCommand(Command): def get_parser(self): return share_folder_parser @@ -388,7 +450,7 @@ def get_share_admin_obj_uids(obj_names, obj_type): all_users = False default_account = False if 'user' in kwargs: - for u in (kwargs.get('user') or []): + for u in _as_append_list(kwargs.get('user')): if u == '*': default_account = True elif u in ('@existing', '@current'): @@ -419,8 +481,7 @@ def get_share_admin_obj_uids(obj_names, obj_type): default_record = False unresolved_names = [] if 'record' in kwargs: - records = kwargs.get('record') or [] - for r in records: + for r in _as_append_list(kwargs.get('record')): if r == '*': default_record = True elif r in ('@existing', '@current'): @@ -433,12 +494,22 @@ def get_share_admin_obj_uids(obj_names, obj_type): sa_record_uids = get_share_admin_obj_uids(unresolved_names, record_pb2.CHECK_SA_ON_RECORD) record_uids.update(sa_record_uids or {}) + ShareFolderCommand._validate_share_folder_kwargs( + action, kwargs, + record_uids=record_uids, + default_record=default_record, + all_records=all_records) + if len(as_users) == 0 and len(as_teams) == 0 and len(record_uids) == 0 and \ not default_record and not default_account and \ not all_users and not all_records: logging.info('Nothing to do') return + if action == 'remove' and as_users: + ShareFolderCommand._confirm_folder_user_removals( + params, shared_folder_uids, as_users, force=kwargs.get('force') is True) + rq_groups = [] def prep_rq(recs, users, curr_sf): @@ -465,19 +536,20 @@ def prep_rq(recs, users, curr_sf): else: sh_fol = { 'shared_folder_uid': sf_uid, - 'users': [{'username': x, 'manage_records': action != 'grant', 'manage_users': action != 'grant'} + 'users': [{'username': x, 'manage_records': False, 'manage_users': False} for x in as_users], - 'teams': [{'team_uid': x, 'manage_records': action != 'grant', 'manage_users': action != 'grant'} + 'teams': [{'team_uid': x, 'manage_records': False, 'manage_users': False} for x in as_teams], - 'records': [{'record_uid': x, 'can_share': action != 'grant', 'can_edit': action != 'grant'} - for x in record_uids] } + if record_uids: + sh_fol['records'] = [ + {'record_uid': x, 'can_share': False, 'can_edit': False} for x in record_uids] chunk_size = 500 rec_list = list(sf_records) user_list = list(sf_users) - num_rec_chunks = math.ceil(len(sf_records) / chunk_size) - num_user_chunks = math.ceil(len(sf_users) / chunk_size) - num_rq_groups = num_user_chunks or 1 * num_rec_chunks or 1 + num_rec_chunks = math.ceil(len(sf_records) / chunk_size) if sf_records else 0 + num_user_chunks = math.ceil(len(sf_users) / chunk_size) if sf_users else 0 + num_rq_groups = (num_rec_chunks or 1) * (num_user_chunks or 1) while len(rq_groups) < num_rq_groups: rq_groups.append([]) rec_chunks = [rec_list[i * chunk_size:(i + 1) * chunk_size] for i in range(num_rec_chunks)] or [[]] @@ -492,46 +564,49 @@ def prep_rq(recs, users, curr_sf): group_idx += 1 self.send_requests(params, rq_groups) - if record_uids and as_users and action == 'remove': - record_share_requests = [] - for sf_uid in shared_folder_uids: - sh_fol = params.shared_folder_cache.get(sf_uid, {'shared_folder_uid': sf_uid}) - rs_rqs = ShareFolderCommand.prepare_record_share_request( - params, kwargs, sf_uid, list(as_users), list(record_uids), - curr_sf=sh_fol) - if rs_rqs: - record_share_requests.extend(rs_rqs) - if record_share_requests: - ShareRecordCommand.send_requests( - params, record_share_requests, share_folder_access_log=action) + @staticmethod + def _validate_share_folder_kwargs(action, kwargs, *, record_uids, default_record, all_records): + has_record_target = _folder_has_record_permission_target(record_uids, default_record, all_records) + has_record_perms = kwargs.get('can_edit') is not None or kwargs.get('can_share') is not None + + if has_record_perms and not has_record_target: + raise CommandError( + 'share-folder', + '-d and -s require a record target: -r , -r *, or -r @existing.') @staticmethod - def prepare_record_share_request(params, kwargs, shared_folder_uid, users, rec_uids, *, - curr_sf): - """Build RecordShareUpdateRequest(s) to revoke per-user record access on remove. - - --expire-in/--expire-at apply to folder user access only (via - shared_folder_update_v3). Records in the folder inherit folder timing; -r sets - record permissions only (can_edit/can_share) on SharedFolderUpdateRecord. - - Remove with user and record: removeSharedRecord for that user only; does not - remove the record from the folder or the owner's vault. - """ - if not rec_uids or not users: - return None - if (kwargs.get('action') or 'grant') != 'remove': - return None - sf_uid_bytes = utils.base64_url_decode(shared_folder_uid) + def _confirm_folder_user_removals(params, shared_folder_uids, users_to_remove, *, force=False): + # type: (KeeperParams, Set[str], Set[str], bool) -> None + current_user = (params.user or '').lower() + if not current_user or current_user not in {u.lower() for u in users_to_remove}: + return - rq_remove = record_pb2.RecordShareUpdateRequest() - for record_uid in rec_uids: - for email in users: - shared_record = record_pb2.SharedRecord() - shared_record.toUsername = email - shared_record.recordUid = utils.base64_url_decode(record_uid) - shared_record.sharedFolderUid = sf_uid_bytes - rq_remove.removeSharedRecord.append(shared_record) - return [rq_remove] if rq_remove.removeSharedRecord else None + removing_self_from_shared_folder = False + for sf_uid in shared_folder_uids: + sh_fol = params.shared_folder_cache.get(sf_uid, {}) + if _is_shared_folder_owner(params, sh_fol): + if not _folder_has_full_manager_excluding(sh_fol, [params.user]): + raise CommandError( + 'share-folder', + 'Cannot remove yourself from this shared folder: no other participant has ' + 'manage users and manage records permission.') + if not force: + answer = user_choice( + 'Removing yourself will relinquish folder ownership. ' + 'Another participant can manage users and records. Proceed?', + 'yn', 'n') + if answer.lower() not in ('y', 'yes'): + raise CommandError('share-folder', 'Operation cancelled.') + else: + removing_self_from_shared_folder = True + + if removing_self_from_shared_folder and not force: + answer = user_choice( + 'Are you sure that you want to delete yourself from this shared folder? ' + 'You will not be able to add yourself back into the shared folder after removal.', + 'yn', 'n') + if answer.lower() not in ('y', 'yes'): + raise CommandError('share-folder', 'Operation cancelled.') @staticmethod def prepare_request(params, kwargs, curr_sf, users, teams, rec_uids, *, @@ -546,7 +621,6 @@ def prepare_request(params, kwargs, curr_sf, users, teams, rec_uids, *, action = kwargs.get('action') or 'grant' mr = kwargs.get('manage_records') mu = kwargs.get('manage_users') - skip_folder_record_remove = action == 'remove' and bool(rec_uids) and bool(users) def apply_share_expiration(target): """Set expiration / timer / rotateOnExpiration on a User/Team share update proto.""" @@ -668,7 +742,7 @@ def apply_share_expiration(target): ce = kwargs.get('can_edit') cs = kwargs.get('can_share') - if default_record and action == 'grant': + if default_record: rq.defaultCanEdit = folder_pb2.BOOLEAN_NO_CHANGE if ce is None else folder_pb2.BOOLEAN_TRUE if ce == 'on' else folder_pb2.BOOLEAN_FALSE rq.defaultCanShare = folder_pb2.BOOLEAN_NO_CHANGE if cs is None else folder_pb2.BOOLEAN_TRUE if cs == 'on' else folder_pb2.BOOLEAN_FALSE @@ -679,27 +753,23 @@ def apply_share_expiration(target): folder_record_update.recordUid = utils.base64_url_decode(record_uid) if record_uid in existing_records: - if action == 'grant': - folder_record_update.canEdit = folder_pb2.BOOLEAN_NO_CHANGE if ce is None else folder_pb2.BOOLEAN_TRUE if ce == 'on' else folder_pb2.BOOLEAN_FALSE - folder_record_update.canShare = folder_pb2.BOOLEAN_NO_CHANGE if cs is None else folder_pb2.BOOLEAN_TRUE if cs == 'on' else folder_pb2.BOOLEAN_FALSE - rq.sharedFolderUpdateRecord.append(folder_record_update) - elif action == 'remove' and not skip_folder_record_remove: - rq.sharedFolderRemoveRecord.append(folder_record_update.recordUid) + folder_record_update.canEdit = folder_pb2.BOOLEAN_NO_CHANGE if ce is None else folder_pb2.BOOLEAN_TRUE if ce == 'on' else folder_pb2.BOOLEAN_FALSE + folder_record_update.canShare = folder_pb2.BOOLEAN_NO_CHANGE if cs is None else folder_pb2.BOOLEAN_TRUE if cs == 'on' else folder_pb2.BOOLEAN_FALSE + rq.sharedFolderUpdateRecord.append(folder_record_update) else: - if action == 'grant': - default_ce = folder_pb2.BOOLEAN_TRUE if curr_sf.get('default_can_edit') is True else folder_pb2.BOOLEAN_FALSE - default_cs = folder_pb2.BOOLEAN_TRUE if curr_sf.get('default_can_share') is True else folder_pb2.BOOLEAN_FALSE - folder_record_update.canEdit = default_ce if ce is None else folder_pb2.BOOLEAN_TRUE if ce == 'on' else folder_pb2.BOOLEAN_FALSE - folder_record_update.canShare = default_cs if cs is None else folder_pb2.BOOLEAN_TRUE if cs == 'on' else folder_pb2.BOOLEAN_FALSE - sf_key = curr_sf.get('shared_folder_key_unencrypted') - if sf_key: - rec = params.record_cache[record_uid] - rec_key = rec['record_key_unencrypted'] - if rec.get('version', 0) < 3: - folder_record_update.encryptedRecordKey = crypto.encrypt_aes_v1(rec_key, sf_key) - else: - folder_record_update.encryptedRecordKey = crypto.encrypt_aes_v2(rec_key, sf_key) - rq.sharedFolderAddRecord.append(folder_record_update) + default_ce = folder_pb2.BOOLEAN_TRUE if curr_sf.get('default_can_edit') is True else folder_pb2.BOOLEAN_FALSE + default_cs = folder_pb2.BOOLEAN_TRUE if curr_sf.get('default_can_share') is True else folder_pb2.BOOLEAN_FALSE + folder_record_update.canEdit = default_ce if ce is None else folder_pb2.BOOLEAN_TRUE if ce == 'on' else folder_pb2.BOOLEAN_FALSE + folder_record_update.canShare = default_cs if cs is None else folder_pb2.BOOLEAN_TRUE if cs == 'on' else folder_pb2.BOOLEAN_FALSE + sf_key = curr_sf.get('shared_folder_key_unencrypted') + if sf_key: + rec = params.record_cache[record_uid] + rec_key = rec['record_key_unencrypted'] + if rec.get('version', 0) < 3: + folder_record_update.encryptedRecordKey = crypto.encrypt_aes_v1(rec_key, sf_key) + else: + folder_record_update.encryptedRecordKey = crypto.encrypt_aes_v2(rec_key, sf_key) + rq.sharedFolderAddRecord.append(folder_record_update) return rq @staticmethod @@ -773,21 +843,12 @@ def send_requests(params, partitioned_requests): for r in statuses: record_uid = utils.base64_url_encode(r.recordUid) status = r.status - if record_uid in params.record_cache: - rec = api.get_record(params, record_uid) - title = rec.title - else: - title = record_uid + title = _record_share_log_title(params, record_uid) if status == 'success': - if user_exp and attr in ( - 'sharedFolderAddRecordStatus', - 'sharedFolderUpdateRecordStatus'): - pass - else: - logging.info('Record share \'%s\' %s', title, - 'added' if attr == 'sharedFolderAddRecordStatus' else - 'updated' if attr == 'sharedFolderUpdateRecordStatus' else - 'removed') + logging.info('Record share \'%s\' %s', title, + 'added' if attr == 'sharedFolderAddRecordStatus' else + 'updated' if attr == 'sharedFolderUpdateRecordStatus' else + 'removed') else: logging.warning('Record share \'%s\' failed', title) except KeeperApiError as kae: @@ -1165,7 +1226,7 @@ def get_contact(user, contacts): rq and ShareRecordCommand.send_requests(params, [rq]) @staticmethod - def send_requests(params, requests, share_folder_access_log=None): + def send_requests(params, requests): requests = iter(requests) rq = next(requests, None) while rq and (len(rq.addSharedRecord) > 0 or len(rq.updateSharedRecord) > 0 or len(rq.removeSharedRecord) > 0): @@ -1196,38 +1257,23 @@ def send_requests(params, requests, share_folder_access_log=None): record_uid = utils.base64_url_encode(status_rs.recordUid) status = status_rs.status email = status_rs.username - if status == 'success': - if share_folder_access_log == 'grant': - if attr in ('addSharedRecordStatus', 'updateSharedRecordStatus'): - exp_text = format_share_expiration_ms( - record_exp.get((record_uid, email.lower()), 0)) - if exp_text: - logging.info( - 'Record access granted to user \'%s\' for record \'%s\', ' - 'expires %s', email, record_uid, exp_text) - else: - logging.info( - 'Record access granted to user \'%s\' for record \'%s\'', - email, record_uid) - elif share_folder_access_log == 'remove': - if attr == 'removeSharedRecordStatus': - logging.info( - 'Record access removed from user \'%s\' for record \'%s\'', - email, record_uid) - elif not share_folder_access_log: - verb = ('granted to' if attr == 'addSharedRecordStatus' - else 'changed for' if attr == 'updateSharedRecordStatus' - else 'revoked from') - exp_text = format_share_expiration_ms( - record_exp.get((record_uid, email.lower()), 0)) - exp_suffix = f', record access expires {exp_text}' if exp_text else '' - logging.info( - 'Record \"%s\" access permissions has been %s user \'%s\'%s', - record_uid, verb, email, exp_suffix) - else: - verb = 'grant' if attr == 'addSharedRecordStatus' else 'change' if attr == 'updateSharedRecordStatus' else 'revoke' - - logging.info('Failed to %s record \"%s\" access permissions for user \'%s\': %s', verb, record_uid, email, status_rs.message) + if status != 'success': + verb = ('grant' if attr == 'addSharedRecordStatus' + else 'change' if attr == 'updateSharedRecordStatus' + else 'revoke') + logging.info( + 'Failed to %s record \"%s\" access permissions for user \'%s\': %s', + verb, record_uid, email, status_rs.message) + continue + verb = ('granted to' if attr == 'addSharedRecordStatus' + else 'changed for' if attr == 'updateSharedRecordStatus' + else 'revoked from') + exp_text = format_share_expiration_ms( + record_exp.get((record_uid, email.lower()), 0)) + exp_suffix = f', record access expires {exp_text}' if exp_text else '' + logging.info( + 'Record \"%s\" access permissions has been %s user \'%s\'%s', + record_uid, verb, email, exp_suffix) rq = next(requests, None) diff --git a/unit-tests/test_command_register.py b/unit-tests/test_command_register.py index 29f81cfe2..aa80f31d2 100644 --- a/unit-tests/test_command_register.py +++ b/unit-tests/test_command_register.py @@ -305,7 +305,7 @@ def test_share_folder(self): self.assertEqual(len(TestRegister.expected_commands), 0) TestRegister.expected_commands.extend(['shared_folder_update_v3']) - cmd.execute(params, action='revoke', user=['user2@keepersecurity.com'], folder=shared_folder_uid) + cmd.execute(params, action='remove', user=['user2@keepersecurity.com'], folder=shared_folder_uid) self.assertEqual(len(TestRegister.expected_commands), 0) def test_share_folder_prepare_request_sets_rotate_on_expiration(self): @@ -349,7 +349,7 @@ def test_share_folder_prepare_request_sets_rotate_on_expiration(self): self.assertFalse(record_msgs, 'record protos must not carry folder-wide expiration') - def test_share_folder_prepare_request_sets_folder_and_record_expiration_when_records_specified(self): + def test_share_folder_prepare_request_expire_in_applies_to_folder_user_only(self): """With -r and --expire-in, only folder user gets a timer; record protos are permissions-only.""" params = get_synced_params() shared_folder_uid = next(iter(params.shared_folder_cache.keys())) @@ -388,29 +388,67 @@ def test_share_folder_prepare_request_sets_folder_and_record_expiration_when_rec self.assertEqual(m.expiration, 0) self.assertFalse(m.rotateOnExpiration) - def test_share_folder_prepare_record_share_request_ignores_grant_expiration(self): - """Grant with --expire-in does not create per-record share timers; folder timing only.""" + def test_share_folder_remove_with_record_permissions_combined(self): + """-a remove affects -e only; -r/-d/-s update record permissions in the same request.""" params = get_synced_params() shared_folder_uid = next(iter(params.shared_folder_cache.keys())) - record_uid = next(iter([x['record_uid'] for x in params.meta_data_cache.values() if x['can_share']])) - curr_sf = dict(params.shared_folder_cache[shared_folder_uid]) + record_uid = next(iter(params.shared_folder_cache[shared_folder_uid]['records']))['record_uid'] - params.key_cache['user2@keepersecurity.com'] = mock.MagicMock( - rsa=utils.base64_url_decode(vault_env.encoded_public_key), ec=None) + curr_sf = dict(params.shared_folder_cache[shared_folder_uid]) + curr_sf['users'] = [{ + 'username': 'user2@keepersecurity.com', + 'manage_records': True, + 'manage_users': True, + }] + curr_sf['records'] = [{'record_uid': record_uid, 'can_edit': True, 'can_share': True}] - rq = register.ShareFolderCommand.prepare_record_share_request( + rq = register.ShareFolderCommand.prepare_request( params, - kwargs={'action': 'grant', 'can_edit': 'on', 'can_share': 'on'}, - shared_folder_uid=shared_folder_uid, + kwargs={'action': 'remove', 'can_edit': 'off'}, + curr_sf=curr_sf, users=['user2@keepersecurity.com'], + teams=[], rec_uids=[record_uid], - curr_sf=curr_sf, ) - self.assertIsNone(rq) + self.assertEqual(list(rq.sharedFolderRemoveUser), ['user2@keepersecurity.com']) + self.assertTrue(list(rq.sharedFolderUpdateRecord)) + self.assertFalse(list(rq.sharedFolderRemoveRecord)) + self.assertFalse(list(rq.sharedFolderAddRecord)) + + def test_share_folder_remove_rejects_record_permissions_without_record_target(self): + params = get_synced_params() + shared_folder_uid = next(iter(params.shared_folder_cache.keys())) + cmd = register.ShareFolderCommand() - def test_share_folder_prepare_request_remove_user_record_keeps_record_in_folder(self): - """Removing user access to one record removes the user from the folder but not the record.""" + with self.assertRaises(CommandError) as ctx: + cmd.execute( + params, + action='remove', + user=['user2@keepersecurity.com'], + folder=shared_folder_uid, + can_edit='on', + force=True, + ) + self.assertIn('-d and -s require a record target', str(ctx.exception)) + + def test_share_folder_grant_rejects_record_permissions_without_record_target(self): + params = get_synced_params() + shared_folder_uid = next(iter(params.shared_folder_cache.keys())) + cmd = register.ShareFolderCommand() + + with self.assertRaises(CommandError) as ctx: + cmd.execute( + params, + action='grant', + folder=shared_folder_uid, + can_edit='on', + force=True, + ) + self.assertIn('-d and -s require a record target', str(ctx.exception)) + + def test_share_folder_prepare_request_remove_user_only(self): + """Remove with -e revokes folder access only; no record protos are sent.""" params = get_synced_params() shared_folder_uid = next(iter(params.shared_folder_cache.keys())) record_uid = next(iter(params.shared_folder_cache[shared_folder_uid]['records']))['record_uid'] @@ -428,31 +466,78 @@ def test_share_folder_prepare_request_remove_user_record_keeps_record_in_folder( curr_sf=curr_sf, users=['user2@keepersecurity.com'], teams=[], - rec_uids=[record_uid], + rec_uids=[], ) self.assertTrue(list(rq.sharedFolderRemoveUser)) self.assertFalse(list(rq.sharedFolderRemoveRecord)) + self.assertFalse(list(rq.sharedFolderUpdateRecord)) + self.assertFalse(list(rq.sharedFolderAddRecord)) - def test_share_folder_prepare_record_share_request_remove_user_record(self): + def test_share_folder_owner_self_remove_blocked_without_backup_manager(self): params = get_synced_params() shared_folder_uid = next(iter(params.shared_folder_cache.keys())) - record_uid = next(iter(params.shared_folder_cache[shared_folder_uid]['records']))['record_uid'] - curr_sf = dict(params.shared_folder_cache[shared_folder_uid]) + params.shared_folder_cache[shared_folder_uid]['owner_account_uid'] = utils.base64_url_encode( + params.account_uid_bytes) + params.shared_folder_cache[shared_folder_uid]['users'] = [{ + 'username': params.user, + 'manage_records': True, + 'manage_users': True, + }] + params.shared_folder_cache[shared_folder_uid]['teams'] = [] + cmd = register.ShareFolderCommand() - rq_list = register.ShareFolderCommand.prepare_record_share_request( - params, - kwargs={'action': 'remove'}, - shared_folder_uid=shared_folder_uid, - users=['user2@keepersecurity.com'], - rec_uids=[record_uid], - curr_sf=curr_sf, - ) + with self.assertRaises(CommandError) as ctx: + cmd.execute( + params, + action='remove', + user=[params.user], + folder=shared_folder_uid, + force=True, + ) + self.assertIn('no other participant has', str(ctx.exception)) + + def test_share_folder_participant_self_remove_prompt_declined(self): + params = get_synced_params() + shared_folder_uid = next(iter(params.shared_folder_cache.keys())) + params.shared_folder_cache[shared_folder_uid]['owner_account_uid'] = utils.generate_uid() + params.shared_folder_cache[shared_folder_uid]['users'] = [ + {'username': params.user, 'manage_records': False, 'manage_users': False}, + {'username': 'user2@keepersecurity.com', 'manage_records': True, 'manage_users': True}, + ] + params.shared_folder_cache[shared_folder_uid]['teams'] = [] + cmd = register.ShareFolderCommand() - self.assertIsNotNone(rq_list) - self.assertEqual(len(rq_list), 1) - self.assertTrue(rq_list[0].removeSharedRecord) - self.assertFalse(rq_list[0].addSharedRecord) + with mock.patch('keepercommander.commands.register.user_choice', return_value='n'): + with self.assertRaises(CommandError) as ctx: + cmd.execute( + params, + action='remove', + user=[params.user], + folder=shared_folder_uid, + ) + self.assertIn('Operation cancelled', str(ctx.exception)) + + def test_share_folder_participant_self_remove_prompt_accepted(self): + params = get_synced_params() + shared_folder_uid = next(iter(params.shared_folder_cache.keys())) + params.shared_folder_cache[shared_folder_uid]['owner_account_uid'] = utils.generate_uid() + params.shared_folder_cache[shared_folder_uid]['users'] = [ + {'username': params.user, 'manage_records': False, 'manage_users': False}, + {'username': 'user2@keepersecurity.com', 'manage_records': True, 'manage_users': True}, + ] + params.shared_folder_cache[shared_folder_uid]['teams'] = [] + cmd = register.ShareFolderCommand() + TestRegister.expected_commands.extend(['shared_folder_update_v3']) + + with mock.patch('keepercommander.commands.register.user_choice', return_value='y'): + cmd.execute( + params, + action='remove', + user=[params.user], + folder=shared_folder_uid, + ) + self.assertEqual(len(TestRegister.expected_commands), 0) def test_share_folder_prepare_request_skips_redundant_user_update_for_record_only(self): """When sharing another record without expiration, skip redundant folder user update.""" From 5f5e0a48b0007158fd2b2f36b26ed25986f69ee3 Mon Sep 17 00:00:00 2001 From: amangalampalli-ks Date: Fri, 3 Jul 2026 10:48:55 +0530 Subject: [PATCH 7/9] Fix -p and -o flags and 1mi expiry --- .../commands/nested_share_folder/helpers.py | 7 ++- keepercommander/commands/register.py | 47 +++++++++------ unit-tests/test_command_register.py | 58 ++++++++++++++++++- 3 files changed, 88 insertions(+), 24 deletions(-) diff --git a/keepercommander/commands/nested_share_folder/helpers.py b/keepercommander/commands/nested_share_folder/helpers.py index 5c990d04a..57ab381e5 100644 --- a/keepercommander/commands/nested_share_folder/helpers.py +++ b/keepercommander/commands/nested_share_folder/helpers.py @@ -309,12 +309,13 @@ def walk(fuid): # Expiration parsing # ═══════════════════════════════════════════════════════════════════════════ -def validate_share_expiration_timestamp(expiration_ms, cmd_name): +def validate_share_expiration_timestamp(expiration_ms, cmd_name, *, now_ms=None): """Reject finite expirations that are less than one minute.""" if expiration_ms is None or expiration_ms == -1: return - min_allowed = int(datetime.datetime.now(timezone.utc).timestamp() * 1000) + MIN_SHARE_EXPIRATION_MS - if expiration_ms < min_allowed: + if now_ms is None: + now_ms = int(datetime.datetime.now(timezone.utc).timestamp() * 1000) + if expiration_ms < now_ms + MIN_SHARE_EXPIRATION_MS: raise CommandError( cmd_name, 'Share expiration must be at least 1 minute.', diff --git a/keepercommander/commands/register.py b/keepercommander/commands/register.py index 17dfbe714..430cfcd3c 100644 --- a/keepercommander/commands/register.py +++ b/keepercommander/commands/register.py @@ -253,6 +253,7 @@ def get_share_expiration(expire_at, expire_in, cmd_name='share-record'): # ( return dt = None # type: Optional[datetime.datetime] + now_ms = None # type: Optional[int] if isinstance(expire_at, str): if expire_at == 'never': return -1 @@ -266,14 +267,16 @@ def get_share_expiration(expire_at, expire_in, cmd_name='share-record'): # ( cmd_name, 'Share expiration must be at least 1 minute.', ) - dt = datetime.datetime.now() + td + now_utc = datetime.datetime.now(datetime.timezone.utc) + now_ms = int(now_utc.timestamp() * 1000) + dt = now_utc + td if dt is None: raise ValueError(f'Incorrect expiration: {expire_at or expire_in}') - expiration_seconds = int(dt.timestamp()) + expiration_ms = int(dt.timestamp() * 1000) from .nested_share_folder.helpers import validate_share_expiration_timestamp - validate_share_expiration_timestamp(expiration_seconds * 1000, cmd_name) - return expiration_seconds + validate_share_expiration_timestamp(expiration_ms, cmd_name, now_ms=now_ms) + return expiration_ms // 1000 def _as_append_list(value): @@ -288,6 +291,16 @@ def _folder_has_record_permission_target(record_uids, default_record, all_record return bool(record_uids or default_record or all_records) +def _folder_user_lookup(shared_folder, email): + # type: (dict, str) -> Optional[dict] + """Find a folder user entry by email (case-insensitive).""" + email_lower = email.lower() + for user in shared_folder.get('users', []): + if user.get('username', '').lower() == email_lower: + return user + return None + + def format_share_expiration_ms(expiration_ms): # type: (int) -> str """Format a share expiration timestamp (milliseconds) for log output.""" @@ -559,7 +572,7 @@ def prep_rq(recs, users, curr_sf): for u_chunk in user_chunks: sf_info = sh_fol.copy() if group_idx: - del sf_info['revision'] + sf_info.pop('revision', None) rq_groups[group_idx].append(prep_rq(r_chunk, u_chunk, sf_info)) group_idx += 1 self.send_requests(params, rq_groups) @@ -645,23 +658,20 @@ def apply_share_expiration(target): rq.defaultManageUsers = folder_pb2.BOOLEAN_NO_CHANGE if len(users) > 0: - existing_users = {x['username'] for x in curr_sf.get('users', [])} for email in users: + current_user = _folder_user_lookup(curr_sf, email) + if current_user: + email = current_user['username'] uo = folder_pb2.SharedFolderUpdateUser() uo.username = email apply_share_expiration(uo) - if email in existing_users: + if current_user: if action == 'grant': - if rec_uids: - current_user = next( - (x for x in curr_sf.get('users', []) if x['username'] == email), None) - if current_user: - mr_unchanged = (mr is None - or (current_user.get('manage_records') is True) == (mr == 'on')) - mu_unchanged = (mu is None - or (current_user.get('manage_users') is True) == (mu == 'on')) - if mr_unchanged and mu_unchanged and not isinstance(share_expiration, int): - continue + if rec_uids and mr is None and mu is None: + mr_unchanged = (current_user.get('manage_records') is True) + mu_unchanged = (current_user.get('manage_users') is True) + if mr_unchanged and mu_unchanged and not isinstance(share_expiration, int): + continue uo.manageRecords = folder_pb2.BOOLEAN_NO_CHANGE if mr is None else folder_pb2.BOOLEAN_TRUE if mr == 'on' else folder_pb2.BOOLEAN_FALSE uo.manageUsers = folder_pb2.BOOLEAN_NO_CHANGE if mu is None else folder_pb2.BOOLEAN_TRUE if mu == 'on' else folder_pb2.BOOLEAN_FALSE rq.sharedFolderUpdateUser.append(uo) @@ -834,7 +844,8 @@ def send_requests(params, partitioned_requests): elif status == 'invited': logging.info('User \'%s\' invited', username) else: - logging.warning('User share \'%s\' failed', username) + logging.warning( + 'User share \'%s\' failed: %s', username, status) for attr in ('sharedFolderAddRecordStatus', 'sharedFolderUpdateRecordStatus', 'sharedFolderRemoveRecordStatus'): diff --git a/unit-tests/test_command_register.py b/unit-tests/test_command_register.py index aa80f31d2..4b91cb112 100644 --- a/unit-tests/test_command_register.py +++ b/unit-tests/test_command_register.py @@ -6,7 +6,7 @@ from data_vault import get_synced_params, VaultEnvironment from keepercommander.commands import register from keepercommander.error import CommandError -from keepercommander.proto import APIRequest_pb2, record_pb2 +from keepercommander.proto import APIRequest_pb2, folder_pb2, record_pb2 from keepercommander import utils from keepercommander.subfolder import NestedShareFolderNode @@ -555,8 +555,7 @@ def test_share_folder_prepare_request_skips_redundant_user_update_for_record_onl rq = register.ShareFolderCommand.prepare_request( params, - kwargs={'action': 'grant', 'manage_records': 'on', 'manage_users': 'on', - 'can_edit': 'on', 'can_share': 'on'}, + kwargs={'action': 'grant', 'can_edit': 'on', 'can_share': 'on'}, curr_sf=curr_sf, users=['user2@keepersecurity.com'], teams=[], @@ -569,6 +568,59 @@ def test_share_folder_prepare_request_skips_redundant_user_update_for_record_onl 'new record grant should update folder record permissions via add') self.assertFalse(list(rq.sharedFolderUpdateRecord)) + def test_share_folder_prepare_request_updates_user_when_explicit_perms_with_record(self): + """-p/-o with -r must always update folder user permissions.""" + params = get_synced_params() + shared_folder_uid = next(iter(params.shared_folder_cache.keys())) + record_uid = next(iter([x['record_uid'] for x in params.meta_data_cache.values() if x['can_share']])) + + curr_sf = dict(params.shared_folder_cache[shared_folder_uid]) + curr_sf['users'] = [{ + 'username': 'user2@keepersecurity.com', + 'manage_records': True, + 'manage_users': True, + }] + curr_sf.setdefault('records', [{'record_uid': record_uid, 'can_edit': True, 'can_share': True}]) + + rq = register.ShareFolderCommand.prepare_request( + params, + kwargs={'action': 'grant', 'manage_records': 'on', 'manage_users': 'on', + 'can_edit': 'on', 'can_share': 'on'}, + curr_sf=curr_sf, + users=['user2@keepersecurity.com'], + teams=[], + rec_uids=[record_uid], + share_expiration=None, + ) + + user_msgs = list(rq.sharedFolderUpdateUser) + self.assertTrue(user_msgs, 'explicit -p/-o must update folder user even with -r') + self.assertEqual(user_msgs[0].manageRecords, folder_pb2.BOOLEAN_TRUE) + self.assertEqual(user_msgs[0].manageUsers, folder_pb2.BOOLEAN_TRUE) + + def test_share_folder_prepare_request_case_insensitive_user_lookup(self): + params = get_synced_params() + shared_folder_uid = next(iter(params.shared_folder_cache.keys())) + curr_sf = dict(params.shared_folder_cache[shared_folder_uid]) + curr_sf['users'] = [{ + 'username': 'User2@KeeperSecurity.com', + 'manage_records': False, + 'manage_users': False, + }] + + rq = register.ShareFolderCommand.prepare_request( + params, + kwargs={'action': 'grant', 'manage_records': 'on', 'manage_users': 'on'}, + curr_sf=curr_sf, + users=['user2@keepersecurity.com'], + teams=[], + rec_uids=[], + ) + + user_msgs = list(rq.sharedFolderUpdateUser) + self.assertEqual(len(user_msgs), 1) + self.assertEqual(user_msgs[0].username, 'User2@KeeperSecurity.com') + def test_share_folder_prepare_request_updates_user_when_folder_wide_expiration(self): """Folder-wide --expire-in (no -r) sets expiration on the folder user share.""" params = get_synced_params() From e2db6754f29dd98fd50c119c798e9c91b8559911 Mon Sep 17 00:00:00 2001 From: amangalampalli-ks Date: Fri, 3 Jul 2026 12:53:40 +0530 Subject: [PATCH 8/9] Restrict outside records to be shared via -r --- keepercommander/commands/register.py | 46 +++++++++++++++++++--------- unit-tests/test_command_register.py | 33 ++++++++++++++------ 2 files changed, 56 insertions(+), 23 deletions(-) diff --git a/keepercommander/commands/register.py b/keepercommander/commands/register.py index 430cfcd3c..1fefcdfbd 100644 --- a/keepercommander/commands/register.py +++ b/keepercommander/commands/register.py @@ -134,7 +134,7 @@ def register_command_info(aliases, command_info): 'record permissions', 'Can edit and can share for records in the folder') record_access.add_argument( '-r', '--record', dest='record', action='append', - help='record name or UID, @existing for all records in the folder, ' + help='record name or UID already in the folder, @existing for all records in the folder, ' 'or \'*\' as default record permission') record_access.add_argument( '-s', '--can-share', dest='can_share', action='store', @@ -513,6 +513,10 @@ def get_share_admin_obj_uids(obj_names, obj_type): default_record=default_record, all_records=all_records) + if record_uids and not default_record and not all_records: + ShareFolderCommand._validate_records_in_shared_folders( + params, shared_folder_uids, record_uids) + if len(as_users) == 0 and len(as_teams) == 0 and len(record_uids) == 0 and \ not default_record and not default_account and \ not all_users and not all_records: @@ -587,6 +591,30 @@ def _validate_share_folder_kwargs(action, kwargs, *, record_uids, default_record 'share-folder', '-d and -s require a record target: -r , -r *, or -r @existing.') + @staticmethod + def _validate_records_in_shared_folders(params, shared_folder_uids, record_uids): + # type: (KeeperParams, Set[str], Set[str]) -> None + """Reject -r targets that are not already linked to the shared folder.""" + for sf_uid in shared_folder_uids: + sh_fol = params.shared_folder_cache.get(sf_uid) + if not sh_fol: + raise CommandError( + 'share-folder', + f'Shared folder "{sf_uid}" is not loaded. Sync down and retry.') + folder_record_uids = {x['record_uid'] for x in sh_fol.get('records', [])} + missing = record_uids - folder_record_uids + if not missing: + continue + labels = [] + for uid in sorted(missing): + rec = params.record_cache.get(uid) + title = rec.get('title_unencrypted') if rec else None + labels.append(title or uid) + folder_name = sh_fol.get('name_unencrypted') or sf_uid + raise CommandError( + 'share-folder', + f'Record(s) not in shared folder "{folder_name}": ' + ', '.join(labels)) + @staticmethod def _confirm_folder_user_removals(params, shared_folder_uids, users_to_remove, *, force=False): # type: (KeeperParams, Set[str], Set[str], bool) -> None @@ -767,19 +795,9 @@ def apply_share_expiration(target): folder_record_update.canShare = folder_pb2.BOOLEAN_NO_CHANGE if cs is None else folder_pb2.BOOLEAN_TRUE if cs == 'on' else folder_pb2.BOOLEAN_FALSE rq.sharedFolderUpdateRecord.append(folder_record_update) else: - default_ce = folder_pb2.BOOLEAN_TRUE if curr_sf.get('default_can_edit') is True else folder_pb2.BOOLEAN_FALSE - default_cs = folder_pb2.BOOLEAN_TRUE if curr_sf.get('default_can_share') is True else folder_pb2.BOOLEAN_FALSE - folder_record_update.canEdit = default_ce if ce is None else folder_pb2.BOOLEAN_TRUE if ce == 'on' else folder_pb2.BOOLEAN_FALSE - folder_record_update.canShare = default_cs if cs is None else folder_pb2.BOOLEAN_TRUE if cs == 'on' else folder_pb2.BOOLEAN_FALSE - sf_key = curr_sf.get('shared_folder_key_unencrypted') - if sf_key: - rec = params.record_cache[record_uid] - rec_key = rec['record_key_unencrypted'] - if rec.get('version', 0) < 3: - folder_record_update.encryptedRecordKey = crypto.encrypt_aes_v1(rec_key, sf_key) - else: - folder_record_update.encryptedRecordKey = crypto.encrypt_aes_v2(rec_key, sf_key) - rq.sharedFolderAddRecord.append(folder_record_update) + logging.debug( + 'Record %s is not in shared folder %s; skipping record permission update', + record_uid, curr_sf.get('shared_folder_uid')) return rq @staticmethod diff --git a/unit-tests/test_command_register.py b/unit-tests/test_command_register.py index 4b91cb112..2f35ea315 100644 --- a/unit-tests/test_command_register.py +++ b/unit-tests/test_command_register.py @@ -353,11 +353,10 @@ def test_share_folder_prepare_request_expire_in_applies_to_folder_user_only(self """With -r and --expire-in, only folder user gets a timer; record protos are permissions-only.""" params = get_synced_params() shared_folder_uid = next(iter(params.shared_folder_cache.keys())) - record_uid = next(iter([x['record_uid'] for x in params.meta_data_cache.values() if x['can_share']])) + record_uid = next(iter(params.shared_folder_cache[shared_folder_uid]['records']))['record_uid'] curr_sf = dict(params.shared_folder_cache[shared_folder_uid]) curr_sf.setdefault('users', []) - curr_sf.setdefault('records', [{'record_uid': record_uid, 'can_edit': True, 'can_share': True}]) future_ts = int(datetime.datetime.now().timestamp()) + 86_400 params.key_cache['user2@keepersecurity.com'] = mock.MagicMock( @@ -447,6 +446,25 @@ def test_share_folder_grant_rejects_record_permissions_without_record_target(sel ) self.assertIn('-d and -s require a record target', str(ctx.exception)) + def test_share_folder_rejects_record_not_in_folder(self): + params = get_synced_params() + shared_folder_uid = next(iter(params.shared_folder_cache.keys())) + folder_record_uids = {x['record_uid'] for x in params.shared_folder_cache[shared_folder_uid]['records']} + foreign_record_uid = next( + uid for uid in params.record_cache if uid not in folder_record_uids) + cmd = register.ShareFolderCommand() + + with self.assertRaises(CommandError) as ctx: + cmd.execute( + params, + action='grant', + folder=shared_folder_uid, + record=[foreign_record_uid], + can_edit='on', + force=True, + ) + self.assertIn('not in shared folder', str(ctx.exception)) + def test_share_folder_prepare_request_remove_user_only(self): """Remove with -e revokes folder access only; no record protos are sent.""" params = get_synced_params() @@ -543,7 +561,7 @@ def test_share_folder_prepare_request_skips_redundant_user_update_for_record_onl """When sharing another record without expiration, skip redundant folder user update.""" params = get_synced_params() shared_folder_uid = next(iter(params.shared_folder_cache.keys())) - record_uid = next(iter([x['record_uid'] for x in params.meta_data_cache.values() if x['can_share']])) + record_uid = next(iter(params.shared_folder_cache[shared_folder_uid]['records']))['record_uid'] curr_sf = dict(params.shared_folder_cache[shared_folder_uid]) curr_sf['users'] = [{ @@ -551,7 +569,6 @@ def test_share_folder_prepare_request_skips_redundant_user_update_for_record_onl 'manage_records': True, 'manage_users': True, }] - curr_sf.setdefault('records', [{'record_uid': record_uid, 'can_edit': True, 'can_share': True}]) rq = register.ShareFolderCommand.prepare_request( params, @@ -564,15 +581,14 @@ def test_share_folder_prepare_request_skips_redundant_user_update_for_record_onl ) self.assertFalse(list(rq.sharedFolderUpdateUser)) - self.assertTrue(list(rq.sharedFolderAddRecord), - 'new record grant should update folder record permissions via add') - self.assertFalse(list(rq.sharedFolderUpdateRecord)) + self.assertTrue(list(rq.sharedFolderUpdateRecord)) + self.assertFalse(list(rq.sharedFolderAddRecord)) def test_share_folder_prepare_request_updates_user_when_explicit_perms_with_record(self): """-p/-o with -r must always update folder user permissions.""" params = get_synced_params() shared_folder_uid = next(iter(params.shared_folder_cache.keys())) - record_uid = next(iter([x['record_uid'] for x in params.meta_data_cache.values() if x['can_share']])) + record_uid = next(iter(params.shared_folder_cache[shared_folder_uid]['records']))['record_uid'] curr_sf = dict(params.shared_folder_cache[shared_folder_uid]) curr_sf['users'] = [{ @@ -580,7 +596,6 @@ def test_share_folder_prepare_request_updates_user_when_explicit_perms_with_reco 'manage_records': True, 'manage_users': True, }] - curr_sf.setdefault('records', [{'record_uid': record_uid, 'can_edit': True, 'can_share': True}]) rq = register.ShareFolderCommand.prepare_request( params, From 06632927c01ff6c5b1a2591285951e975b88256e Mon Sep 17 00:00:00 2001 From: amangalampalli-ks Date: Mon, 6 Jul 2026 18:56:18 +0530 Subject: [PATCH 9/9] Update help --- keepercommander/commands/register.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/keepercommander/commands/register.py b/keepercommander/commands/register.py index 1fefcdfbd..b8de28b37 100644 --- a/keepercommander/commands/register.py +++ b/keepercommander/commands/register.py @@ -112,7 +112,8 @@ def register_command_info(aliases, command_info): choices=['on', 'off'], help='account permission: can manage records. Requires -e.') folder_access.add_argument( '-o', '--manage-users', dest='manage_users', action='store', - choices=['on', 'off'], help='account permission: can manage users. Requires -e.') + choices=['on', 'off'], + help='account permission: can manage users. Mutually exclusive with --expire-at/--expire-in. Requires -e.') expiration = folder_access.add_mutually_exclusive_group() expiration.add_argument( '--expire-at', dest='expire_at', action='store', metavar='TIMESTAMP',