diff --git a/keepercommander/commands/nested_share_folder/helpers.py b/keepercommander/commands/nested_share_folder/helpers.py index c6624b137..5c990d04a 100644 --- a/keepercommander/commands/nested_share_folder/helpers.py +++ b/keepercommander/commands/nested_share_folder/helpers.py @@ -74,6 +74,9 @@ re.IGNORECASE, ) +MIN_SHARE_EXPIRATION_MS = 60_000 +"""Minimum share expiration is one minute (milliseconds).""" + # ═══════════════════════════════════════════════════════════════════════════ # Error-handling patterns (eliminates repetitive try/except boilerplate) @@ -306,6 +309,18 @@ def walk(fuid): # Expiration parsing # ═══════════════════════════════════════════════════════════════════════════ +def validate_share_expiration_timestamp(expiration_ms, cmd_name): + """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: + raise CommandError( + cmd_name, + 'Share expiration must be at least 1 minute.', + ) + + def parse_expiration(expire_at, expire_in, cmd_name): """Parse ``--expire-at`` / ``--expire-in`` into a millisecond timestamp. @@ -320,13 +335,15 @@ def parse_expiration(expire_at, expire_in, cmd_name): if expire_at: try: dt = datetime.datetime.fromisoformat(raw.replace('Z', '+00:00')) - return int(dt.timestamp() * 1000) + expiration_ms = int(dt.timestamp() * 1000) except ValueError: raise CommandError( cmd_name, f'Invalid --expire-at format: {raw!r}. ' f'Use ISO datetime, e.g. 2027-01-01T00:00:00Z or "never"', ) + validate_share_expiration_timestamp(expiration_ms, cmd_name) + return expiration_ms m = _EXPIRATION_RE.fullmatch(raw) if not m: @@ -336,6 +353,11 @@ def parse_expiration(expire_at, expire_in, cmd_name): ) amount = int(m.group(1)) unit = m.group(2).lower() + if unit.startswith('mi') and amount < 1: + raise CommandError( + cmd_name, + 'Share expiration must be at least 1 minute.', + ) now = datetime.datetime.now(timezone.utc) delta_map = { 'mi': timedelta(minutes=amount), @@ -345,7 +367,9 @@ def parse_expiration(expire_at, expire_in, cmd_name): 'y': timedelta(days=amount * 365), } delta = next(v for k, v in delta_map.items() if unit.startswith(k)) - return int((now + delta).timestamp() * 1000) + expiration_ms = int((now + delta).timestamp() * 1000) + validate_share_expiration_timestamp(expiration_ms, cmd_name) + return expiration_ms # ═══════════════════════════════════════════════════════════════════════════ diff --git a/keepercommander/commands/nested_share_folder/sharing_commands.py b/keepercommander/commands/nested_share_folder/sharing_commands.py index a8c5a207b..d4029fac5 100644 --- a/keepercommander/commands/nested_share_folder/sharing_commands.py +++ b/keepercommander/commands/nested_share_folder/sharing_commands.py @@ -98,8 +98,23 @@ def _dispatch(params, action, record_uid, email, access_role_type, expiration): params=params, record_uid=record_uid, new_owner_email=email), 'owner') if action == 'grant': - if NestedShareRecordShareCommand._is_already_shared( - params, record_uid, email): + existing = NestedShareRecordShareCommand._get_direct_user_share( + params, record_uid, email) + if existing: + if _nsf.is_record_share_update_noop( + existing, access_role_type, expiration): + logging.info( + "Record '%s' already shared with '%s' at the requested " + "role and expiration; no change needed.", + record_uid, email) + return ({ + 'results': [{ + 'record_uid': record_uid, + 'success': True, + 'message': 'Already has requested access', + }], + 'success': True, + }, 'update') logging.debug( "Record '%s' is already shared with user '%s'; switching to update.", record_uid, email) @@ -115,6 +130,17 @@ def _dispatch(params, action, record_uid, email, access_role_type, expiration): return (_nsf.unshare_record_v3( params=params, record_uid=record_uid, recipient_email=email), 'revoke') + @staticmethod + def _get_direct_user_share(params, record_uid, email): + """Return direct AT_USER share metadata for *email*, or None.""" + try: + access_result = _nsf.get_record_accesses_v3(params, [record_uid]) + return _nsf.find_direct_user_share_access( + access_result, record_uid, email) + except Exception as exc: + logging.debug("Could not fetch record accesses for '%s': %s", record_uid, exc) + return None + @staticmethod def _is_already_shared(params, record_uid, email): """Return True if *email* already has a *direct* non-owner share on *record_uid*. @@ -126,21 +152,8 @@ def _is_already_shared(params, record_uid, email): causes the caller to dispatch ``share_record_v3`` (a fresh direct grant) which correctly overrides the inherited folder permission. """ - try: - access_result = _nsf.get_record_accesses_v3(params, [record_uid]) - for a in access_result.get('record_accesses', []): - if a.get('record_uid') != record_uid or a.get('owner', False): - continue - if a.get('access_type') and a.get('access_type') != 'AT_USER': - continue - if a.get('inherited'): - continue - if a.get('accessor_name', '').casefold() == email.casefold(): - return True - return False - except Exception as exc: - logging.debug("Could not fetch record accesses for '%s': %s", record_uid, exc) - return False + return NestedShareRecordShareCommand._get_direct_user_share( + params, record_uid, email) is not None @staticmethod def _log_results(result, action, email): diff --git a/keepercommander/commands/record.py b/keepercommander/commands/record.py index 7977d0ab5..87639cf0f 100644 --- a/keepercommander/commands/record.py +++ b/keepercommander/commands/record.py @@ -1558,7 +1558,7 @@ def execute(self, params, **kwargs): 'record_type': record.record_type, 'title': record.title, 'description': vault_extensions.get_record_description(record), - 'record_category': 'Nested' if is_nsf else 'Classic', + 'record_category': 'nested' if is_nsf else 'classic', } all_results.append(result_item) else: @@ -1566,7 +1566,7 @@ def execute(self, params, **kwargs): table = [] headers = ['Record UID', 'Type', 'Title', 'Description', 'Record Category'] for record in records: - record_category = 'Nested' if record.record_uid in nsf_records_map else 'Classic' + record_category = 'nested' if record.record_uid in nsf_records_map else 'classic' row = [record.record_uid, record.record_type, record.title, vault_extensions.get_record_description(record), record_category] table.append(row) @@ -1655,7 +1655,7 @@ def execute(self, params, **kwargs): for item in all_results: if item['type'] == 'record': row = [item['type'], item['record_uid'], item['title'], - f"Type: {item['record_type']}, Description: {item['description']}, Record Category: {item.get('record_category', 'Classic')}"] + f"Type: {item['record_type']}, Description: {item['description']}, Record Category: {item.get('record_category', 'classic')}"] elif item['type'] == 'shared_folder': row = [item['type'], item['shared_folder_uid'], item['name'], f"Folder Category: Classic, Can Edit: {item['can_edit']}, Can Share: {item['can_share']}"] @@ -1832,7 +1832,7 @@ def execute(self, params, **kwargs): for record in records: # Determine if record is from Nested Share Folder or Classic is_nested_share = hasattr(params, 'nested_share_records') and record.record_uid in params.nested_share_records - record_category = 'Nested' if is_nested_share else 'Classic' + record_category = 'nested' if is_nested_share else 'classic' row = [record.record_uid, record.record_type, record.title, vault_extensions.get_record_description(record), record.shared, record_category] table.append(row) diff --git a/keepercommander/commands/register.py b/keepercommander/commands/register.py index 858f3e6de..5aac059db 100644 --- a/keepercommander/commands/register.py +++ b/keepercommander/commands/register.py @@ -227,7 +227,7 @@ def register_command_info(aliases, command_info): create_account_parser = argparse.ArgumentParser(prog='create-account', description='Create Keeper Account') create_account_parser.add_argument('email', help='email') -def get_share_expiration(expire_at, expire_in): # (Optional[str], Optional[str]) -> Optional[int] +def get_share_expiration(expire_at, expire_in, cmd_name='share-record'): # (Optional[str], Optional[str], str) -> Optional[int] if not expire_at and not expire_in: return @@ -240,11 +240,20 @@ def get_share_expiration(expire_at, expire_in): # (Optional[str], Optional[s if expire_in == 'never': return -1 td = parse_timeout(expire_in) + if td < datetime.timedelta(minutes=1): + raise CommandError( + cmd_name, + 'Share expiration must be at least 1 minute.', + + ) dt = datetime.datetime.now() + td if dt is None: raise ValueError(f'Incorrect expiration: {expire_at or expire_in}') - return int(dt.timestamp()) + expiration_seconds = int(dt.timestamp()) + from .nested_share_folder.helpers import validate_share_expiration_timestamp + validate_share_expiration_timestamp(expiration_seconds * 1000, cmd_name) + return expiration_seconds class ShareFolderCommand(Command): @@ -310,7 +319,8 @@ def get_share_admin_obj_uids(obj_names, obj_type): share_expiration = None if action == 'grant': - share_expiration = get_share_expiration(kwargs.get('expire_at'), kwargs.get('expire_in')) + share_expiration = get_share_expiration( + kwargs.get('expire_at'), kwargs.get('expire_in'), cmd_name='share-folder') rotate_on_expiration = bool(kwargs.get('rotate_on_expiration')) if rotate_on_expiration: @@ -690,7 +700,8 @@ def prep_request(params, kwargs): # type: (KeeperParams, Dict[str, Any]) -> Un share_expiration = None if action == 'grant': - share_expiration = get_share_expiration(kwargs.get('expire_at'), kwargs.get('expire_in')) + share_expiration = get_share_expiration( + kwargs.get('expire_at'), kwargs.get('expire_in'), cmd_name='share-record') rotate_on_expiration = bool(kwargs.get('rotate_on_expiration')) if rotate_on_expiration: diff --git a/keepercommander/commands/supershell/app.py b/keepercommander/commands/supershell/app.py index 8275f40ab..10e48f3f3 100644 --- a/keepercommander/commands/supershell/app.py +++ b/keepercommander/commands/supershell/app.py @@ -254,8 +254,8 @@ def _get_welcome_screen_content(self) -> str: [bold {t['primary_bright']}]Folder Icons[/bold {t['primary_bright']}] [{t['text_dim']}]•[/{t['text_dim']}] Legacy Personal Folder 🔒 [{t['text_dim']}]•[/{t['text_dim']}] Legacy Shared Folder 📦 - [{t['text_dim']}]•[/{t['text_dim']}] Drive Shared Folder 👥 - [{t['text_dim']}]•[/{t['text_dim']}] Drive NonShared Folder 📁 + [{t['text_dim']}]•[/{t['text_dim']}] Nested Shared Folder (Shared) 👥 + [{t['text_dim']}]•[/{t['text_dim']}] Nested Shared Folder (NonShared) 📁 [{t['text_dim']}]Press [/{t['text_dim']}][{t['primary']}]?[/{t['primary']}][{t['text_dim']}] for full keyboard shortcuts[/{t['text_dim']}]""" @@ -393,8 +393,8 @@ async def on_mount(self): [bold {t['primary_bright']}]Folder Icons[/bold {t['primary_bright']}] [{t['text_dim']}]•[/{t['text_dim']}] Legacy Personal Folder 🔒 [{t['text_dim']}]•[/{t['text_dim']}] Legacy Shared Folder 📦 - [{t['text_dim']}]•[/{t['text_dim']}] Drive Shared Folder 👥 - [{t['text_dim']}]•[/{t['text_dim']}] Drive NonShared Folder 📁 + [{t['text_dim']}]•[/{t['text_dim']}] Nested Shared Folder (Shared) 👥 + [{t['text_dim']}]•[/{t['text_dim']}] Nested Shared Folder (NonShared) 📁 [{t['text_dim']}]Press [/{t['text_dim']}][{t['primary']}]?[/{t['primary']}][{t['text_dim']}] for full keyboard shortcuts[/{t['text_dim']}]""" detail_widget.update(help_content) @@ -984,8 +984,8 @@ def _get_folder_icon(self, folder_node): - Legacy Personal Folder (user_folder) → 🔒 - Legacy Shared Folder (shared_folder) → 📦 - Subfolder in Shared (shared_folder_folder) → 📦 - - Drive Shared Folder (nested_share_folder, shared) → 👥 - - Drive NonShared Folder (nested_share_folder, not shared) → 📁 + - Nested Shared Folder (Shared) (nested_share_folder, shared) → 👥 + - Nested Shared Folder (NonShared) (nested_share_folder, not shared) → 📁 """ from ...subfolder import BaseFolderNode if folder_node is None: diff --git a/keepercommander/commands/supershell/screens/help.py b/keepercommander/commands/supershell/screens/help.py index dcf13f088..a92a39c5f 100644 --- a/keepercommander/commands/supershell/screens/help.py +++ b/keepercommander/commands/supershell/screens/help.py @@ -119,8 +119,8 @@ def compose(self) -> ComposeResult: [green]Folder Icons:[/green] 🔒 Legacy Personal Folder 📦 Legacy Shared Folder - 👥 Drive Shared Folder - 📁 Drive NonShared Folder""", classes="help_column") + 👥 Nested Shared Folder (Shared) + 📁 Nested Shared Folder (NonShared)""", classes="help_column") yield Static("[dim]Press Esc or q to close[/dim]", id="help_footer") def action_dismiss(self): diff --git a/keepercommander/nested_share_folder/__init__.py b/keepercommander/nested_share_folder/__init__.py index a4a6641b6..8436b6ba1 100644 --- a/keepercommander/nested_share_folder/__init__.py +++ b/keepercommander/nested_share_folder/__init__.py @@ -40,6 +40,7 @@ 'create_record_data_v3', 'record_add_v3', 'record_update_v3', 'create_record_v3', 'update_record_v3', 'create_records_batch_v3', 'get_record_details_v3', 'get_record_accesses_v3', + 'find_direct_user_share_access', 'is_record_share_update_noop', 'share_record_v3', 'update_record_share_v3', 'unshare_record_v3', 'batch_update_record_shares_v3', 'batch_create_record_shares_v3', 'batch_unshare_records_v3', diff --git a/keepercommander/nested_share_folder/folder_api.py b/keepercommander/nested_share_folder/folder_api.py index 9c40fe00e..6656f324e 100644 --- a/keepercommander/nested_share_folder/folder_api.py +++ b/keepercommander/nested_share_folder/folder_api.py @@ -364,14 +364,15 @@ def grant_folder_access_v3(params, folder_uid, user_uid, role='viewer', existing = _check_existing_access(params, folder_uid, actual_uid_bytes, target_role_name, access_type_label) if existing is not None: - if existing == target_role_name: + if existing == target_role_name and expiration_timestamp is None: return {'folder_uid': folder_uid, 'user_uid': identifier_label, 'access_type': access_type_label, 'status': 'SUCCESS', 'message': f"{'Team' if as_team else 'User'} already has {role} access", 'success': True, 'action_taken': 'already_had_access'} result = update_folder_access_v3(params, folder_uid, identifier_label, - role=role, as_team=as_team) + role=role, as_team=as_team, + expiration_timestamp=expiration_timestamp) result['action_taken'] = 'updated' return result @@ -382,7 +383,7 @@ def grant_folder_access_v3(params, folder_uid, user_uid, role='viewer', ad.accessRoleType = access_role ad.permissions.CopyFrom(get_folder_permissions_for_role(access_role)) - if expiration_timestamp: + if expiration_timestamp is not None: ad.tlaProperties.expiration = expiration_timestamp if share_folder_key: @@ -430,9 +431,9 @@ def _check_existing_access(params, folder_uid, uid_bytes, target_role_name, def update_folder_access_v3(params, folder_uid, user_uid, role=None, hidden=None, - as_team=False): - if role is None and hidden is None: - raise ValueError("At least one field (role or hidden) required") + expiration_timestamp=None, as_team=False): + if role is None and hidden is None and expiration_timestamp is None: + raise ValueError("At least one field (role, hidden, or expiration) required") resolved = resolve_folder_identifier(params, folder_uid) if not resolved: raise ValueError(f"Folder '{folder_uid}' not found") @@ -453,6 +454,8 @@ def update_folder_access_v3(params, folder_uid, user_uid, role=None, hidden=None ad.permissions.CopyFrom(get_folder_permissions_for_role(resolved_role)) if hidden is not None: ad.hidden = hidden + if expiration_timestamp is not None: + ad.tlaProperties.expiration = expiration_timestamp response = folder_access_update_v3(params, folder_access_updates=[ad]) result = parse_folder_access_result(response, folder_uid, identifier_label, diff --git a/keepercommander/nested_share_folder/record_api.py b/keepercommander/nested_share_folder/record_api.py index db886a78b..f0bcc9e69 100644 --- a/keepercommander/nested_share_folder/record_api.py +++ b/keepercommander/nested_share_folder/record_api.py @@ -9,7 +9,7 @@ from .. import utils, crypto, api from ..params import KeeperParams -from ..proto import record_pb2, folder_pb2, record_endpoints_pb2, record_details_pb2, record_sharing_pb2 +from ..proto import record_pb2, folder_pb2, record_endpoints_pb2, record_details_pb2, record_sharing_pb2, tla_pb2 from ..error import KeeperApiError from ..api import pad_aes_gcm @@ -336,21 +336,84 @@ def get_record_accesses_v3(params, record_uids): 'can_update_access', 'can_delete', 'can_change_ownership', 'can_request_access', 'can_approve_access'): ao[flag] = getattr(d, flag, False) + # Proto3 scalar fields have no presence; only test the submessage. + if d.HasField('tlaProperties'): + ao['tla_expiration'] = d.tlaProperties.expiration result['record_accesses'].append(ao) for fu in rs.forbiddenRecords: result['forbidden_records'].append(utils.base64_url_encode(fu)) return result +def find_direct_user_share_access(access_result, record_uid, email): + """Return the direct AT_USER share row for *email* on *record_uid*, or None.""" + email_cf = email.casefold() + for access in access_result.get('record_accesses', []): + if access.get('record_uid') != record_uid or access.get('owner', False): + continue + if access.get('access_type') and access.get('access_type') != 'AT_USER': + continue + if access.get('inherited'): + continue + if access.get('accessor_name', '').casefold() == email_cf: + return access + return None + + +_SHARE_EXPIRATION_NOOP_TOLERANCE_MS = 60_000 + + +def is_record_share_update_noop(existing, access_role_type, expiration_timestamp): + """True when an update would leave role and expiration unchanged.""" + if existing.get('access_role_type') != access_role_type: + return False + existing_exp = existing.get('tla_expiration') + if expiration_timestamp is None: + return True + if expiration_timestamp == -1: + return not existing_exp or existing_exp <= 0 + if expiration_timestamp > 0: + if not existing_exp or existing_exp <= 0: + return False + return abs(existing_exp - expiration_timestamp) <= _SHARE_EXPIRATION_NOOP_TOLERANCE_MS + return False + + # ══════════════════════════════════════════════════════════════════════════ # Record sharing (Strategy: share / update / revoke) # ══════════════════════════════════════════════════════════════════════════ +def _apply_share_expiration_tla(tla_props, expiration_timestamp): + """Set expiration / notification fields on a TLAProperties proto.""" + if expiration_timestamp is None: + return + tla_props.expiration = expiration_timestamp + if expiration_timestamp > 0: + tla_props.timerNotificationType = tla_pb2.NOTIFY_OWNER + + +def _build_revoke_permission(params, record_uid, recipient_email): + """Build a minimal Permissions proto for revokeSharingPermissions.""" + _, _, uid_bytes, _ = get_user_public_key(params, recipient_email) + if not uid_bytes: + raise ValueError(f"User {recipient_email} not found") + + uid_b = utils.base64_url_decode(record_uid) + perm = record_sharing_pb2.Permissions() + perm.recipientUid = uid_bytes + perm.recordUid = uid_b + perm.rules.accessTypeUid = uid_bytes + perm.rules.accessType = folder_pb2.AT_USER + perm.rules.recordUid = uid_b + return perm + + def _build_share_permissions(params, record_uid, recipient_email, access_role_type, - expiration_timestamp, include_role): + expiration_timestamp, include_role, skip_sync=False): """Build a Permissions protobuf for share/update — single source of truth.""" - from .. import sync_down as sd - sd.sync_down(params) + if not skip_sync: + from .. import sync_down as sd + sd.sync_down(params) rec = get_record_from_cache(params, record_uid) if not rec: @@ -381,16 +444,25 @@ def _build_share_permissions(params, record_uid, recipient_email, access_role_ty perm.rules.owner = False if include_role and access_role_type is not None: perm.rules.accessRoleType = access_role_type - if expiration_timestamp: - perm.rules.tlaProperties.expiration = expiration_timestamp + _apply_share_expiration_tla(perm.rules.tlaProperties, expiration_timestamp) return perm -def share_record_v3(params, record_uid, recipient_email, access_role_type, - expiration_timestamp=None): +def _send_revoke_share_request(params, record_uid, recipient_email): + perm = _build_revoke_permission(params, record_uid, recipient_email) + rq = record_sharing_pb2.Request() + rq.revokeSharingPermissions.append(perm) + rs = api.communicate_rest(params, rq, 'vault/records/v3/share', + rs_type=record_sharing_pb2.Response) + results = [parse_sharing_status(s) for s in rs.revokedSharingStatus] + return {'results': results, 'success': all(r['success'] for r in results)} + + +def _send_create_share_request(params, record_uid, recipient_email, access_role_type, + expiration_timestamp, skip_sync=False): perm = _build_share_permissions(params, record_uid, recipient_email, access_role_type, expiration_timestamp, - include_role=True) + include_role=True, skip_sync=skip_sync) rq = record_sharing_pb2.Request() rq.createSharingPermissions.append(perm) rs = api.communicate_rest(params, rq, 'vault/records/v3/share', @@ -399,46 +471,78 @@ def share_record_v3(params, record_uid, recipient_email, access_role_type, return {'results': results, 'success': all(r['success'] for r in results)} -def update_record_share_v3(params, record_uid, recipient_email, - access_role_type=None, expiration_timestamp=None): +def share_record_v3(params, record_uid, recipient_email, access_role_type, + expiration_timestamp=None): perm = _build_share_permissions(params, record_uid, recipient_email, access_role_type, expiration_timestamp, include_role=True) rq = record_sharing_pb2.Request() - rq.updateSharingPermissions.append(perm) + rq.createSharingPermissions.append(perm) rs = api.communicate_rest(params, rq, 'vault/records/v3/share', rs_type=record_sharing_pb2.Response) - results = [parse_sharing_status(s) for s in rs.updatedSharingStatus] + results = [parse_sharing_status(s) for s in rs.createdSharingStatus] return {'results': results, 'success': all(r['success'] for r in results)} -def unshare_record_v3(params, record_uid, recipient_email): +def update_record_share_v3(params, record_uid, recipient_email, + access_role_type=None, expiration_timestamp=None): + """Update an existing direct share. + + Role changes use ``updateSharingPermissions``. Positive expirations use + revoke then create (two requests) because the server only INSERTs + ``record_access_expiration`` on create — sending ``tlaProperties`` on + update fails when the share already exists. + """ from .. import sync_down as sd sd.sync_down(params) rec = get_record_from_cache(params, record_uid) if not rec: raise ValueError(f"Record {record_uid} not found in cache") - _, _, uid_bytes, _ = get_user_public_key(params, recipient_email) - if not uid_bytes: - raise ValueError(f"User {recipient_email} not found") - uid_b = utils.base64_url_decode(record_uid) - perm = record_sharing_pb2.Permissions() - perm.recipientUid = uid_bytes - perm.recordUid = uid_b - perm.rules.accessTypeUid = uid_bytes - perm.rules.accessType = folder_pb2.AT_USER - perm.rules.recordUid = uid_b + if expiration_timestamp is not None and expiration_timestamp > 0: + if access_role_type is None: + raise ValueError( + 'access_role_type is required when setting share expiration') + # Expiration must be applied via create, not update. The server + # rejects revoke+create for the same (recipient, record) in one + # request, so run them sequentially (one sync_down above only). + logger.info( + "Revoking existing share with '%s' before re-granting with expiration...", + recipient_email) + revoke_result = _send_revoke_share_request(params, record_uid, recipient_email) + if not revoke_result.get('success'): + return revoke_result + logger.info( + "Re-granting share to '%s' with updated role and expiration...", + recipient_email) + return _send_create_share_request( + params, record_uid, recipient_email, access_role_type, + expiration_timestamp, skip_sync=True) + perm = _build_share_permissions(params, record_uid, recipient_email, + access_role_type, expiration_timestamp, + include_role=True, skip_sync=True) rq = record_sharing_pb2.Request() - rq.revokeSharingPermissions.append(perm) + rq.updateSharingPermissions.append(perm) rs = api.communicate_rest(params, rq, 'vault/records/v3/share', rs_type=record_sharing_pb2.Response) - results = [parse_sharing_status(s) for s in rs.revokedSharingStatus] + results = [parse_sharing_status(s) for s in rs.updatedSharingStatus] return {'results': results, 'success': all(r['success'] for r in results)} +def unshare_record_v3(params, record_uid, recipient_email, skip_sync=False): + if not skip_sync: + from .. import sync_down as sd + sd.sync_down(params) + + rec = get_record_from_cache(params, record_uid) + if not rec: + raise ValueError(f"Record {record_uid} not found in cache") + + return _send_revoke_share_request(params, record_uid, recipient_email) + + # ══════════════════════════════════════════════════════════════════════════ # Batch record sharing (bulk update / revoke in a single REST call per chunk) # ══════════════════════════════════════════════════════════════════════════ @@ -461,6 +565,45 @@ def batch_update_record_shares_v3(params, updates, expiration_timestamp=None, ch sd.sync_down(params) outcomes = [] + recreate = expiration_timestamp is not None and expiration_timestamp > 0 + if recreate: + for i in range(0, len(updates), chunk_size): + chunk = updates[i:i + chunk_size] + rq = record_sharing_pb2.Request() + built = [] + for u in chunk: + try: + rq.revokeSharingPermissions.append( + _build_revoke_permission(params, u['record_uid'], u['email'])) + built.append(u) + except Exception as exc: + outcomes.append((u, {'success': False, 'skipped': True, + 'message': str(exc)})) + if not built: + continue + try: + rs = api.communicate_rest(params, rq, 'vault/records/v3/share', + rs_type=record_sharing_pb2.Response) + statuses = [parse_sharing_status(x) for x in rs.revokedSharingStatus] + if statuses: + revoked = {s['record_uid'] for s in statuses if s['success']} + else: + revoked = {u['record_uid'] for u in built} + for u in built: + if u['record_uid'] not in revoked: + outcomes.append((u, {'success': False, + 'message': 'Revoke failed'})) + except Exception as exc: + for u in built: + outcomes.append((u, {'success': False, 'message': str(exc)})) + failed_uids = {u['record_uid'] for u, r in outcomes if not r.get('success')} + create_updates = [u for u in updates if u['record_uid'] not in failed_uids] + if create_updates: + create_outcomes = batch_create_record_shares_v3( + params, create_updates, expiration_timestamp, chunk_size) + outcomes.extend(create_outcomes) + return outcomes + for i in range(0, len(updates), chunk_size): chunk = updates[i:i + chunk_size] rq = record_sharing_pb2.Request() @@ -470,7 +613,7 @@ def batch_update_record_shares_v3(params, updates, expiration_timestamp=None, ch perm = _build_share_permissions( params, u['record_uid'], u['email'], u['access_role_type'], expiration_timestamp, - include_role=True, + include_role=True, skip_sync=True, ) rq.updateSharingPermissions.append(perm) built.append(u) @@ -526,7 +669,7 @@ def batch_create_record_shares_v3(params, creates, expiration_timestamp=None, ch perm = _build_share_permissions( params, c['record_uid'], c['email'], c['access_role_type'], expiration_timestamp, - include_role=True, + include_role=True, skip_sync=True, ) rq.createSharingPermissions.append(perm) built.append(c) diff --git a/unit-tests/test_command_register.py b/unit-tests/test_command_register.py index 2da3c9b00..3074af5a7 100644 --- a/unit-tests/test_command_register.py +++ b/unit-tests/test_command_register.py @@ -201,6 +201,13 @@ def test_share_record_update_expiration_without_roe(self): finally: TestRegister.record_share_rq_rs = original + def test_get_share_expiration_rejects_sub_minute(self): + with self.assertRaises(CommandError) as ctx: + register.get_share_expiration(None, '0mi', cmd_name='share-record') + self.assertIn('at least 1 minute', str(ctx.exception)) + with self.assertRaises(CommandError): + register.get_share_expiration('2020-01-01T00:00:00', None, cmd_name='share-record') + def test_share_record_update_clears_expiration_with_never(self): """--expire-at never on an existing share must clear the timer (expiration = -1).""" params = get_synced_params() diff --git a/unit-tests/test_nested_share_folder.py b/unit-tests/test_nested_share_folder.py index c5bd7b591..a9e784ae5 100644 --- a/unit-tests/test_nested_share_folder.py +++ b/unit-tests/test_nested_share_folder.py @@ -69,6 +69,15 @@ def _make_record(record_uid=None, title='Test Record'): } +def _make_sharing_status(record_uid, recipient_uid_bytes=None): + from keepercommander.proto import record_sharing_pb2 + status = record_sharing_pb2.Status() + status.recordUid = utils.base64_url_decode(record_uid) + status.recipientUid = recipient_uid_bytes or utils.base64_url_decode(utils.generate_uid()) + status.status = record_sharing_pb2.SUCCESS + return status + + class TestCommandHelpers(TestCase): def test_parse_expiration_none(self): @@ -99,6 +108,14 @@ def test_parse_expiration_invalid(self): with self.assertRaises(CommandError): parse_expiration(None, 'invalid', 'test') + def test_parse_expiration_rejects_sub_minute(self): + from keepercommander.commands.nested_share_folder.helpers import parse_expiration + with self.assertRaises(CommandError) as ctx: + parse_expiration(None, '0mi', 'test') + self.assertIn('at least 1 minute', str(ctx.exception)) + with self.assertRaises(CommandError): + parse_expiration('2020-01-01T00:00:00Z', None, 'test') + def test_infer_role(self): from keepercommander.commands.nested_share_folder.helpers import infer_role self.assertEqual(infer_role({'can_change_ownership': True}), 'full-manager') @@ -783,6 +800,218 @@ def test_grant_folder_access_sends_invite_when_no_active_share( mock_handle_invite.assert_called_once_with(params, email, True) mock_access_update.assert_not_called() + @patch('keepercommander.nested_share_folder.folder_api.parse_folder_access_result') + @patch('keepercommander.nested_share_folder.folder_api.folder_access_update_v3') + @patch('keepercommander.nested_share_folder.folder_api._resolve_accessor') + @patch('keepercommander.nested_share_folder.folder_api.resolve_folder_identifier') + def test_update_folder_access_v3_sets_expiration( + self, mock_resolve_folder, mock_resolve_accessor, + mock_access_update, mock_parse_result): + from keepercommander.nested_share_folder.folder_api import update_folder_access_v3 + + fuid, _ = _make_folder() + email = 'user@example.com' + uid_bytes = utils.base64_url_decode(utils.generate_uid()) + mock_resolve_folder.return_value = fuid + mock_resolve_accessor.return_value = (uid_bytes, email, 1) + mock_parse_result.return_value = {'success': True} + mock_access_update.return_value = Mock() + + expiration = 1_700_000_000_000 + update_folder_access_v3( + _make_params(), fuid, email, expiration_timestamp=expiration) + + update_call = mock_access_update.call_args + ad = update_call.kwargs['folder_access_updates'][0] + self.assertEqual(ad.tlaProperties.expiration, expiration) + + @patch('keepercommander.nested_share_folder.folder_api.update_folder_access_v3') + @patch('keepercommander.nested_share_folder.folder_api._check_existing_access') + @patch('keepercommander.nested_share_folder.folder_api.get_user_public_key') + @patch('keepercommander.nested_share_folder.folder_api.resolve_folder_identifier') + def test_grant_folder_access_update_passes_expiration( + self, mock_resolve_folder, mock_get_public_key, + mock_existing, mock_update): + from keepercommander.nested_share_folder.folder_api import grant_folder_access_v3 + + fuid, fobj = _make_folder() + email = 'user@example.com' + uid_bytes = utils.base64_url_decode(utils.generate_uid()) + mock_resolve_folder.return_value = fuid + mock_get_public_key.return_value = (Mock(), False, uid_bytes, False) + mock_existing.return_value = 'viewer' + mock_update.return_value = {'success': True} + + expiration = 1_800_000_000_000 + grant_folder_access_v3( + _make_params(nested_share_folders={fuid: fobj}), + fuid, email, role='content-manager', expiration_timestamp=expiration) + + mock_update.assert_called_once_with( + mock.ANY, fuid, email, role='content-manager', as_team=False, + expiration_timestamp=expiration) + + @patch('keepercommander.nested_share_folder.folder_api.update_folder_access_v3') + @patch('keepercommander.nested_share_folder.folder_api._check_existing_access') + @patch('keepercommander.nested_share_folder.folder_api.get_user_public_key') + @patch('keepercommander.nested_share_folder.folder_api.resolve_folder_identifier') + def test_grant_folder_access_same_role_updates_expiration( + self, mock_resolve_folder, mock_get_public_key, + mock_existing, mock_update): + from keepercommander.nested_share_folder.folder_api import grant_folder_access_v3 + + fuid, fobj = _make_folder() + email = 'user@example.com' + uid_bytes = utils.base64_url_decode(utils.generate_uid()) + mock_resolve_folder.return_value = fuid + mock_get_public_key.return_value = (Mock(), False, uid_bytes, False) + mock_existing.return_value = 'viewer' + mock_update.return_value = {'success': True} + + expiration = 1_900_000_000_000 + grant_folder_access_v3( + _make_params(nested_share_folders={fuid: fobj}), + fuid, email, role='viewer', expiration_timestamp=expiration) + + mock_update.assert_called_once_with( + mock.ANY, fuid, email, role='viewer', as_team=False, + expiration_timestamp=expiration) + + +class TestNestedShareFolderRecordApi(TestCase): + + def setUp(self): + from keepercommander.nested_share_folder import record_api # noqa: F401 + mock.patch('keepercommander.sync_down.sync_down').start() + + def tearDown(self): + mock.patch.stopall() + + @patch('keepercommander.nested_share_folder.record_api.api.communicate_rest') + @patch('keepercommander.nested_share_folder.record_api.encrypt_for_recipient') + @patch('keepercommander.nested_share_folder.record_api.get_user_public_key') + @patch('keepercommander.nested_share_folder.record_api.get_record_from_cache') + def test_update_record_share_v3_sets_expiration_and_notification( + self, mock_get_record, mock_get_public_key, + mock_encrypt, mock_communicate): + from keepercommander.nested_share_folder.record_api import update_record_share_v3 + from keepercommander.proto import tla_pb2 + + ruid, robj = _make_record() + email = 'user@example.com' + uid_bytes = utils.base64_url_decode(utils.generate_uid()) + mock_get_record.return_value = robj + mock_get_public_key.return_value = (Mock(), False, uid_bytes, False) + mock_encrypt.return_value = b'enc-key' + mock_response = Mock() + mock_response.updatedSharingStatus = [] + mock_communicate.return_value = mock_response + + update_record_share_v3( + _make_params(nested_share_records={ruid: robj}), + ruid, email, access_role_type=1, expiration_timestamp=-1) + + rq = mock_communicate.call_args[0][1] + perm = rq.updateSharingPermissions[0] + self.assertEqual(perm.rules.tlaProperties.expiration, -1) + + @patch('keepercommander.nested_share_folder.record_api.api.communicate_rest') + @patch('keepercommander.nested_share_folder.record_api.encrypt_for_recipient') + @patch('keepercommander.nested_share_folder.record_api.get_user_public_key') + @patch('keepercommander.nested_share_folder.record_api.get_record_from_cache') + def test_update_record_share_v3_recreate_when_setting_expiration( + self, mock_get_record, mock_get_public_key, + mock_encrypt, mock_communicate): + from keepercommander.nested_share_folder.record_api import update_record_share_v3 + from keepercommander.proto import tla_pb2 + + ruid, robj = _make_record() + email = 'user@example.com' + uid_bytes = utils.base64_url_decode(utils.generate_uid()) + mock_get_record.return_value = robj + mock_get_public_key.return_value = (Mock(), False, uid_bytes, False) + mock_encrypt.return_value = b'enc-key' + mock_response = Mock() + mock_response.revokedSharingStatus = [_make_sharing_status(ruid, uid_bytes)] + mock_response.createdSharingStatus = [_make_sharing_status(ruid, uid_bytes)] + mock_communicate.side_effect = [mock_response, mock_response] + + expiration = 1_900_000_000_000 + update_record_share_v3( + _make_params(nested_share_records={ruid: robj}), + ruid, email, access_role_type=1, expiration_timestamp=expiration) + + self.assertEqual(mock_communicate.call_count, 2) + revoke_rq = mock_communicate.call_args_list[0][0][1] + create_rq = mock_communicate.call_args_list[1][0][1] + self.assertEqual(len(revoke_rq.revokeSharingPermissions), 1) + self.assertEqual(len(revoke_rq.createSharingPermissions), 0) + self.assertEqual(len(create_rq.createSharingPermissions), 1) + self.assertEqual(len(create_rq.updateSharingPermissions), 0) + perm = create_rq.createSharingPermissions[0] + self.assertEqual(perm.rules.tlaProperties.expiration, expiration) + self.assertEqual(perm.rules.tlaProperties.timerNotificationType, tla_pb2.NOTIFY_OWNER) + + @patch('keepercommander.sync_down.sync_down') + @patch('keepercommander.nested_share_folder.record_api.api.communicate_rest') + @patch('keepercommander.nested_share_folder.record_api.encrypt_for_recipient') + @patch('keepercommander.nested_share_folder.record_api.get_user_public_key') + @patch('keepercommander.nested_share_folder.record_api.get_record_from_cache') + def test_update_record_share_v3_recreate_syncs_once( + self, mock_get_record, mock_get_public_key, + mock_encrypt, mock_communicate, mock_sync_down): + from keepercommander.nested_share_folder import record_api as ra + + ruid, robj = _make_record() + email = 'user@example.com' + uid_bytes = utils.base64_url_decode(utils.generate_uid()) + mock_get_record.return_value = robj + mock_get_public_key.return_value = (Mock(), False, uid_bytes, False) + mock_encrypt.return_value = b'enc-key' + mock_response = Mock() + mock_response.revokedSharingStatus = [_make_sharing_status(ruid, uid_bytes)] + mock_response.createdSharingStatus = [_make_sharing_status(ruid, uid_bytes)] + mock_communicate.side_effect = [mock_response, mock_response] + + ra.update_record_share_v3( + _make_params(nested_share_records={ruid: robj}), + ruid, email, access_role_type=1, expiration_timestamp=1_900_000_000_000) + + self.assertEqual(mock_sync_down.call_count, 1) + + def test_is_record_share_update_noop(self): + from keepercommander.nested_share_folder.record_api import is_record_share_update_noop + + existing = {'access_role_type': 2, 'tla_expiration': 1_900_000_000_000} + self.assertTrue(is_record_share_update_noop(existing, 2, None)) + self.assertTrue(is_record_share_update_noop( + existing, 2, 1_900_000_000_500)) + self.assertFalse(is_record_share_update_noop(existing, 4, None)) + self.assertFalse(is_record_share_update_noop( + existing, 2, 1_900_001_000_000)) + self.assertTrue(is_record_share_update_noop( + {'access_role_type': 2}, 2, -1)) + + @patch('keepercommander.nested_share_folder.record_api.api.communicate_rest') + def test_get_record_accesses_v3_reads_tla_expiration(self, mock_communicate): + from keepercommander.nested_share_folder.record_api import get_record_accesses_v3 + from keepercommander.proto import record_details_pb2, folder_pb2, tla_pb2 + + ruid = utils.generate_uid() + rs = record_details_pb2.RecordAccessResponse() + ra = rs.recordAccesses.add() + ra.data.recordUid = utils.base64_url_decode(ruid) + ra.data.accessTypeUid = b'\x01' * 16 + ra.data.accessType = folder_pb2.AT_USER + ra.data.accessRoleType = folder_pb2.VIEWER + ra.data.tlaProperties.expiration = 1_783_667_017_211 + ra.data.tlaProperties.timerNotificationType = tla_pb2.NOTIFY_OWNER + ra.accessorInfo.name = 'user@example.com' + mock_communicate.return_value = rs + + result = get_record_accesses_v3(_make_params(), [ruid]) + self.assertEqual(result['record_accesses'][0]['tla_expiration'], 1_783_667_017_211) + class TestNestedShareFolderDisplayCommands(TestCase):