From e75769aab101427b1de9bf683f4ceb06f5700d34 Mon Sep 17 00:00:00 2001 From: erinlewis-keeper Date: Wed, 7 Oct 2026 17:53:57 -0700 Subject: [PATCH 1/4] feat(DR-1330): generate azure credentials with auth flow --- .gitignore | 1 + .../commands/pam_cloud/entra_sign_in.py | 158 ++++++++++++++++++ .../pam_cloud/pam_cloud_credentials.py | 83 ++++++++- unit-tests/test_entra_sign_in.py | 122 ++++++++++++++ unit-tests/test_pam_cloud_credentials.py | 106 ++++++++++++ 5 files changed, 461 insertions(+), 9 deletions(-) create mode 100644 keepercommander/commands/pam_cloud/entra_sign_in.py create mode 100644 unit-tests/test_entra_sign_in.py create mode 100644 unit-tests/test_pam_cloud_credentials.py diff --git a/.gitignore b/.gitignore index 1b40c8dc0..7d1e58bee 100644 --- a/.gitignore +++ b/.gitignore @@ -27,3 +27,4 @@ tests/*_results/ tests/*.log tests/compliance_test.env docker-compose.yml +uv.lock diff --git a/keepercommander/commands/pam_cloud/entra_sign_in.py b/keepercommander/commands/pam_cloud/entra_sign_in.py new file mode 100644 index 000000000..c4d08afe4 --- /dev/null +++ b/keepercommander/commands/pam_cloud/entra_sign_in.py @@ -0,0 +1,158 @@ +"""Interactive Microsoft Entra sign-in for a CLI: OAuth 2.0 authorization code flow with PKCE. + +The user signs in (including any MFA prompt) in their own browser and Entra redirects back to a +short-lived loopback listener on this machine, which redeems the code for an access token. No +password or MFA code ever passes through Commander. The app registration needs the +``http://localhost`` redirect under "Mobile and desktop applications" and public client flows +enabled. +""" +import base64 +import hashlib +import http.server +import json +import logging +import secrets +import threading +import time +import urllib.parse +import webbrowser + +import requests + +from keepercommander.error import CommandError + + +logger = logging.getLogger(__name__) + +COMMAND = 'pam-cloud-generate-credentials' +DEFAULT_AUTHORITY = 'https://login.microsoftonline.com' +SIGN_IN_TIMEOUT_SECONDS = 300 + + +def _b64url(raw): + return base64.urlsafe_b64encode(raw).rstrip(b'=').decode('ascii') + + +def pkce_pair(): + """Return (code_verifier, code_challenge) for the S256 method.""" + verifier = _b64url(secrets.token_bytes(32)) + return verifier, _b64url(hashlib.sha256(verifier.encode('ascii')).digest()) + + +def build_authorize_url(authority, tenant_id, client_id, redirect_uri, scope, state, code_challenge, login_hint=None): + query = { + 'client_id': client_id, + 'response_type': 'code', + 'redirect_uri': redirect_uri, + 'response_mode': 'query', + 'scope': scope, + 'state': state, + 'code_challenge': code_challenge, + 'code_challenge_method': 'S256', + } + if login_hint: + query['login_hint'] = login_hint + return f'{authority}/{urllib.parse.quote(tenant_id, safe="")}/oauth2/v2.0/authorize?' \ + + urllib.parse.urlencode(query) + + +def token_claims(token): + """Return the claims of a JWT without verifying it (it came straight from Entra over TLS).""" + try: + segment = token.split('.')[1] + return json.loads(base64.urlsafe_b64decode(segment + '=' * (-len(segment) % 4))) + except Exception: + return {} + + +def signed_in_identities(claims): + return {str(claims[key]).strip().lower() + for key in ('upn', 'unique_name', 'preferred_username', 'email') if claims.get(key)} + + +class _CallbackHandler(http.server.BaseHTTPRequestHandler): + def do_GET(self): + params = {k: v[0] for k, v in urllib.parse.parse_qs(urllib.parse.urlparse(self.path).query).items()} + if 'code' in params or 'error' in params: + self.server.result = params + body = 'Sign-in complete. You can close this window and return to Keeper Commander.' + else: + body = 'Waiting for sign-in.' + payload = body.encode('utf-8') + self.send_response(200) + self.send_header('Content-Type', 'text/plain; charset=utf-8') + self.send_header('Content-Length', str(len(payload))) + self.end_headers() + self.wfile.write(payload) + + def log_message(self, *args): + pass + + +def _wait_for_redirect(server, timeout): + server.result = None + server.timeout = 1 + deadline = time.monotonic() + timeout + while server.result is None and time.monotonic() < deadline: + server.handle_request() + return server.result + + +def sign_in(tenant_id, client_id, scope, login_hint, authority=DEFAULT_AUTHORITY, + timeout=SIGN_IN_TIMEOUT_SECONDS, open_browser=webbrowser.open): + """Sign `login_hint` in through the system browser and return the token response dict. + + Raises CommandError if the sign-in is cancelled, times out, fails, or completes as a + different user than `login_hint`. + """ + server = http.server.HTTPServer(('127.0.0.1', 0), _CallbackHandler) + try: + redirect_uri = f'http://localhost:{server.server_port}' + verifier, challenge = pkce_pair() + state = secrets.token_urlsafe(16) + url = build_authorize_url(authority, tenant_id, client_id, redirect_uri, f'{scope} openid profile', + state, challenge, login_hint) + + print('Opening your browser to sign in to Microsoft. If it does not open, visit:') + print(f' {url}') + try: + open_browser(url) + except Exception as e: + logger.debug('Could not open a browser: %r', e) + + result = _wait_for_redirect(server, timeout) + finally: + server.server_close() + + if result is None: + raise CommandError(COMMAND, 'The Microsoft sign-in was not completed in time.') + if result.get('state') != state: + raise CommandError(COMMAND, 'The Microsoft sign-in response did not match the request.') + if 'error' in result: + raise CommandError(COMMAND, f'Microsoft sign-in failed: {result.get("error_description") or result["error"]}') + + try: + response = requests.post( + f'{authority}/{urllib.parse.quote(tenant_id, safe="")}/oauth2/v2.0/token', + data={ + 'grant_type': 'authorization_code', + 'client_id': client_id, + 'code': result['code'], + 'redirect_uri': redirect_uri, + 'code_verifier': verifier, + 'scope': f'{scope} openid profile', + }, + timeout=30) + token = response.json() + except (requests.RequestException, ValueError) as e: + raise CommandError(COMMAND, f'Could not redeem the Microsoft sign-in: {type(e).__name__}') + + if response.status_code != 200 or not token.get('access_token'): + description = (token.get('error_description') or token.get('error') or 'unknown error').splitlines()[0] + raise CommandError(COMMAND, f'Microsoft rejected the sign-in: {description}') + + expected = (login_hint or '').strip().lower() + identities = signed_in_identities(token_claims(token.get('id_token') or token['access_token'])) + if expected and expected not in identities: + raise CommandError(COMMAND, f'The sign-in was completed as a different user than {login_hint}.') + return token diff --git a/keepercommander/commands/pam_cloud/pam_cloud_credentials.py b/keepercommander/commands/pam_cloud/pam_cloud_credentials.py index 374d5be87..52a2cd610 100644 --- a/keepercommander/commands/pam_cloud/pam_cloud_credentials.py +++ b/keepercommander/commands/pam_cloud/pam_cloud_credentials.py @@ -1,9 +1,11 @@ import argparse import base64 import json +import datetime import logging from keepercommander.commands.base import Command, GroupCommand, RecordMixin +from keepercommander.commands.pam_cloud import entra_sign_in from keepercommander.commands.pam.pam_dto import GatewayAction from keepercommander.commands.pam.router_helper import dispatch_streamed_access_elevation_action, get_dag_leafs from keepercommander.commands.pam.vault_target import ( @@ -74,14 +76,66 @@ def _resolve_config_uid(params, record_uid, configuration): return _resolve_configuration_record(params, configuration, linked_config_uids=config_uids) +def _config_field(record, label): + """Value of the config record field called `label` (e.g. tenantId), or ''.""" + for field in list(record.fields) + list(record.custom): + if (field.label or '').casefold() == label.casefold(): + return field.get_default_value(str) or '' + return '' + + +# Not narrowed to any one API or application: .default asks for everything the app registration is +# already consented for, against the tenant directory the user lives in. +DEFAULT_SIGN_IN_SCOPE = 'https://graph.microsoft.com/.default' + + +def _sign_in_locally(config_record, login, client_id_override, scope_override=None): + """Sign the user in to Entra in their own browser (MFA happens there) and return the + (credential_record, expiration) the Gateway would otherwise have minted.""" + tenant_id = _config_field(config_record, 'tenantId') + client_id = client_id_override or _config_field(config_record, 'clientId') + if not tenant_id or not client_id: + raise CommandError('pam-cloud-generate-credentials', + 'The PAM configuration needs a tenant ID and client ID (or pass --client-id).') + + scope = scope_override or DEFAULT_SIGN_IN_SCOPE + token = entra_sign_in.sign_in(tenant_id, client_id, scope, login_hint=login) + + expires_in = int(token.get('expires_in') or 3600) + expiration = (datetime.datetime.now(datetime.timezone.utc) + + datetime.timedelta(seconds=expires_in)).isoformat() + return { + 'record_type': 'pamUser', + 'title': f'Temporary AZURE access token - {login}', + 'fields': [ + {'type': 'login', 'value': login}, + {'type': 'password', 'value': ''}, + {'type': 'secret', 'label': 'privatePEMKey', 'value': json.dumps({'accessToken': token['access_token']})}, + {'type': 'text', 'label': 'Provider', 'value': 'azure'}, + ], + 'notes': (f"Short-lived AZURE credentials issued by Keeper PAM for '{login}'. " + f"Auto-expires {expiration}. Do not share."), + }, expiration + + class PAMCloudGenerateCredentialsCommand(Command): parser = argparse.ArgumentParser(prog='pam cloud generate-credentials', description='Generate a temporary cloud access token for a PAM cloud resource, ' - 'scoped to the logged-in user. The resource\'s own cloud provider ' - 'mints the credential; the gateway never creates a new account.') + 'scoped to the logged-in user; the gateway never creates a new account. ' + 'Azure: the Gateway grants the access, then you sign in to Microsoft ' + 'in your browser (MFA works) to receive the token.') parser.add_argument('record', help='PAM cloud resource (pamCloudResource) UID, title, or folder path') parser.add_argument('--configuration', '-c', dest='configuration', help='PAM configuration UID or title, if the resource is linked to more than one') + parser.add_argument('--client-id', dest='client_id', + help='Azure: application (client) ID to sign in with. Defaults to ' + 'the PAM configuration\'s client ID, which needs a public-client ' + 'http://localhost redirect.') + parser.add_argument('--scope', dest='scope', + help='Azure: narrow the token to a specific API, e.g. ' + 'https://management.azure.com/.default. Not needed normally: the default is ' + 'https://graph.microsoft.com/.default, the tenant the user exists in, with ' + 'nothing narrowed. The API must be in the app registration\'s permissions.') parser.add_argument('--duration-seconds', '-d', dest='duration_seconds', type=int, help='Credential lifetime in seconds (provider-clamped). Leave unset to use the ' 'resource\'s own configured elevation time, which also matches how long the ' @@ -120,6 +174,12 @@ def execute(self, params, **kwargs): 'username': params.user, 'senderPublicKey': sender_public_key, } + + config_record = vault.KeeperRecord.load(params, config_uid) + if isinstance(config_record, vault.TypedRecord) and config_record.record_type == 'pamAzureConfiguration': + # Entra cannot mint a token for a user without that user signing in (MFA included), so the + # Gateway only attaches the access and the requester signs in to Microsoft themselves below. + inputs['clientSideAuth'] = True duration_seconds = kwargs.get('duration_seconds') if duration_seconds: inputs['durationSeconds'] = duration_seconds @@ -135,13 +195,18 @@ def execute(self, params, **kwargs): if not data.get('success', False): raise CommandError('pam-cloud-generate-credentials', data.get('error') or 'Unknown error') - encrypted_record = data.get('encrypted_record') - if not encrypted_record: - raise CommandError('pam-cloud-generate-credentials', - 'The Gateway did not return any credential material.') + if data.get('clientSideAuth'): + credential_record, expiration = _sign_in_locally( + config_record, inputs['username'], kwargs.get('client_id'), kwargs.get('scope')) + else: + encrypted_record = data.get('encrypted_record') + if not encrypted_record: + raise CommandError('pam-cloud-generate-credentials', + 'The Gateway did not return any credential material.') - decrypted = crypto.decrypt_ec(base64.b64decode(encrypted_record), private_key) - credential_record = json.loads(decrypted) + decrypted = crypto.decrypt_ec(base64.b64decode(encrypted_record), private_key) + credential_record = json.loads(decrypted) + expiration = data.get('expiration', '') fields = credential_record.get('fields', []) login = next((f.get('value') for f in fields if f.get('type') == 'login'), '') @@ -152,7 +217,7 @@ def execute(self, params, **kwargs): print(f' Provider: {provider}') print(f' Login: {login}') print(f' Credential: {"**********" if has_secret else "(none)"}') - print(f' Expiration: {data.get("expiration", "")}') + print(f' Expiration: {expiration}') if kwargs.get('save_record'): typed_record = vault.TypedRecord() diff --git a/unit-tests/test_entra_sign_in.py b/unit-tests/test_entra_sign_in.py new file mode 100644 index 000000000..ebb23f269 --- /dev/null +++ b/unit-tests/test_entra_sign_in.py @@ -0,0 +1,122 @@ +import base64 +import hashlib +import json +import threading +import urllib.parse +import urllib.request +from unittest import TestCase, mock + +from keepercommander.commands.pam_cloud import entra_sign_in +from keepercommander.error import CommandError + + +def _jwt(**claims): + body = base64.urlsafe_b64encode(json.dumps(claims).encode()).decode().rstrip('=') + return f'h.{body}.s' + + +def _token_response(status=200, **payload): + response = mock.Mock() + response.status_code = status + response.json.return_value = payload + return response + + +class FakeBrowser: + """Follows the authorize URL by calling the loopback redirect, like a signed-in browser.""" + + def __init__(self, **redirect_params): + self.redirect_params = redirect_params + self.authorize_url = None + + def __call__(self, url): + self.authorize_url = urllib.parse.urlparse(url) + query = {k: v[0] for k, v in urllib.parse.parse_qs(self.authorize_url.query).items()} + params = {'state': query['state'], **self.redirect_params} + redirect = f'{query["redirect_uri"]}/?{urllib.parse.urlencode(params)}' + # The listener is single-threaded and not yet serving, so hit it from another thread. + threading.Thread(target=lambda: urllib.request.urlopen(redirect, timeout=10).read(), daemon=True).start() + + +class PkceTest(TestCase): + + def test_challenge_is_s256_of_verifier(self): + verifier, challenge = entra_sign_in.pkce_pair() + expected = base64.urlsafe_b64encode(hashlib.sha256(verifier.encode()).digest()).rstrip(b'=').decode() + self.assertEqual(expected, challenge) + self.assertGreaterEqual(len(verifier), 43) + + def test_authorize_url_carries_pkce_state_and_hint(self): + url = entra_sign_in.build_authorize_url( + 'https://login.microsoftonline.com', 'tenant-1', 'client-1', 'http://localhost:1234', + 'https://management.azure.com/.default openid', 'st', 'chal', 'alice@contoso.com') + parsed = urllib.parse.urlparse(url) + query = urllib.parse.parse_qs(parsed.query) + self.assertEqual('/tenant-1/oauth2/v2.0/authorize', parsed.path) + self.assertEqual(['S256'], query['code_challenge_method']) + self.assertEqual(['chal'], query['code_challenge']) + self.assertEqual(['st'], query['state']) + self.assertEqual(['alice@contoso.com'], query['login_hint']) + + +class SignInTest(TestCase): + + def _sign_in(self, browser, token_response, login='alice@contoso.com'): + with mock.patch.object(entra_sign_in.requests, 'post', return_value=token_response) as post, \ + mock.patch('builtins.print'): + token = entra_sign_in.sign_in('tenant-1', 'client-1', 'https://management.azure.com/.default', + login_hint=login, timeout=10, open_browser=browser) + return token, post + + def test_success_redeems_code_with_verifier_and_no_secret(self): + browser = FakeBrowser(code='auth-code') + ok = _token_response(access_token='at', expires_in=3599, + id_token=_jwt(preferred_username='alice@contoso.com')) + + token, post = self._sign_in(browser, ok) + + self.assertEqual('at', token['access_token']) + body = post.call_args.kwargs['data'] + self.assertEqual('authorization_code', body['grant_type']) + self.assertEqual('auth-code', body['code']) + self.assertIn('code_verifier', body) + self.assertNotIn('client_secret', body) + self.assertTrue(post.call_args.args[0].endswith('/tenant-1/oauth2/v2.0/token')) + challenge = urllib.parse.parse_qs(browser.authorize_url.query)['code_challenge'][0] + expected = base64.urlsafe_b64encode(hashlib.sha256(body['code_verifier'].encode()).digest()).rstrip(b'=').decode() + self.assertEqual(expected, challenge) + + def test_other_user_is_refused(self): + ok = _token_response(access_token='at', id_token=_jwt(preferred_username='mallory@contoso.com')) + with self.assertRaises(CommandError) as ctx: + self._sign_in(FakeBrowser(code='c'), ok) + self.assertIn('different user', str(ctx.exception)) + + def test_token_without_identity_claims_is_refused(self): + with self.assertRaises(CommandError): + self._sign_in(FakeBrowser(code='c'), _token_response(access_token='opaque')) + + def test_user_cancelling_raises(self): + with self.assertRaises(CommandError) as ctx: + self._sign_in(FakeBrowser(error='access_denied', error_description='cancelled'), mock.Mock()) + self.assertIn('cancelled', str(ctx.exception)) + + def test_state_mismatch_is_refused(self): + browser = FakeBrowser(code='c', state='forged') + with self.assertRaises(CommandError) as ctx: + self._sign_in(browser, mock.Mock()) + self.assertIn('did not match', str(ctx.exception)) + + def test_timeout_raises(self): + with mock.patch('builtins.print'): + with self.assertRaises(CommandError) as ctx: + entra_sign_in.sign_in('t', 'c', 's', 'a@b.com', timeout=0, open_browser=lambda url: None) + self.assertIn('not completed in time', str(ctx.exception)) + + def test_rejected_token_request_raises_first_line_only(self): + rejected = _token_response(400, error='invalid_client', + error_description='AADSTS7000218: public client\r\nTrace ID: x') + with self.assertRaises(CommandError) as ctx: + self._sign_in(FakeBrowser(code='c'), rejected) + self.assertIn('AADSTS7000218', str(ctx.exception)) + self.assertNotIn('Trace ID', str(ctx.exception)) diff --git a/unit-tests/test_pam_cloud_credentials.py b/unit-tests/test_pam_cloud_credentials.py new file mode 100644 index 000000000..8bb4612d2 --- /dev/null +++ b/unit-tests/test_pam_cloud_credentials.py @@ -0,0 +1,106 @@ +import json +from unittest import TestCase, mock + +from keepercommander import crypto, vault +from keepercommander.commands.pam_cloud import pam_cloud_credentials as mod +from keepercommander.error import CommandError + + +class SignInLocallyTest(TestCase): + + def _config(self, tenant='tenant-1', client='client-1'): + record = vault.TypedRecord() + record.type_name = 'pamAzureConfiguration' + record.fields.append(vault.TypedField.new_field('secret', tenant, 'tenantId')) + record.fields.append(vault.TypedField.new_field('secret', client, 'clientId')) + return record + + def test_config_field_reads_by_label(self): + config = self._config() + self.assertEqual('tenant-1', mod._config_field(config, 'tenantId')) + self.assertEqual('', mod._config_field(config, 'missing')) + + def test_builds_record_from_token_with_tenant_wide_default_scope(self): + with mock.patch.object(mod.entra_sign_in, 'sign_in', + return_value={'access_token': 'at', 'expires_in': 3599}) as sign_in: + record, expiration = mod._sign_in_locally(self._config(), 'alice@contoso.com', None) + + args, kwargs = sign_in.call_args + self.assertEqual(('tenant-1', 'client-1', 'https://graph.microsoft.com/.default'), args) + self.assertEqual('alice@contoso.com', kwargs['login_hint']) + fields = {f.get('label') or f['type']: f['value'] for f in record['fields']} + self.assertEqual('alice@contoso.com', fields['login']) + self.assertEqual({'accessToken': 'at'}, json.loads(fields['privatePEMKey'])) + self.assertEqual('azure', fields['Provider']) + self.assertTrue(expiration) + + def test_client_id_override(self): + with mock.patch.object(mod.entra_sign_in, 'sign_in', return_value={'access_token': 'at'}) as sign_in: + mod._sign_in_locally(self._config(), 'alice@contoso.com', 'override-client') + + self.assertEqual(('tenant-1', 'override-client', 'https://graph.microsoft.com/.default'), + sign_in.call_args.args) + + def test_scope_override_narrows_the_token(self): + with mock.patch.object(mod.entra_sign_in, 'sign_in', return_value={'access_token': 'at'}) as sign_in: + mod._sign_in_locally(self._config(), 'alice@contoso.com', None, 'https://management.azure.com/.default') + + self.assertEqual('https://management.azure.com/.default', sign_in.call_args.args[2]) + + def test_missing_tenant_or_client_is_error(self): + with self.assertRaises(CommandError): + mod._sign_in_locally(self._config(client=''), 'alice@contoso.com', None) + + +class ExecuteTest(TestCase): + """The requester is always the user who gets the token.""" + + def _run(self, config_type, gateway_data=None, sent=None): + params = mock.Mock() + params.user = 'dev-admin@chan-demo.com' + params.record_cache = {'CFG': {'record_key_unencrypted': crypto.get_random_bytes(32)}} + resource = vault.TypedRecord() + resource.record_uid = 'RES' + resource.type_name = 'pamCloudResource' + config = vault.TypedRecord() + config.type_name = config_type + config.fields.append(vault.TypedField.new_field('secret', 'tenant-1', 'tenantId')) + config.fields.append(vault.TypedField.new_field('secret', 'client-1', 'clientId')) + sent = {} if sent is None else sent + + def dispatch(_params, action, **_kw): + sent.update(action.inputs) + return gateway_data or {'success': True, 'clientSideAuth': True} + + args = {'record': 'RES', 'configuration': None, 'client_id': None, 'scope': None, + 'duration_seconds': None, 'gateway': None, 'save_record': False} + with mock.patch.object(mod, 'resolve_pam_record', return_value=resource), \ + mock.patch.object(mod, '_resolve_config_uid', return_value='CFG'), \ + mock.patch.object(mod.vault.KeeperRecord, 'load', return_value=config), \ + mock.patch.object(mod, 'dispatch_streamed_access_elevation_action', side_effect=dispatch), \ + mock.patch.object(mod.entra_sign_in, 'sign_in', + return_value={'access_token': 'at', 'expires_in': 3599}) as sign_in, \ + mock.patch('builtins.print'): + mod.PAMCloudGenerateCredentialsCommand().execute(params, **args) + return sent, sign_in + + def test_azure_signs_the_requester_in_and_sends_no_password(self): + sent, sign_in = self._run('pamAzureConfiguration') + + self.assertTrue(sent['clientSideAuth']) + self.assertEqual('dev-admin@chan-demo.com', sent['username']) + self.assertNotIn('password', sent) + self.assertEqual('dev-admin@chan-demo.com', sign_in.call_args.kwargs['login_hint']) + + def test_non_azure_is_left_to_the_gateway(self): + sent = {} + with self.assertRaises(CommandError): + self._run('pamAwsConfiguration', gateway_data={'success': False, 'error': 'stop'}, sent=sent) + + self.assertNotIn('clientSideAuth', sent) + self.assertEqual('dev-admin@chan-demo.com', sent['username']) + + def test_gateway_error_is_raised(self): + with self.assertRaises(CommandError) as ctx: + self._run('pamAzureConfiguration', gateway_data={'success': False, 'error': 'nope'}) + self.assertIn('nope', str(ctx.exception)) From cf5871aceac2049e2dfaa085fd18773224d15a17 Mon Sep 17 00:00:00 2001 From: adeshmukh-ks Date: Thu, 8 Oct 2026 19:30:54 +0530 Subject: [PATCH 2/4] Fix inheritance issues with revoking parent folder access --- .../nested_share_folder/folder_commands.py | 3 + .../nested_share_folder/folder_api.py | 641 ++++++++++++------ keepercommander/nested_share_folder/sync.py | 152 +++++ unit-tests/test_nested_share_folder.py | 239 ++++++- 4 files changed, 814 insertions(+), 221 deletions(-) diff --git a/keepercommander/commands/nested_share_folder/folder_commands.py b/keepercommander/commands/nested_share_folder/folder_commands.py index 93c6b9c53..061bef518 100644 --- a/keepercommander/commands/nested_share_folder/folder_commands.py +++ b/keepercommander/commands/nested_share_folder/folder_commands.py @@ -529,6 +529,9 @@ def _apply(cls, params, action, folder_uid, recipient, role, expiration, taken = result.get('action_taken', verb) if taken == 'already_had_access': logging.info("%s '%s' already has access", kind, recipient) + elif taken in ('denied', 'already_denied'): + logging.info("%s '%s' inherited access denied on this folder", + kind, recipient) else: logging.info("%s share '%s' %s", kind, recipient, verb) else: diff --git a/keepercommander/nested_share_folder/folder_api.py b/keepercommander/nested_share_folder/folder_api.py index c49ad9c12..1aadbae34 100644 --- a/keepercommander/nested_share_folder/folder_api.py +++ b/keepercommander/nested_share_folder/folder_api.py @@ -27,6 +27,13 @@ FolderUsageType, SetBooleanValue, resolve_role_name, ROLE_NAME_MAP, get_folder_permissions_for_role, ) +from .sync import ( + plan_folder_access_change, folder_access_state_from_accessors, + get_folder_access_state, + FOLDER_ACCESS_NONE, FOLDER_ACCESS_INHERITED, FOLDER_ACCESS_DIRECT, + FOLDER_ACCESS_DENIED, + FOLDER_ACCESS_ADD, FOLDER_ACCESS_UPDATE, FOLDER_ACCESS_REMOVE, +) logger = logging.getLogger(__name__) @@ -320,10 +327,173 @@ def _apply_folder_expiration_tla(tla_props, expiration_timestamp, tla_props.rotateOnExpiration = True +# ── Access-state aware request planning ─────────────────────────────────── +# +# A child folder's accessor can be: +# * direct - its own access row (and folder-key edge) on this folder +# * inherited - access flowing from a parent folder; no folder-key edge here +# * denied - an inherited access explicitly denied on this folder +# * none - no access +# +# The backend refuses to change a folder key on another user's existing access +# row, so turning inherited access into an explicit role must be an *add* with +# the recipient-encrypted folder key, not an update. Otherwise the child loses +# its only key edge when the parent grant is revoked and disappears on sync. +# ``plan_folder_access_change`` (sync.py) owns the transition rules. + +_STEP_MESSAGES = { + FOLDER_ACCESS_ADD: 'Access granted successfully', + FOLDER_ACCESS_UPDATE: 'Access updated successfully', + FOLDER_ACCESS_REMOVE: 'Access revoked successfully', +} + + +def _lookup_folder_accessors(params, folder_uid, accessor_uid_b64, + access_type_label=None): + """Return every server accessor row for one actor on *folder_uid*. + + Rows matching *access_type_label* are preferred; otherwise all rows for + the UID are returned. Lookup failures return an empty list. + """ + try: + info = get_folder_access_v3(params, [folder_uid], resolve_usernames=False) + except Exception as exc: + logger.debug('Folder accessor lookup failed for %s: %s', folder_uid, exc) + return [] + rows = [] + for fr in info.get('results', []): + if not fr.get('success'): + continue + rows.extend(a for a in fr.get('accessors', []) + if a.get('accessor_uid') == accessor_uid_b64) + if access_type_label: + typed = [a for a in rows if a.get('access_type') == access_type_label] + if typed: + return typed + return rows + + +def _folder_access_state(params, folder_uid, accessor_uid_b64, accessors): + """Classify an accessor, preferring server rows over the sync cache. + + The sync cache only carries the caller's own row (plus denials seen in + sync), so it is a fallback for when the server returned nothing. + """ + if accessors: + return folder_access_state_from_accessors(accessors) + try: + return get_folder_access_state(params, folder_uid, accessor_uid_b64) + except Exception as exc: + logger.debug('Cached folder access state unavailable for %s: %s', folder_uid, exc) + return FOLDER_ACCESS_NONE + + +def _current_role_name(accessors): + """Role of the most specific accessor row (direct over inherited).""" + for a in sorted(accessors or [], key=lambda r: bool(r.get('inherited'))): + if a.get('role'): + return a['role'] + return None + + +def _encrypted_folder_key_for(params, folder_uid, accessor_uid_b64, as_team, + user_email=None, user_public_key=None, + use_ecc=False, team_keys=None): + """Encrypt *folder_uid*'s key for a user (public key) or team.""" + fk = get_folder_key(params, folder_uid) + ek = folder_pb2.EncryptedDataKey() + if as_team: + if team_keys is None: + team_keys = get_team_keys(params, accessor_uid_b64) + efk, key_type = encrypt_for_team( + fk, team_keys, prefer_aes=False, + forbid_rsa=getattr(params, 'forbid_rsa', False)) + else: + if not user_public_key: + if not user_email or '@' not in user_email: + _, user_email = resolve_uid_email(params, user_email or accessor_uid_b64) + user_public_key, use_ecc = load_user_public_key(params, user_email) + efk = encrypt_for_recipient(fk, user_public_key, use_ecc) + key_type = (folder_pb2.encrypted_by_public_key_ecc if use_ecc + else folder_pb2.encrypted_by_public_key) + ek.encryptedKey = efk + ek.encryptedKeyType = key_type + return ek + + +def _build_access_step_data(folder_uid, uid_bytes, access_type_enum, step, + role=None, hidden=None, expiration_timestamp=None, + rotate_on_expiration=False, folder_key=None): + """Build the FolderAccessData for one planned step.""" + ad = folder_pb2.FolderAccessData() + ad.folderUid = utils.base64_url_decode(folder_uid) + ad.accessTypeUid = uid_bytes + ad.accessType = access_type_enum + if step['request'] == FOLDER_ACCESS_REMOVE: + return ad + if step['denied_access']: + # A denial is not a direct grant: no role change and never a folder key. + ad.deniedAccess = True + return ad + if role: + access_role = resolve_role_name(role) + ad.accessRoleType = access_role + ad.permissions.CopyFrom(get_folder_permissions_for_role(access_role)) + if hidden is not None: + ad.hidden = hidden + _apply_folder_expiration_tla(ad.tlaProperties, expiration_timestamp, + rotate_on_expiration) + if step['include_folder_key']: + if folder_key is None: + raise ValueError('Folder key is required to create direct folder access') + ad.folderKey.CopyFrom(folder_key) + return ad + + +def _send_access_step(params, step, ad): + if step['request'] == FOLDER_ACCESS_ADD: + return folder_access_update_v3(params, folder_access_adds=[ad]) + if step['request'] == FOLDER_ACCESS_UPDATE: + return folder_access_update_v3(params, folder_access_updates=[ad]) + return folder_access_update_v3(params, folder_access_removes=[ad]) + + +def _execute_access_plan(params, folder_uid, identifier_label, uid_bytes, + access_type_enum, plan, folder_key_factory=None, + messages=None, **fields): + """Send *plan* steps sequentially, stopping at the first failure. + + Used for the two-step re-add of a denied accessor: the add is only sent + once the denial removal succeeded, so no add races an existing denied row. + """ + messages = dict(_STEP_MESSAGES, **(messages or {})) + result = None + for idx, step in enumerate(plan): + folder_key = folder_key_factory() if step['include_folder_key'] else None + ad = _build_access_step_data(folder_uid, uid_bytes, access_type_enum, step, + folder_key=folder_key, **fields) + message = ('Access denied successfully' if step['denied_access'] + else messages[step['request']]) + response = _send_access_step(params, step, ad) + result = parse_folder_access_result(response, folder_uid, identifier_label, message) + result['request'] = step['request'] + if not result.get('success'): + if idx < len(plan) - 1: + result['message'] = (f"{result.get('message')} " + f"(step {idx + 1} of {len(plan)}; remaining steps skipped)") + break + return result + + def grant_folder_access_v3(params, folder_uid, user_uid, role='viewer', share_folder_key=True, expiration_timestamp=None, as_team=False, rotate_on_expiration=False): """Grant a user *or team* access to a Nested Share Folder. + + The request depends on the accessor's current state on this folder: + existing direct access is updated (no new key); inherited access becomes a + direct grant via ``folderAccessAdds`` with the recipient-encrypted folder + key; a denied accessor has the denial removed first, then is added. """ resolved = resolve_folder_identifier(params, folder_uid) if not resolved: @@ -370,57 +540,52 @@ def grant_folder_access_v3(params, folder_uid, user_uid, role='viewer', access_role = resolve_role_name(role) target_role_name = folder_pb2.AccessRoleType.Name(access_role) + accessor_uid_b64 = utils.base64_url_encode(actual_uid_bytes) - if actual_uid_bytes: - 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 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, - expiration_timestamp=expiration_timestamp, - rotate_on_expiration=rotate_on_expiration) - result['action_taken'] = 'updated' - return result - - ad = folder_pb2.FolderAccessData() - ad.folderUid = utils.base64_url_decode(folder_uid) - ad.accessTypeUid = actual_uid_bytes - ad.accessType = access_type_enum - ad.accessRoleType = access_role - ad.permissions.CopyFrom(get_folder_permissions_for_role(access_role)) - - _apply_folder_expiration_tla( - ad.tlaProperties, expiration_timestamp, rotate_on_expiration) - - if share_folder_key: - fk = get_folder_key(params, folder_uid) - ek = folder_pb2.EncryptedDataKey() - if as_team: - efk, key_type = encrypt_for_team( - fk, team_keys, prefer_aes=False, - forbid_rsa=getattr(params, 'forbid_rsa', False)) - ek.encryptedKey = efk - ek.encryptedKeyType = key_type - else: - if not user_public_key: - user_public_key, use_ecc = load_user_public_key(params, user_email) - efk = encrypt_for_recipient(fk, user_public_key, use_ecc) - ek.encryptedKey = efk - ek.encryptedKeyType = (folder_pb2.encrypted_by_public_key_ecc if use_ecc - else folder_pb2.encrypted_by_public_key) - ad.folderKey.CopyFrom(ek) - - response = folder_access_update_v3(params, folder_access_adds=[ad]) - result = parse_folder_access_result(response, folder_uid, identifier_label, - 'Access granted successfully') + accessors = _lookup_folder_accessors(params, folder_uid, accessor_uid_b64, + access_type_label) + state = _folder_access_state(params, folder_uid, accessor_uid_b64, accessors) + + if state == FOLDER_ACCESS_DIRECT: + if _current_role_name(accessors) == 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, + expiration_timestamp=expiration_timestamp, + rotate_on_expiration=rotate_on_expiration, + known_state=FOLDER_ACCESS_DIRECT) + result['action_taken'] = 'updated' + return result + + plan = plan_folder_access_change(params, folder_uid, accessor_uid_b64, 'grant', + state=state) + if not share_folder_key: + if state in (FOLDER_ACCESS_INHERITED, FOLDER_ACCESS_DENIED): + raise ValueError('A folder key is required to give direct access on a ' + 'folder where the accessor has inherited or denied access') + plan = [dict(step, include_folder_key=False) for step in plan] + + def folder_key_factory(): + return _encrypted_folder_key_for( + params, folder_uid, accessor_uid_b64, as_team, + user_email=user_email, user_public_key=user_public_key, + use_ecc=use_ecc, team_keys=team_keys) + + result = _execute_access_plan( + params, folder_uid, identifier_label, actual_uid_bytes, access_type_enum, + plan, folder_key_factory=folder_key_factory, + role=role, expiration_timestamp=expiration_timestamp, + rotate_on_expiration=rotate_on_expiration) result['access_type'] = access_type_label - result.setdefault('action_taken', 'granted' if result['success'] else 'grant_failed') + result['previous_access_state'] = state + if result['success']: + result.setdefault('action_taken', 'granted') + else: + result.setdefault('action_taken', 'grant_failed') return result @@ -442,7 +607,14 @@ 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, expiration_timestamp=None, as_team=False, - rotate_on_expiration=False): + rotate_on_expiration=False, known_state=None): + """Change an accessor's role / hidden flag / expiration on a folder. + + Direct access is changed with ``folderAccessUpdates`` and keeps its + existing folder key. Inherited (or denied) access is turned into a direct + grant with ``folderAccessAdds`` plus the recipient-encrypted folder key, + so the child keeps its own key edge if parent access is later revoked. + """ 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) @@ -454,47 +626,48 @@ def update_folder_access_v3(params, folder_uid, user_uid, role=None, hidden=None params, user_uid, as_team) if not actual_uid_bytes: raise ValueError(f"{'Team' if as_team else 'User'} '{user_uid}' not found") + accessor_uid_b64 = utils.base64_url_encode(actual_uid_bytes) - ad = folder_pb2.FolderAccessData() - ad.folderUid = utils.base64_url_decode(folder_uid) - ad.accessTypeUid = actual_uid_bytes - ad.accessType = access_type_enum - if role: - resolved_role = resolve_role_name(role) - ad.accessRoleType = resolved_role - ad.permissions.CopyFrom(get_folder_permissions_for_role(resolved_role)) - if hidden is not None: - ad.hidden = hidden - _apply_folder_expiration_tla( - ad.tlaProperties, expiration_timestamp, rotate_on_expiration) - - response = folder_access_update_v3(params, folder_access_updates=[ad]) - result = parse_folder_access_result(response, folder_uid, identifier_label, - 'Access updated successfully') + accessors = [] + state = known_state + if state is None: + accessors = _lookup_folder_accessors( + params, folder_uid, accessor_uid_b64, + folder_pb2.AccessType.Name(access_type_enum)) + state = _folder_access_state(params, folder_uid, accessor_uid_b64, accessors) + + if state in (FOLDER_ACCESS_DIRECT, FOLDER_ACCESS_NONE): + # NONE: nothing known to convert; let the server validate the update. + plan = [{'request': FOLDER_ACCESS_UPDATE, 'include_folder_key': False, + 'denied_access': False}] + else: + plan = plan_folder_access_change(params, folder_uid, accessor_uid_b64, + 'grant', state=state) + if role is None: + # An add needs a role; keep the one currently in effect. + current = (_current_role_name(accessors) or '').lower() + if current not in ROLE_NAME_MAP: + raise ValueError( + 'A role is required to change inherited or denied access on ' + 'this folder (it becomes a direct grant)') + role = current + + def folder_key_factory(): + return _encrypted_folder_key_for( + params, folder_uid, accessor_uid_b64, as_team, + user_email=user_uid if '@' in (user_uid or '') else None) + + result = _execute_access_plan( + params, folder_uid, identifier_label, actual_uid_bytes, access_type_enum, + plan, folder_key_factory=folder_key_factory, + messages={FOLDER_ACCESS_ADD: 'Access updated successfully'}, + role=role, hidden=hidden, expiration_timestamp=expiration_timestamp, + rotate_on_expiration=rotate_on_expiration) result['access_type'] = 'AT_TEAM' if as_team else 'AT_USER' + result['previous_access_state'] = state return result -def _lookup_folder_accessor(params, folder_uid, accessor_uid_b64, access_type_label): - """Return a folder accessor row from ``get_folder_access_v3``, if present.""" - try: - info = get_folder_access_v3(params, [folder_uid], resolve_usernames=True) - for fr in info.get('results', []): - if not fr.get('success'): - continue - accessors = fr.get('accessors', []) - for accessor in accessors: - if (accessor.get('accessor_uid') == accessor_uid_b64 - and accessor.get('access_type') == access_type_label): - return accessor - for accessor in accessors: - if accessor.get('accessor_uid') == accessor_uid_b64: - return accessor - except Exception as exc: - logger.debug('Folder accessor lookup failed for %s: %s', folder_uid, exc) - return None - - def _folder_inherits_parent_permissions(params, folder_uid): """Return True when *folder_uid* has a parent and still inherits its access list.""" nsf_folders = getattr(params, 'nested_share_folders', {}) @@ -638,6 +811,13 @@ def revoke_folder_access_v3(params, folder_uid, user_uid, as_team=False): Looks up the accessor via ``get_folder_access_v3`` so sub-folders use the server-reported UID and access type (including inherited accessors). + + * Direct access is removed with ``folderAccessRemoves``. + * Inherited access cannot be removed on the child (it comes from the + parent), so it is denied with ``folderAccessUpdates`` + + ``deniedAccess=True`` and no folder key. + * Already-denied access needs no request. + On success, evicts the local access cache and sets ``params.sync_data``. """ resolved = resolve_folder_identifier(params, folder_uid) @@ -654,9 +834,10 @@ def revoke_folder_access_v3(params, folder_uid, user_uid, as_team=False): accessor_uid_b64 = utils.base64_url_encode(actual_uid_bytes) access_type_label = folder_pb2.AccessType.Name(access_type_enum) - accessor = _lookup_folder_accessor( + accessors = _lookup_folder_accessors( params, folder_uid, accessor_uid_b64, access_type_label) - if accessor: + if accessors: + accessor = accessors[0] server_uid = accessor.get('accessor_uid') if server_uid: actual_uid_bytes = utils.base64_url_decode(server_uid) @@ -671,145 +852,174 @@ def revoke_folder_access_v3(params, folder_uid, user_uid, as_team=False): f"Unrecognised access type '{server_type}' returned by server " f"for folder '{folder_uid}'") - ad = folder_pb2.FolderAccessData() - ad.folderUid = utils.base64_url_decode(folder_uid) - ad.accessTypeUid = actual_uid_bytes - ad.accessType = access_type_enum - - response = folder_access_update_v3(params, folder_access_removes=[ad]) - result = parse_folder_access_result(response, folder_uid, identifier_label, - 'Access revoked successfully') + state = _folder_access_state(params, folder_uid, accessor_uid_b64, accessors) + action = 'deny' if state == FOLDER_ACCESS_INHERITED else 'remove' + plan = plan_folder_access_change(params, folder_uid, accessor_uid_b64, action, + state=state) + if not plan: + return {'folder_uid': folder_uid, 'user_uid': identifier_label, + 'access_type': access_type_label, 'status': 'SUCCESS', + 'message': 'Access is already denied on this folder', + 'success': True, 'action_taken': 'already_denied', + 'previous_access_state': state} + + result = _execute_access_plan( + params, folder_uid, identifier_label, actual_uid_bytes, access_type_enum, plan) result['access_type'] = access_type_label + result['previous_access_state'] = state if result.get('success'): + if action == 'deny': + result['action_taken'] = 'denied' _evict_folder_accessor_cache(params, folder_uid, accessor_uid_b64) params.sync_data = True return result +def _batch_accessor_index(params, folder_uids): + """Map (folder_uid, accessor_uid_b64) -> server accessor rows for a batch.""" + index = {} + uids = sorted(set(folder_uids)) + for i in range(0, len(uids), 100): + try: + info = get_folder_access_v3(params, uids[i:i + 100], resolve_usernames=False) + except Exception as exc: + logger.debug('Batch folder accessor lookup failed: %s', exc) + continue + for fr in info.get('results', []): + if not fr.get('success'): + continue + for a in fr.get('accessors', []): + index.setdefault((fr['folder_uid'], a.get('accessor_uid')), []).append(a) + return index + + +def _batch_resolve_accessor(params, spec): + as_team = bool(spec.get('as_team')) + if as_team: + resolved_team = resolve_team_identifier(params, spec['user_uid']) + if not resolved_team: + raise ValueError(f"Team '{spec['user_uid']}' not found") + _, uid_bytes = resolved_team + return uid_bytes, folder_pb2.AT_TEAM, as_team + uid_bytes = resolve_user_uid_bytes(params, spec['user_uid']) + if not uid_bytes: + raise ValueError(f"User '{spec['user_uid']}' not found") + return uid_bytes, folder_pb2.AT_USER, as_team + + def manage_folder_access_batch_v3(params, access_grants=None, access_updates=None, access_revokes=None): """Apply a batch of folder access grants/updates/revokes. + + Each operation is planned from the accessor's current state (see + ``plan_folder_access_change``): + + * grant/update of inherited access -> ``folderAccessAdds`` + folder key + * grant/update of denied access -> denial removed in a first request, + then ``folderAccessAdds`` + folder key in the main request (only for + denials that were removed successfully) + * grant/update of direct access -> ``folderAccessUpdates`` (no key) + * revoke of inherited access -> ``folderAccessUpdates`` + deniedAccess + * revoke of direct access -> ``folderAccessRemoves`` """ - adds, updates, removes = [], [], [] - tracking = [] - forbid_rsa = getattr(params, 'forbid_rsa', False) - - for spec in (access_grants or []): - fuid = resolve_folder_identifier(params, spec['folder_uid']) - if not fuid: - raise ValueError(f"Folder '{spec['folder_uid']}' not found") - as_team = bool(spec.get('as_team')) - role = spec.get('role', 'viewer') - fk = get_folder_key(params, fuid) - - ad = folder_pb2.FolderAccessData() - ad.folderUid = utils.base64_url_decode(fuid) - ad.accessRoleType = resolve_role_name(role) - - ek = folder_pb2.EncryptedDataKey() - if as_team: - resolved_team = resolve_team_identifier(params, spec['user_uid']) - if not resolved_team: - raise ValueError(f"Team '{spec['user_uid']}' not found") - team_uid_b64, uid_bytes = resolved_team - team_keys = get_team_keys(params, team_uid_b64) - - efk, key_type = encrypt_for_team(fk, team_keys, prefer_aes=False, - forbid_rsa=forbid_rsa) - ek.encryptedKey = efk - ek.encryptedKeyType = key_type - ad.accessType = folder_pb2.AT_TEAM + ops = [] + for op, specs in (('grant', access_grants), ('update', access_updates), + ('revoke', access_revokes)): + for spec in (specs or []): + fuid = resolve_folder_identifier(params, spec['folder_uid']) + if not fuid: + raise ValueError(f"Folder '{spec['folder_uid']}' not found") + uid_bytes, access_type_enum, as_team = _batch_resolve_accessor(params, spec) + ops.append({'op': op, 'folder_uid': fuid, 'spec': spec, + 'uid_bytes': uid_bytes, + 'uid_b64': utils.base64_url_encode(uid_bytes), + 'access_type_enum': access_type_enum, 'as_team': as_team}) + + index = _batch_accessor_index(params, [o['folder_uid'] for o in ops]) + + pre_removes, adds, updates, removes = [], [], [], [] + for o in ops: + spec, fuid, uid_b64 = o['spec'], o['folder_uid'], o['uid_b64'] + rows = index.get((fuid, uid_b64), []) + state = _folder_access_state(params, fuid, uid_b64, rows) + o['state'] = state + role = spec.get('role') + + if o['op'] == 'revoke': + action = 'deny' if state == FOLDER_ACCESS_INHERITED else 'remove' + plan = plan_folder_access_change(params, fuid, uid_b64, action, state=state) + elif o['op'] == 'update' and state in (FOLDER_ACCESS_DIRECT, FOLDER_ACCESS_NONE): + plan = [{'request': FOLDER_ACCESS_UPDATE, 'include_folder_key': False, + 'denied_access': False}] else: - uid_bytes, email = resolve_uid_email(params, spec['user_uid']) - if not uid_bytes: - raise ValueError(f"User '{spec['user_uid']}' not found") - pk, use_ecc = load_user_public_key(params, email) - efk = encrypt_for_recipient(fk, pk, use_ecc) - ek.encryptedKey = efk - ek.encryptedKeyType = (folder_pb2.encrypted_by_public_key_ecc if use_ecc - else folder_pb2.encrypted_by_public_key) - ad.accessType = folder_pb2.AT_USER - - ad.accessTypeUid = uid_bytes - ad.folderKey.CopyFrom(ek) - adds.append(ad) - tracking.append(('grant', fuid, spec['user_uid'], spec, as_team)) - - for spec in (access_updates or []): - fuid = resolve_folder_identifier(params, spec['folder_uid']) - if not fuid: - raise ValueError(f"Folder '{spec['folder_uid']}' not found") - as_team = bool(spec.get('as_team')) - if as_team: - resolved_team = resolve_team_identifier(params, spec['user_uid']) - if not resolved_team: - raise ValueError(f"Team '{spec['user_uid']}' not found") - _, uid_bytes = resolved_team - access_type_enum = folder_pb2.AT_TEAM - else: - uid_bytes = resolve_user_uid_bytes(params, spec['user_uid']) - if not uid_bytes: - raise ValueError(f"User '{spec['user_uid']}' not found") - access_type_enum = folder_pb2.AT_USER - - ad = folder_pb2.FolderAccessData() - ad.folderUid = utils.base64_url_decode(fuid) - ad.accessTypeUid = uid_bytes - ad.accessType = access_type_enum - if spec.get('role'): - ad.accessRoleType = resolve_role_name(spec['role']) - if spec.get('hidden') is not None: - ad.hidden = spec['hidden'] - updates.append(ad) - tracking.append(('update', fuid, spec['user_uid'], spec, as_team)) - - for spec in (access_revokes or []): - fuid = resolve_folder_identifier(params, spec['folder_uid']) - if not fuid: - raise ValueError(f"Folder '{spec['folder_uid']}' not found") - as_team = bool(spec.get('as_team')) - if as_team: - resolved_team = resolve_team_identifier(params, spec['user_uid']) - if not resolved_team: - raise ValueError(f"Team '{spec['user_uid']}' not found") - _, uid_bytes = resolved_team - access_type_enum = folder_pb2.AT_TEAM - else: - uid_bytes = resolve_user_uid_bytes(params, spec['user_uid']) - if not uid_bytes: - raise ValueError(f"User '{spec['user_uid']}' not found") - access_type_enum = folder_pb2.AT_USER - - ad = folder_pb2.FolderAccessData() - ad.folderUid = utils.base64_url_decode(fuid) - ad.accessTypeUid = uid_bytes - ad.accessType = access_type_enum - removes.append(ad) - tracking.append(('revoke', fuid, spec['user_uid'], spec, as_team)) - - response = folder_access_update_v3( - params, - folder_access_adds=adds or None, - folder_access_updates=updates or None, - folder_access_removes=removes or None) - - results = [{'operation': op, 'folder_uid': f, 'user_uid': u, - 'access_type': 'AT_TEAM' if at else 'AT_USER', - 'status': 'SUCCESS', 'message': f'{op.capitalize()} completed', 'success': True} - for op, f, u, _, at in tracking] - - if response.folderAccessResults: - for r in response.folderAccessResults: - f = utils.base64_url_encode(r.folderUid) - u = utils.base64_url_encode(r.accessUid) if r.accessUid else 'unknown' - for i, (op, tf, tu, _, at) in enumerate(tracking): - if tf == f and tu == u: - results[i] = { - 'operation': op, 'folder_uid': f, 'user_uid': u, - 'access_type': 'AT_TEAM' if at else 'AT_USER', - 'status': folder_pb2.FolderModifyStatus.Name(r.status), - 'message': r.message, 'success': False} - break + plan = plan_folder_access_change(params, fuid, uid_b64, 'grant', state=state) + if o['op'] == 'grant': + role = role or 'viewer' + if plan[-1]['request'] == FOLDER_ACCESS_ADD and not role: + current = (_current_role_name(rows) or '').lower() + if current not in ROLE_NAME_MAP: + raise ValueError( + f"A role is required to change inherited or denied access for " + f"'{spec['user_uid']}' on folder '{fuid}'") + role = current + + o['steps'] = plan + for idx, step in enumerate(plan): + folder_key = None + if step['include_folder_key']: + folder_key = _encrypted_folder_key_for( + params, fuid, uid_b64, o['as_team'], + user_email=spec['user_uid'] if not o['as_team'] else None) + ad = _build_access_step_data( + fuid, o['uid_bytes'], o['access_type_enum'], step, + role=role, hidden=spec.get('hidden'), folder_key=folder_key) + if idx < len(plan) - 1: + pre_removes.append((o, ad)) # denial removal before the add + elif step['request'] == FOLDER_ACCESS_ADD: + adds.append((o, ad)) + elif step['request'] == FOLDER_ACCESS_UPDATE: + updates.append((o, ad)) + else: + removes.append((o, ad)) + + def _failures(response): + failed = {} + for r in (response.folderAccessResults or []): + if r.status == folder_pb2.SUCCESS: + continue + key = (utils.base64_url_encode(r.folderUid), + utils.base64_url_encode(r.accessUid) if r.accessUid else None) + failed[key] = (folder_pb2.FolderModifyStatus.Name(r.status), r.message) + return failed + + failed = {} + if pre_removes: + failed.update(_failures(folder_access_update_v3( + params, folder_access_removes=[ad for _, ad in pre_removes]))) + # Only add accessors whose denial was actually removed. + adds = [(o, ad) for o, ad in adds + if (o['folder_uid'], o['uid_b64']) not in failed] + + if adds or updates or removes: + failed.update(_failures(folder_access_update_v3( + params, + folder_access_adds=[ad for _, ad in adds] or None, + folder_access_updates=[ad for _, ad in updates] or None, + folder_access_removes=[ad for _, ad in removes] or None))) + + results = [] + for o in ops: + res = {'operation': o['op'], 'folder_uid': o['folder_uid'], + 'user_uid': o['spec']['user_uid'], + 'access_type': 'AT_TEAM' if o['as_team'] else 'AT_USER', + 'previous_access_state': o['state'], + 'status': 'SUCCESS', 'success': True, + 'message': (f"{o['op'].capitalize()} completed" if o['steps'] + else 'Access is already denied on this folder')} + err = failed.get((o['folder_uid'], o['uid_b64'])) + if err: + res.update(status=err[0], message=err[1], success=False) + results.append(res) return results @@ -939,6 +1149,7 @@ def get_folder_access_v3(params, folder_uids, continuation_token=None, ai = { 'accessor_uid': auid, 'access_type': at, 'role': rt, 'inherited': bool(a.inherited), 'hidden': bool(a.hidden), + 'denied_access': bool(getattr(a, 'deniedAccess', False)), 'username': username, 'date_created': a.dateCreated or None, 'last_modified': a.lastModified or None, diff --git a/keepercommander/nested_share_folder/sync.py b/keepercommander/nested_share_folder/sync.py index a0332717d..36754f71d 100644 --- a/keepercommander/nested_share_folder/sync.py +++ b/keepercommander/nested_share_folder/sync.py @@ -19,6 +19,12 @@ def _ensure_nested_share_folder_attrs(params): params.nested_share_folder_keys = {} if not hasattr(params, 'nested_share_folder_accesses'): params.nested_share_folder_accesses = {} + if not hasattr(params, 'nested_share_folder_denied_accesses'): + # {folder_uid: {actor_uid: {...}}} - accessors whose inherited access + # was denied on this folder. Denied rows are hidden from the participant + # list, but the share command needs to know they exist so that a re-add + # becomes folderAccessRemoves (denial) followed by folderAccessAdds. + params.nested_share_folder_denied_accesses = {} if not hasattr(params, 'nested_share_records'): params.nested_share_records = {} if not hasattr(params, 'nested_share_record_data'): @@ -73,6 +79,7 @@ def clear_caches(params): params.nested_share_folders.clear() params.nested_share_folder_keys.clear() params.nested_share_folder_accesses.clear() + params.nested_share_folder_denied_accesses.clear() params.nested_share_records.clear() params.nested_share_record_data.clear() params.nested_share_record_keys.clear() @@ -296,6 +303,7 @@ def _process_folder_accesses(params, folder_accesses): 'access_role_type': fa.accessRoleType if fa.accessRoleType else 0, 'inherited': fa.inherited if fa.inherited else False, 'hidden': fa.hidden if fa.hidden else False, + 'denied_access': bool(getattr(fa, 'deniedAccess', False)), 'date_created': fa.dateCreated if fa.dateCreated else 0, 'last_modified': fa.lastModified if fa.lastModified else 0, } @@ -327,9 +335,42 @@ def _process_folder_accesses(params, folder_accesses): 'can_list_records': p.canListRecords, 'can_list_folders': p.canListFolders, } + if fa_obj['denied_access']: + _remember_denied_folder_access(params, folder_uid, access_uid, fa.accessType) + continue + # A live (non-denied) row supersedes any previously cached denial. + _forget_denied_folder_access(params, folder_uid, access_uid) params.nested_share_folder_accesses[folder_uid].append(fa_obj) +def _remember_denied_folder_access(params, folder_uid, actor_uid, access_type=None): + # Deliberately not shaped like a folder access row (no 'access_type_uid'), + # so it can't be mistaken for a real grant. + params.nested_share_folder_denied_accesses.setdefault(folder_uid, {})[actor_uid] = { + 'denied': True, + 'actor_uid': actor_uid, + 'access_type': access_type, + } + + +def _forget_denied_folder_access(params, folder_uid, actor_uid): + """Drop a cached denial and, if none remain, clear the folder-level flag. + + The folder key popped by _process_denied_folder_accesses() is not restored + here: _decrypt_nested_share_folder_keys() runs at the end of every sync and + re-derives the key for any folder missing 'folder_key_unencrypted'. + """ + denied = params.nested_share_folder_denied_accesses.get(folder_uid) + if denied: + denied.pop(actor_uid, None) + if not denied: + params.nested_share_folder_denied_accesses.pop(folder_uid, None) + if not params.nested_share_folder_denied_accesses.get(folder_uid): + folder_obj = params.nested_share_folders.get(folder_uid) + if folder_obj is not None: + folder_obj.pop('denied', None) + + def _process_folder_sharing_states(params, folder_sharing_states): """Store the per-folder sharing state from sync-down. @@ -360,6 +401,8 @@ def _process_revoked_folder_accesses(params, revoked_folder_accesses): fa for fa in params.nested_share_folder_accesses[folder_uid] if fa['access_type_uid'] != actor_uid ] + # Revoking the access row also removes a denial row for that actor. + _forget_denied_folder_access(params, folder_uid, actor_uid) def _process_denied_folder_accesses(params, denied_folder_accesses): @@ -373,6 +416,8 @@ def _process_denied_folder_accesses(params, denied_folder_accesses): fa for fa in params.nested_share_folder_accesses[folder_uid] if fa['access_type_uid'] != actor_uid ] + _remember_denied_folder_access( + params, folder_uid, actor_uid, getattr(dfa, 'accessType', None)) if folder_uid in params.nested_share_folders: folder_obj = params.nested_share_folders[folder_uid] folder_obj.pop('folder_key_unencrypted', None) @@ -603,6 +648,7 @@ def _process_removed_folders(params, removed_folders): params.nested_share_folders.pop(folder_uid, None) params.nested_share_folder_keys.pop(folder_uid, None) params.nested_share_folder_accesses.pop(folder_uid, None) + params.nested_share_folder_denied_accesses.pop(folder_uid, None) params.nested_share_folder_sharing_states.pop(folder_uid, None) params.nested_share_folder_records.pop(folder_uid, None) params.subfolder_cache.pop(folder_uid, None) @@ -1207,3 +1253,109 @@ def _backfill_nsf_record_access_caches(params, record_uid, record_obj, rd_obj): is_owner, rd_obj['user_account_uid'] ) + + +# --------------------------------------------------------------------------- +# Folder access state helpers (used by the nested-share folder share command) +# --------------------------------------------------------------------------- + +FOLDER_ACCESS_NONE = 'none' +FOLDER_ACCESS_INHERITED = 'inherited' +FOLDER_ACCESS_DIRECT = 'direct' +FOLDER_ACCESS_DENIED = 'denied' + +# Request plan steps. Each step maps to one folder access request list. +FOLDER_ACCESS_ADD = 'folderAccessAdds' +FOLDER_ACCESS_UPDATE = 'folderAccessUpdates' +FOLDER_ACCESS_REMOVE = 'folderAccessRemoves' + + +def get_folder_access_state(params, folder_uid, actor_uid): + """Classify an accessor's relationship to a nested-share folder. + + Returns one of FOLDER_ACCESS_DENIED / DIRECT / INHERITED / NONE. + A denial wins over any inherited row for the same actor. + """ + _ensure_nested_share_folder_attrs(params) + if actor_uid in (params.nested_share_folder_denied_accesses.get(folder_uid) or {}): + return FOLDER_ACCESS_DENIED + rows = [fa for fa in (params.nested_share_folder_accesses.get(folder_uid) or []) + if fa.get('access_type_uid') == actor_uid] + if any(not fa.get('inherited') for fa in rows): + return FOLDER_ACCESS_DIRECT + if rows: + return FOLDER_ACCESS_INHERITED + return FOLDER_ACCESS_NONE + + +def folder_access_state_from_accessors(accessors): + """Classify server accessor rows (``get_folder_access_v3`` shape) for one actor. + + ``accessors`` is every row returned for the same actor on one folder (the + server may report an inherited row alongside a direct or denied one). + A denial wins, then any direct (non-inherited) row, then inherited. + """ + rows = [a for a in (accessors or []) if a] + if not rows: + return FOLDER_ACCESS_NONE + if any(a.get('denied_access') for a in rows): + return FOLDER_ACCESS_DENIED + if any(not a.get('inherited') for a in rows): + return FOLDER_ACCESS_DIRECT + return FOLDER_ACCESS_INHERITED + + +def plan_folder_access_change(params, folder_uid, actor_uid, action, state=None): + """Return the ordered request steps for a share change on a child folder. + + ``action`` is one of ``'grant'`` (set/change a role), ``'deny'`` or + ``'remove'``. Each step is a dict ``{'request': , + 'include_folder_key': bool, 'denied_access': bool}``. Steps must be sent + sequentially; a later step is sent only if the previous one succeeded. + + ``state`` should be supplied by callers that looked the accessor up on the + server (``folder_access_state_from_accessors``). The sync cache only holds + the caller's own access row, so for other accessors the cache-based + ``get_folder_access_state`` is only a fallback. + + Rules: + * inherited -> grant : folderAccessAdds + recipient-encrypted folderKey + (an update on an inherited row cannot carry a folder key for another + user, so the child would be orphaned when the parent is revoked). + * denied -> grant : folderAccessRemoves (denial), then + folderAccessAdds + folderKey. + * none -> grant : folderAccessAdds + folderKey. + * direct -> grant : folderAccessUpdates, no folderKey. + * inherited -> deny : folderAccessUpdates with deniedAccess=True, no folderKey. + * denied -> deny / remove : nothing to send. + * direct -> remove: folderAccessRemoves. + * none -> remove: folderAccessRemoves (let the server report it). + Raises ValueError for transitions that are not allowed. + """ + if state is None: + state = get_folder_access_state(params, folder_uid, actor_uid) + add_with_key = {'request': FOLDER_ACCESS_ADD, 'include_folder_key': True, 'denied_access': False} + remove = {'request': FOLDER_ACCESS_REMOVE, 'include_folder_key': False, 'denied_access': False} + + if action == 'grant': + if state == FOLDER_ACCESS_DIRECT: + return [{'request': FOLDER_ACCESS_UPDATE, 'include_folder_key': False, 'denied_access': False}] + if state == FOLDER_ACCESS_DENIED: + return [remove, add_with_key] + return [add_with_key] # inherited or none + + if action == 'deny': + if state == FOLDER_ACCESS_INHERITED: + return [{'request': FOLDER_ACCESS_UPDATE, 'include_folder_key': False, 'denied_access': True}] + if state == FOLDER_ACCESS_DENIED: + return [] + raise ValueError('Only inherited access can be denied (current state: %s)' % state) + + if action == 'remove': + if state in (FOLDER_ACCESS_DIRECT, FOLDER_ACCESS_NONE): + return [remove] + if state == FOLDER_ACCESS_DENIED: + return [] + raise ValueError('Inherited access cannot be removed directly; it comes from a parent folder') + + raise ValueError('Unknown folder access action: %s' % action) diff --git a/unit-tests/test_nested_share_folder.py b/unit-tests/test_nested_share_folder.py index 20ab55ff0..0c3fac318 100644 --- a/unit-tests/test_nested_share_folder.py +++ b/unit-tests/test_nested_share_folder.py @@ -44,6 +44,7 @@ def _make_params(**overrides): p.nested_share_folders = {} p.nested_share_folder_keys = {} p.nested_share_folder_accesses = {} + p.nested_share_folder_denied_accesses = {} p.nested_share_records = {} p.nested_share_record_data = {} p.nested_share_record_keys = {} @@ -62,6 +63,13 @@ def _make_params(**overrides): return p +def _direct_viewer_row(uid_bytes, inherited=False, denied=False, role='VIEWER', + access_type='AT_USER'): + return {'accessor_uid': utils.base64_url_encode(uid_bytes), + 'access_type': access_type, 'role': role, + 'inherited': inherited, 'denied_access': denied} + + def _make_folder(folder_uid=None, name='Test Folder', parent_uid=None, inherit_user_permissions=None): fuid = folder_uid or utils.generate_uid() @@ -1627,7 +1635,7 @@ def test_update_folder_access_v3_sets_expiration( 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._lookup_folder_accessors') @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( @@ -1640,7 +1648,7 @@ def test_grant_folder_access_update_passes_expiration( 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_existing.return_value = [_direct_viewer_row(uid_bytes)] mock_update.return_value = {'success': True} expiration = 1_800_000_000_000 @@ -1650,10 +1658,11 @@ def test_grant_folder_access_update_passes_expiration( mock_update.assert_called_once_with( mock.ANY, fuid, email, role='content-manager', as_team=False, - expiration_timestamp=expiration, rotate_on_expiration=False) + expiration_timestamp=expiration, rotate_on_expiration=False, + known_state='direct') @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._lookup_folder_accessors') @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( @@ -1666,7 +1675,7 @@ def test_grant_folder_access_same_role_updates_expiration( 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_existing.return_value = [_direct_viewer_row(uid_bytes)] mock_update.return_value = {'success': True} expiration = 1_900_000_000_000 @@ -1676,7 +1685,8 @@ def test_grant_folder_access_same_role_updates_expiration( mock_update.assert_called_once_with( mock.ANY, fuid, email, role='viewer', as_team=False, - expiration_timestamp=expiration, rotate_on_expiration=False) + expiration_timestamp=expiration, rotate_on_expiration=False, + known_state='direct') @patch('keepercommander.nested_share_folder.folder_api.folder_access_update_v3') @@ -2613,3 +2623,220 @@ def test_all_commands_registered(self): 'kd-record-access', 'kd-folder-access'] for name in removed: self.assertNotIn(name, commands) + + +# ══════════════════════════════════════════════════════════════════════════ +# Child-folder access transitions (inherited / denied / direct) +# ══════════════════════════════════════════════════════════════════════════ + +def _access_response(*statuses): + rs = Mock() + results = [] + for st in statuses: + r = Mock() + r.status = st + r.message = '' + r.folderUid = b'' + r.accessUid = b'' + results.append(r) + rs.folderAccessResults = results + return rs + + +_FA = 'keepercommander.nested_share_folder.folder_api.' + + +class TestNestedShareFolderAccessTransitions(TestCase): + + def setUp(self): + self.parent_uid, parent = _make_folder(name='Parent') + self.child_uid, child = _make_folder(name='Child', parent_uid=self.parent_uid) + self.params = _make_params(nested_share_folders={ + self.parent_uid: parent, self.child_uid: child}) + self.uid_bytes = utils.base64_url_decode(utils.generate_uid()) + self.email = 'user@example.com' + self.fake_key = folder_pb2.EncryptedDataKey( + encryptedKey=b'enc', encryptedKeyType=folder_pb2.encrypted_by_public_key) + + def _grant(self, rows, access_side_effect): + from keepercommander.nested_share_folder.folder_api import grant_folder_access_v3 + with patch(_FA + 'resolve_folder_identifier', return_value=self.child_uid), \ + patch(_FA + 'get_user_public_key', + return_value=(Mock(), False, self.uid_bytes, False)), \ + patch(_FA + '_lookup_folder_accessors', return_value=rows), \ + patch(_FA + '_encrypted_folder_key_for', return_value=self.fake_key) as enc, \ + patch(_FA + 'folder_access_update_v3', side_effect=access_side_effect) as upd: + result = grant_folder_access_v3(self.params, self.child_uid, self.email, + role='content-manager') + return result, upd, enc + + def test_grant_inherited_becomes_add_with_folder_key(self): + rows = [_direct_viewer_row(self.uid_bytes, inherited=True)] + result, upd, enc = self._grant(rows, [_access_response(folder_pb2.SUCCESS)]) + self.assertTrue(result['success']) + self.assertEqual(result['previous_access_state'], 'inherited') + upd.assert_called_once() + ad = upd.call_args.kwargs['folder_access_adds'][0] + self.assertTrue(ad.HasField('folderKey')) + self.assertEqual(ad.folderKey.encryptedKeyType, folder_pb2.encrypted_by_public_key) + self.assertNotIn('folder_access_updates', upd.call_args.kwargs) + enc.assert_called_once() + + def test_grant_inherited_same_role_still_creates_direct_grant(self): + rows = [_direct_viewer_row(self.uid_bytes, inherited=True, role='CONTENT_MANAGER')] + result, upd, _ = self._grant(rows, [_access_response(folder_pb2.SUCCESS)]) + self.assertNotEqual(result.get('action_taken'), 'already_had_access') + self.assertIn('folder_access_adds', upd.call_args.kwargs) + + def test_grant_denied_removes_denial_then_adds(self): + rows = [_direct_viewer_row(self.uid_bytes, inherited=True, denied=True)] + result, upd, _ = self._grant(rows, [_access_response(folder_pb2.SUCCESS), + _access_response(folder_pb2.SUCCESS)]) + self.assertTrue(result['success']) + self.assertEqual(upd.call_count, 2) + first, second = upd.call_args_list + self.assertIn('folder_access_removes', first.kwargs) + self.assertFalse(first.kwargs['folder_access_removes'][0].HasField('folderKey')) + add = second.kwargs['folder_access_adds'][0] + self.assertTrue(add.HasField('folderKey')) + self.assertFalse(add.deniedAccess) + + def test_grant_denied_skips_add_when_removal_fails(self): + rows = [_direct_viewer_row(self.uid_bytes, denied=True)] + failure = next(v for k, v in folder_pb2.FolderModifyStatus.items() if v != folder_pb2.SUCCESS) + result, upd, _ = self._grant(rows, [_access_response(failure)]) + self.assertFalse(result['success']) + upd.assert_called_once() + self.assertIn('folder_access_removes', upd.call_args.kwargs) + + def test_grant_direct_uses_update_without_key(self): + from keepercommander.nested_share_folder.folder_api import grant_folder_access_v3 + rows = [_direct_viewer_row(self.uid_bytes)] + with patch(_FA + 'resolve_folder_identifier', return_value=self.child_uid), \ + patch(_FA + 'get_user_public_key', + return_value=(Mock(), False, self.uid_bytes, False)), \ + patch(_FA + '_lookup_folder_accessors', return_value=rows), \ + patch(_FA + '_resolve_accessor', + return_value=(self.uid_bytes, self.email, folder_pb2.AT_USER)), \ + patch(_FA + '_encrypted_folder_key_for') as enc, \ + patch(_FA + 'folder_access_update_v3', + return_value=_access_response(folder_pb2.SUCCESS)) as upd: + result = grant_folder_access_v3(self.params, self.child_uid, self.email, + role='content-manager') + self.assertEqual(result['action_taken'], 'updated') + ad = upd.call_args.kwargs['folder_access_updates'][0] + self.assertFalse(ad.HasField('folderKey')) + enc.assert_not_called() + + def test_update_inherited_keeps_current_role_and_adds_key(self): + from keepercommander.nested_share_folder.folder_api import update_folder_access_v3 + rows = [_direct_viewer_row(self.uid_bytes, inherited=True)] + with patch(_FA + 'resolve_folder_identifier', return_value=self.child_uid), \ + patch(_FA + '_resolve_accessor', + return_value=(self.uid_bytes, self.email, folder_pb2.AT_USER)), \ + patch(_FA + '_lookup_folder_accessors', return_value=rows), \ + patch(_FA + '_encrypted_folder_key_for', return_value=self.fake_key), \ + patch(_FA + 'folder_access_update_v3', + return_value=_access_response(folder_pb2.SUCCESS)) as upd: + update_folder_access_v3(self.params, self.child_uid, self.email, + expiration_timestamp=1_900_000_000_000) + ad = upd.call_args.kwargs['folder_access_adds'][0] + self.assertEqual(ad.accessRoleType, folder_pb2.VIEWER) + self.assertTrue(ad.HasField('folderKey')) + self.assertEqual(ad.tlaProperties.expiration, 1_900_000_000_000) + + def _revoke(self, rows, response=None): + with patch(_FA + 'resolve_folder_identifier', return_value=self.child_uid), \ + patch(_FA + '_resolve_accessor', + return_value=(self.uid_bytes, self.email, folder_pb2.AT_USER)), \ + patch(_FA + '_lookup_folder_accessors', return_value=rows), \ + patch(_FA + 'folder_access_update_v3', + return_value=response or _access_response(folder_pb2.SUCCESS)) as upd: + result = revoke_folder_access_v3(self.params, self.child_uid, self.email) + return result, upd + + def test_revoke_inherited_sends_denial_without_key(self): + result, upd = self._revoke([_direct_viewer_row(self.uid_bytes, inherited=True)]) + self.assertEqual(result['action_taken'], 'denied') + ad = upd.call_args.kwargs['folder_access_updates'][0] + self.assertTrue(ad.deniedAccess) + self.assertFalse(ad.HasField('folderKey')) + + def test_revoke_direct_sends_remove(self): + _, upd = self._revoke([_direct_viewer_row(self.uid_bytes)]) + self.assertIn('folder_access_removes', upd.call_args.kwargs) + + def test_revoke_already_denied_sends_nothing(self): + result, upd = self._revoke([_direct_viewer_row(self.uid_bytes, denied=True)]) + self.assertTrue(result['success']) + self.assertEqual(result['action_taken'], 'already_denied') + upd.assert_not_called() + + def test_batch_mixed_states(self): + from keepercommander.nested_share_folder.folder_api import manage_folder_access_batch_v3 + inh, den, direct = (utils.generate_uid() for _ in range(3)) + index = { + (self.child_uid, inh): [{'accessor_uid': inh, 'access_type': 'AT_USER', + 'role': 'VIEWER', 'inherited': True}], + (self.child_uid, den): [{'accessor_uid': den, 'access_type': 'AT_USER', + 'role': 'VIEWER', 'inherited': True, + 'denied_access': True}], + (self.child_uid, direct): [{'accessor_uid': direct, 'access_type': 'AT_USER', + 'role': 'VIEWER', 'inherited': False}], + } + with patch(_FA + 'resolve_folder_identifier', return_value=self.child_uid), \ + patch(_FA + 'resolve_user_uid_bytes', + side_effect=lambda p, u: utils.base64_url_decode(u)), \ + patch(_FA + '_batch_accessor_index', return_value=index), \ + patch(_FA + '_encrypted_folder_key_for', return_value=self.fake_key), \ + patch(_FA + 'folder_access_update_v3', + return_value=_access_response()) as upd: + results = manage_folder_access_batch_v3( + self.params, + access_grants=[{'folder_uid': self.child_uid, 'user_uid': den, + 'role': 'viewer'}], + access_updates=[{'folder_uid': self.child_uid, 'user_uid': inh, + 'role': 'content-manager'}, + {'folder_uid': self.child_uid, 'user_uid': direct, + 'role': 'content-manager'}]) + self.assertTrue(all(r['success'] for r in results)) + self.assertEqual(upd.call_count, 2) + pre, main = upd.call_args_list + self.assertEqual(len(pre.kwargs['folder_access_removes']), 1) + adds = main.kwargs['folder_access_adds'] + self.assertEqual({utils.base64_url_encode(a.accessTypeUid) for a in adds}, {inh, den}) + self.assertTrue(all(a.HasField('folderKey') for a in adds)) + upd_rows = main.kwargs['folder_access_updates'] + self.assertEqual([utils.base64_url_encode(a.accessTypeUid) for a in upd_rows], [direct]) + self.assertFalse(upd_rows[0].HasField('folderKey')) + + +class TestPlanFolderAccessChange(TestCase): + + def test_state_from_accessors(self): + from keepercommander.nested_share_folder.sync import folder_access_state_from_accessors as f + self.assertEqual(f([]), 'none') + self.assertEqual(f([{'inherited': True}]), 'inherited') + self.assertEqual(f([{'inherited': True}, {'inherited': False}]), 'direct') + self.assertEqual(f([{'inherited': False}, {'denied_access': True}]), 'denied') + + def test_transitions(self): + from keepercommander.nested_share_folder.sync import plan_folder_access_change as plan + p = _make_params() + reqs = lambda st, act: [s['request'] for s in plan(p, 'f', 'a', act, state=st)] + self.assertEqual(reqs('inherited', 'grant'), ['folderAccessAdds']) + self.assertEqual(reqs('none', 'grant'), ['folderAccessAdds']) + self.assertEqual(reqs('denied', 'grant'), ['folderAccessRemoves', 'folderAccessAdds']) + self.assertEqual(reqs('direct', 'grant'), ['folderAccessUpdates']) + self.assertEqual(reqs('inherited', 'deny'), ['folderAccessUpdates']) + self.assertEqual(reqs('denied', 'remove'), []) + self.assertEqual(reqs('direct', 'remove'), ['folderAccessRemoves']) + self.assertTrue(plan(p, 'f', 'a', 'grant', state='inherited')[0]['include_folder_key']) + self.assertFalse(plan(p, 'f', 'a', 'grant', state='direct')[0]['include_folder_key']) + deny = plan(p, 'f', 'a', 'deny', state='inherited')[0] + self.assertTrue(deny['denied_access']) + self.assertFalse(deny['include_folder_key']) + with self.assertRaises(ValueError): + plan(p, 'f', 'a', 'remove', state='inherited') + with self.assertRaises(ValueError): + plan(p, 'f', 'a', 'deny', state='direct') From e6677ec6688ef05d3da43284e22e1c2ffefb8026 Mon Sep 17 00:00:00 2001 From: amangalampalli-ks Date: Thu, 8 Oct 2026 20:20:23 +0530 Subject: [PATCH 3/4] KC-1494: Add Service Mode JSON output in export and pam action debug dump (#2422) * Add Service Mode JSON output in export and pam action debug dump * Fix claude review points --- .../commands/pam_debug/__init__.py | 11 +- keepercommander/commands/pam_debug/dump.py | 195 ++++++++++++++++-- keepercommander/importer/commands.py | 46 +++-- keepercommander/importer/imp_exp.py | 62 +++++- keepercommander/service/util/command_util.py | 142 ++++++++++++- keepercommander/service/util/config_reader.py | 7 +- .../service/util/parse_keeper_response.py | 12 +- .../service/util/protected_records.py | 77 +++++-- .../service/util/verified_command.py | 101 ++++++++- unit-tests/pam/test_pam_debug_nsf.py | 154 +++++++++++++- unit-tests/service/test_command.py | 111 +++++++++- unit-tests/service/test_protected_records.py | 48 +++++ unit-tests/service/test_verified_command.py | 62 +++++- unit-tests/test_importer.py | 137 +++++++++++- 14 files changed, 1101 insertions(+), 64 deletions(-) diff --git a/keepercommander/commands/pam_debug/__init__.py b/keepercommander/commands/pam_debug/__init__.py index 14e1e17e7..6cb54c904 100644 --- a/keepercommander/commands/pam_debug/__init__.py +++ b/keepercommander/commands/pam_debug/__init__.py @@ -4,6 +4,7 @@ from typing import TYPE_CHECKING from ..pam_import.record_loader import load_pam_record # noqa: F401 — re-export for pam_debug modules +from ...error import CommandError if TYPE_CHECKING: from ...params import KeeperParams @@ -11,7 +12,13 @@ def get_connection(params: KeeperParams) -> ConnectionBase: - if value_to_boolean(os.environ.get("USE_LOCAL_DAG", False)) is False: + use_local_dag = value_to_boolean(os.environ.get("USE_LOCAL_DAG", False)) + if use_local_dag and getattr(params, 'service_mode', False): + raise CommandError( + 'pam action debug dump', + 'The local DAG engine is not available through Service Mode.', + ) + if use_local_dag is False: from ...keeper_dag.connection.commander import Connection as CommanderConnection # New per-graph endpoints (`/api/user/graph-sync//`) are # protobuf-only — JSON reads on those routes return empty. Match the @@ -21,4 +28,4 @@ def get_connection(params: KeeperParams) -> ConnectionBase: use_write_protobuf=True) else: from ...keeper_dag.connection.local import Connection as LocalConnection - return LocalConnection() \ No newline at end of file + return LocalConnection() diff --git a/keepercommander/commands/pam_debug/dump.py b/keepercommander/commands/pam_debug/dump.py index ed6a202de..b63209eea 100644 --- a/keepercommander/commands/pam_debug/dump.py +++ b/keepercommander/commands/pam_debug/dump.py @@ -5,7 +5,8 @@ import json import logging import pathlib -from typing import TYPE_CHECKING +import sys +from typing import TYPE_CHECKING, Iterable from ..base import Command, FolderMixin from ...subfolder import get_folder_uids @@ -16,6 +17,7 @@ from ..pam.vault_target import resolve_pam_folder_uid from ..pam_import.keeper_ai_settings import get_resource_settings from ..pam_import.nsf_helpers import get_folder_record_uids +from ...error import CommandError from ...keeper_dag.crypto import decrypt_aes from . import get_connection, load_pam_record @@ -25,18 +27,24 @@ ALL_GRAPH_IDS = [g.value for g in PamGraphId] +MAX_SERVICE_MODE_DUMP_RECORDS = 1000 +MAX_SERVICE_MODE_DUMP_CONFIGS = 20 # DELETION means the edge is absent; UNDENIAL cancels a DENIAL (treated as absent) _EXCLUDE_EDGE_TYPES = frozenset({EdgeType.DELETION, EdgeType.UNDENIAL}) class PAMDebugDumpCommand(Command): - parser = argparse.ArgumentParser(prog=':') + parser = argparse.ArgumentParser( + prog='pam action debug dump', + description='Dump folder records and GraphSync data as JSON.') parser.add_argument('folder_uid', action='store', help='Folder UID or path. Use empty string for the root folder.') parser.add_argument('--recursive', '-r', required=False, dest='recursive', action='store_true', help='Include records in all subfolders.') - parser.add_argument('--save-as', '-s', required=True, dest='save_as', action='store', + parser.add_argument('--format', choices=['json'], + help='Emit JSON to stdout (or the Service Mode API response).') + parser.add_argument('--save-as', '-s', required=False, dest='save_as', action='store', help='Output file path to save JSON results.') def get_parser(self): @@ -46,8 +54,59 @@ def execute(self, params: 'KeeperParams', **kwargs): folder_uid_arg = kwargs.get('folder_uid', '') recursive = kwargs.get('recursive', False) save_as = kwargs.get('save_as') + output_format = kwargs.get('format') + + from ...importer.imp_exp import is_export_restricted + if is_export_restricted(params): + raise CommandError( + 'pam action debug dump', + 'PAM debug dump is disabled by enterprise export restrictions.', + ) + + if getattr(params, 'service_mode', False): + if save_as is not None or output_format != 'json': + raise CommandError( + 'pam action debug dump', + 'Service Mode allows only JSON output in the API response; ' + 'do not specify --save-as/-s.', + ) + folder_cache = getattr(params, 'folder_cache', {}) or {} + nsf_folders = getattr(params, 'nested_share_folders', {}) or {} + if folder_uid_arg not in folder_cache and folder_uid_arg not in nsf_folders: + raise CommandError( + 'pam action debug dump', + 'Service Mode requires a visible folder UID.', + ) + elif save_as is None and output_format != 'json': + raise CommandError( + 'pam action debug dump', + 'Specify --save-as/-s FILE or --format=json.', + ) + + def _write_result(rows: Iterable[dict]) -> None: + def _write_json_array(stream) -> int: + encoder = json.JSONEncoder(indent=2) + count = 0 + stream.write('[') + for row in rows: + stream.write('\n' if count == 0 else ',\n') + at_line_start = True + for chunk in encoder.iterencode(row): + for part in chunk.splitlines(keepends=True): + if at_line_start: + stream.write(' ') + stream.write(part) + at_line_start = part.endswith('\n') + count += 1 + if count: + stream.write('\n') + stream.write(']\n') + return count + + if save_as is None: + _write_json_array(sys.stdout) + return - def _write_result(data: list) -> None: p = pathlib.Path(save_as) if p.exists(): counter = 1 @@ -57,8 +116,9 @@ def _write_result(data: list) -> None: p = candidate break counter += 1 + data = list(rows) with open(p, 'w', encoding='utf-8') as fh: - fh.write(json.dumps(data, indent=2)) + json.dump(data, fh, indent=2) logging.info('Saved %d record(s) to %s', len(data), p) # 1. Resolve folder UID(s) from UID or path (classic + NSF) @@ -125,12 +185,22 @@ def _on_folder(f): _write_result([]) return + if (getattr(params, 'service_mode', False) + and len(record_folder_map) > MAX_SERVICE_MODE_DUMP_RECORDS): + raise CommandError( + 'pam action debug dump', + f'Service Mode PAM debug dumps are limited to ' + f'{MAX_SERVICE_MODE_DUMP_RECORDS} folder records. ' + 'Use a smaller folder scope or omit --recursive.', + ) + # 3. Filter by version, then group valid records by config_uid. # Supported versions: 3 (typed), 5 (KSM App/Gateway), 6 (PAM Configuration). # Versions 1–2/4 are legacy/attachment records; skip with a warning. config_to_records: dict[str, list[str]] = {} - record_config_map: dict[str, str | None] = {} + record_configs: dict[str, set[str]] = {} valid_uids: list[str] = [] # passed version filter, in discovery order + unavailable_record_uids: list[str] = [] for rec_uid in record_folder_map: rec = get_record_from_cache(params, rec_uid) @@ -138,6 +208,7 @@ def _on_folder(f): loaded = load_pam_record(params, rec_uid) if loaded is None: logging.warning('skipping record %s version unknown - not in record cache', rec_uid) + unavailable_record_uids.append(rec_uid) continue version = getattr(loaded, 'version', None) rec = {'version': version, 'revision': 0, 'shared': False} @@ -156,7 +227,7 @@ def _on_folder(f): # v6 PAM Configuration records ARE their own graph root - no rotation-cache entry exists for them. if version == 6: config_to_records.setdefault(rec_uid, []).append(rec_uid) - record_config_map[rec_uid] = rec_uid + record_configs.setdefault(rec_uid, set()).add(rec_uid) continue rotation = params.record_rotation_cache.get(rec_uid) @@ -179,26 +250,49 @@ def _on_folder(f): if config_uid: config_to_records.setdefault(config_uid, []).append(rec_uid) - record_config_map[rec_uid] = config_uid + record_configs.setdefault(rec_uid, set()).add(config_uid) continue logging.debug('Record %s: no rotation entry, no local vault config, ' 'no krouter leafs match; graph data unavailable.', rec_uid) - record_config_map[rec_uid] = None + record_configs.setdefault(rec_uid, set()) + + unavailable_folders = sorted({ + record_folder_map[uid][0] for uid in unavailable_record_uids + }) if not valid_uids: - _write_result([]) + if getattr(params, 'service_mode', False): + _write_result(_build_unavailable_rows(unavailable_folders)) + else: + _write_result([]) return + if (getattr(params, 'service_mode', False) + and len(config_to_records) > MAX_SERVICE_MODE_DUMP_CONFIGS): + raise CommandError( + 'pam action debug dump', + f'Service Mode PAM debug dumps are limited to ' + f'{MAX_SERVICE_MODE_DUMP_CONFIGS} PAM configurations. ' + 'Use a smaller folder scope or omit --recursive.', + ) + # 4. Load all 5 DAGs once per config_uid # keyed by (config_uid, graph_id) dag_cache: dict[tuple[str, int], 'DAGType' | None] = {} + graph_load_errors: dict[tuple[str, int], dict] = {} + config_load_errors: dict[str, dict] = {} conn = get_connection(params) for config_uid in config_to_records: config_record = load_pam_record(params, config_uid) if config_record is None: logging.error('Configuration record %s not found; skipping graph load.', config_uid) + config_load_errors[config_uid] = { + 'stage': 'graph_sync', + 'config_uid': config_uid, + 'message': 'PAM configuration record could not be loaded.', + } for graph_id in ALL_GRAPH_IDS: dag_cache[(config_uid, graph_id)] = None continue @@ -214,11 +308,15 @@ def _on_folder(f): except Exception as err: logging.error('Failed to load graph %d for config %s: %s', graph_id, config_uid, err) dag_cache[(config_uid, graph_id)] = None + graph_load_errors[(config_uid, graph_id)] = { + 'stage': 'graph_sync', + 'config_uid': config_uid, + 'graph': PamGraphId(graph_id).name, + 'message': 'Graph data could not be loaded.', + } # 5. Build per-record output - result = [] - - for rec_uid in valid_uids: + def _build_row(rec_uid: str) -> dict: folder_uid, folder_parent_uid = record_folder_map[rec_uid] rec = get_record_from_cache(params, rec_uid) or {} nsf_meta = getattr(params, 'nested_share_records', {}).get(rec_uid) or {} @@ -243,6 +341,8 @@ def _on_folder(f): # data - same structure as `get --format=json` (classic + NSF) data = {} + record_errors = [] + loaded = None try: raw = rec.get('data_unencrypted') if raw: @@ -264,8 +364,17 @@ def _on_folder(f): notes = getattr(loaded, 'notes', None) if loaded else None if notes: data['notes'] = notes + if not data and loaded is None: + record_errors.append({ + 'stage': 'record_data', + 'message': 'Record data could not be loaded.', + }) except Exception as err: logging.warning('Could not build data for record %s: %s', rec_uid, err) + record_errors.append({ + 'stage': 'record_data', + 'message': 'Record data could not be built.', + }) # graph_sync - dict keyed by config_uid, then by graph name. # A record may be referenced by more than one PAM Configuration; we query @@ -275,8 +384,20 @@ def _on_folder(f): # "edges": [...] - present only when there are active, non-deleted edges # Config/graph keys are omitted when the record has no presence there. graph_sync: dict[str, dict[str, dict]] = {} + configs_for_record = record_configs.get(rec_uid, set()) + if not configs_for_record: + record_errors.append({ + 'stage': 'graph_sync', + 'message': 'No PAM configuration could be resolved; graph data is unavailable.', + }) + for (c_uid, graph_id), dag in dag_cache.items(): if dag is None: + if c_uid in configs_for_record: + error = (config_load_errors.get(c_uid) + or graph_load_errors.get((c_uid, graph_id))) + if error and error not in record_errors: + record_errors.append(error) continue try: graph_entry = _collect_graph_entry(dag, rec_uid, params, c_uid) @@ -286,15 +407,53 @@ def _on_folder(f): except Exception as err: logging.warning('Error collecting graph data for record %s graph %d config %s: %s', rec_uid, graph_id, c_uid, err) - - result.append({ + record_errors.append({ + 'stage': 'graph_sync', + 'config_uid': c_uid, + 'graph': PamGraphId(graph_id).name, + 'message': 'Graph data could not be collected for this record.', + }) + + row = { 'uid': rec_uid, 'metadata': metadata, 'data': data, 'graph_sync': graph_sync, - }) - - _write_result(result) + } + if record_errors: + row['errors'] = record_errors + return row + + def _iter_rows(): + for uid in valid_uids: + yield _build_row(uid) + if getattr(params, 'service_mode', False): + yield from _build_unavailable_rows(unavailable_folders) + + _write_result(_iter_rows()) + + +def _build_unavailable_rows(folder_uids: Iterable[str]): + """Return anonymous error rows so Service Mode never echoes a hidden UID.""" + for folder_uid in folder_uids: + yield { + 'uid': None, + 'metadata': { + 'uid': None, + 'folder_uid': folder_uid, + 'folder_uid_parent': None, + 'version': None, + 'shared': None, + 'client_modified_time': None, + 'revision': None, + }, + 'data': {}, + 'graph_sync': {}, + 'errors': [{ + 'stage': 'record_data', + 'message': 'One or more folder records could not be loaded.', + }], + } def _collect_graph_entry(dag: 'DAGType', record_uid: str, params: 'KeeperParams', diff --git a/keepercommander/importer/commands.py b/keepercommander/importer/commands.py index 4e605c461..dd3400c7d 100644 --- a/keepercommander/importer/commands.py +++ b/keepercommander/importer/commands.py @@ -23,6 +23,7 @@ from .json.json import KeeperJsonImporter, KeeperJsonExporter from ..commands.base import dump_report_data, raise_parse_exception, suppress_exit, user_choice, Command from ..commands.enterprise_common import EnterpriseCommand +from ..error import CommandError from ..params import KeeperParams from ..proto import record_pb2 @@ -134,7 +135,8 @@ def _positive_int_arg(value): import_parser.exit = suppress_exit -export_parser = argparse.ArgumentParser(prog='export', description='Export vault data from Keeper to a local file') +export_parser = argparse.ArgumentParser( + prog='export', description='Export vault data from Keeper to a file or standard output') export_parser.add_argument('--format', dest='format', choices=['json', 'csv', 'keepass'], required=True, help='file format') export_parser.add_argument('--max-size', dest='max_size', @@ -146,7 +148,7 @@ def _positive_int_arg(value): export_parser.add_argument('--zip', dest='zip_archive', action='store_true', help='Create ZIP archive for file attachments. JSON only') export_parser.add_argument('--owned-only', dest='owned_only', action='store_true', - help='Only export owned records') + help='Only export owned records') export_parser.add_argument('--save-in-vault', dest='save_in_vault', action='store_true', help='Stores exports file as a record attachment. KeePass only') export_parser.add_argument('--force', dest='force', action='store_true', help='Suppress user interaction. Assume "yes"') @@ -385,7 +387,12 @@ def get_parser(self): def execute(self, params, **kwargs): - if is_export_restricted(params): + if imp_exp.is_export_restricted(params): + if getattr(params, 'service_mode', False): + raise CommandError( + 'export', + 'Export is disabled by enterprise export restrictions.', + ) logging.warning('Permissions Required: `export` command is disabled. ' 'Please contact your enterprise administrator.') return @@ -394,6 +401,26 @@ def execute(self, params, **kwargs): export_name = kwargs.pop('name', None) save_in_file = kwargs.pop('save_in_file', None) + if getattr(params, 'service_mode', False): + has_file_options = ( + kwargs.get('max_size') is not None + or kwargs.get('file_password') is not None + or kwargs.get('kbdx_key_file') is not None + or kwargs.get('zip_archive') is True + or kwargs.get('save_in_vault') is True + or kwargs.get('force') is True + or save_in_file is not None + ) + if export_format != 'json' or export_name is not None or has_file_options: + raise CommandError( + 'export', + 'Service Mode permits only JSON export to the API response. ' + 'Use export --format=json with optional --folder and --owned-only; ' + 'do not specify an output filename or other options.', + ) + kwargs['_service_mode_record_limit'] = imp_exp.SERVICE_MODE_EXPORT_MAX_RECORDS + kwargs['_service_mode_input_limit'] = imp_exp.SERVICE_MODE_EXPORT_MAX_INPUT_BYTES + if export_format: msize = kwargs.pop('max_size', None) if isinstance(msize, str): @@ -422,17 +449,8 @@ def execute(self, params, **kwargs): def is_export_restricted(params): - is_export_restricted = False - - booleans = params.enforcements['booleans'] if params.enforcements and 'booleans' in params.enforcements else [] - - if len(booleans) > 0: - restrict_export_boolean = next((s for s in booleans if s['key'] == 'restrict_export'), None) - - if restrict_export_boolean: - is_export_restricted = restrict_export_boolean['value'] - - return is_export_restricted + """Backward-compatible alias for the shared export policy check.""" + return imp_exp.is_export_restricted(params) def set_permission(perm, user, permit, restrict, perm_name): diff --git a/keepercommander/importer/imp_exp.py b/keepercommander/importer/imp_exp.py index e37a9542e..f6c67317d 100644 --- a/keepercommander/importer/imp_exp.py +++ b/keepercommander/importer/imp_exp.py @@ -57,6 +57,8 @@ RECORD_MAX_DATA_WARN = 'Skipping record "{}": Data size of {} exceeds limit of {}' LARGE_FIELD_MSG = 'This field is stored as attachment "{}" to avoid 2Mb record limit' FILE_ATTACHMENT_CHUNK = 100 +SERVICE_MODE_EXPORT_MAX_RECORDS = 5000 +SERVICE_MODE_EXPORT_MAX_INPUT_BYTES = 5 * 1024 * 1024 STANDARD_RECORD_TYPES = { @@ -273,9 +275,35 @@ def load_existing_nsf_record_for_match(params, record_uid): }) +def is_export_restricted(params): + """Return whether the enterprise policy disables vault data exports.""" + enforcements = getattr(params, 'enforcements', None) or {} + if not isinstance(enforcements, dict): + return False + booleans = enforcements.get('booleans') or [] + if not isinstance(booleans, (list, tuple)): + return False + return any( + isinstance(enforcement, dict) + and enforcement.get('key') == 'restrict_export' + and bool(enforcement.get('value')) + for enforcement in booleans + ) + + +def _byte_length(value): + if isinstance(value, bytes): + return len(value) + if isinstance(value, str): + return len(value.encode('utf-8', errors='replace')) + return 0 + + def export(params, file_format, filename, **kwargs): # type: (KeeperParams, str, str, ...) -> None """Export data from Vault to a file in an assortment of formats.""" + service_mode_record_limit = kwargs.pop('_service_mode_record_limit', None) + service_mode_input_limit = kwargs.pop('_service_mode_input_limit', None) sync_down.sync_down(params) exporter = exporter_for_format(file_format)() # type: BaseExporter @@ -361,7 +389,19 @@ def on_folder(base_folder): # type: (BaseFolderNode) -> None # for record_uid in params.record_cache.keys(): # ext_id += 1 # external_ids[record_uid] = ext_id - record_list = params.record_cache if not kwargs.get('owned_only') else {uid:params.record_cache[uid] for uid in params.record_owner_cache if params.record_owner_cache[uid].owner is True} + if kwargs.get('owned_only'): + owner_cache = getattr(params, 'record_owner_cache', {}) or {} + # Iterate the records still available to this command, rather than + # owner metadata alone. Service Mode hides its protected records from + # record_cache while the ownership metadata may still contain them. + record_list = { + uid: record for uid, record in params.record_cache.items() + if getattr(owner_cache.get(uid), 'owner', False) is True + } + else: + record_list = params.record_cache + service_mode_record_count = 0 + service_mode_input_bytes = 0 for record_uid in record_list: if record_filter or folder_path: if record_uid not in record_filter: @@ -370,6 +410,26 @@ def on_folder(base_folder): # type: (BaseFolderNode) -> None record = record_list[record_uid] record_version = record.get('version') or 0 if record_version == 2 or record_version == 3: + if service_mode_record_limit is not None or service_mode_input_limit is not None: + record_data = record.get('data_unencrypted') or b'' + record_extra = record.get('extra_unencrypted') or b'' + + service_mode_record_count += 1 + service_mode_input_bytes += _byte_length(record_data) + _byte_length(record_extra) + if (service_mode_record_limit is not None + and service_mode_record_count > service_mode_record_limit): + raise CommandError( + 'export', + 'Service Mode JSON export exceeds its record processing limit. ' + 'Use --folder or --owned-only to reduce the export scope.', + ) + if (service_mode_input_limit is not None + and service_mode_input_bytes > service_mode_input_limit): + raise CommandError( + 'export', + 'Service Mode JSON export exceeds its input data processing limit. ' + 'Use --folder or --owned-only to reduce the export scope.', + ) try: rec = convert_keeper_record(record, exporter.has_attachments()) if not rec: diff --git a/keepercommander/service/util/command_util.py b/keepercommander/service/util/command_util.py index e5f91c195..d38ca9d2b 100644 --- a/keepercommander/service/util/command_util.py +++ b/keepercommander/service/util/command_util.py @@ -36,10 +36,91 @@ from ..decorators.logging import logger, debug_decorator, sanitize_debug_data, sanitize_command_fields from ... import cli, utils from ...crypto import encrypt_aes_v2 -from ...error import KeeperApiError +from ...error import CommandError, KeeperApiError + + +MAX_SERVICE_MODE_JSON_RESPONSE_BYTES = 10 * 1024 * 1024 +MAX_SERVICE_MODE_JSON_LOG_BYTES = 1024 * 1024 + + +def is_pam_debug_dump_command(command_tokens) -> bool: + """Return True for `pam action|a debug dump|d` token sequences.""" + return ( + len(command_tokens) >= 4 + and command_tokens[0].lower() == 'pam' + and command_tokens[1].lower() in ('action', 'a') + and command_tokens[2].lower() == 'debug' + and command_tokens[3].lower() in ('dump', 'd') + ) + + +class ServiceModeOutputLimitExceeded(Exception): + """Raised when a bulk JSON command would exceed its response-size budget.""" + + +class _OutputByteBudget: + def __init__(self, max_bytes: int): + self.max_bytes = max_bytes + self.bytes_written = 0 + self.exceeded = False + + def consume(self, value: str): + if self.exceeded: + raise ServiceModeOutputLimitExceeded + byte_count = len(value.encode('utf-8', errors='replace')) + if self.bytes_written + byte_count > self.max_bytes: + self.exceeded = True + raise ServiceModeOutputLimitExceeded + self.bytes_written += byte_count + + +class _BoundedStringIO(io.StringIO): + """StringIO that rejects writes exceeding a UTF-8 byte limit.""" + + def __init__(self, budget: _OutputByteBudget): + super().__init__() + self.budget = budget + + def write(self, value: str) -> int: + self.budget.consume(value) + return super().write(value) + + +class _TruncatedStringIO(io.StringIO): + """Keep a bounded diagnostic log buffer without interrupting command work.""" + + def __init__(self, max_bytes: int): + super().__init__() + self.max_bytes = max_bytes + self.bytes_written = 0 + self.truncated = False + + def write(self, value: str) -> int: + if self.truncated: + return len(value) + byte_count = len(value.encode('utf-8', errors='replace')) + if self.bytes_written + byte_count > self.max_bytes: + self.truncated = True + return len(value) + self.bytes_written += byte_count + return super().write(value) class CommandExecutor: + @staticmethod + def _is_bulk_json_command(command: str) -> bool: + """Whether this command returns a potentially large JSON document.""" + try: + tokens = Verifycommand.tokenize_service_command(command) + except ValueError: + return False + + if not tokens: + return False + if tokens[0].lower() == 'export': + return True + return is_pam_debug_dump_command(tokens) + @staticmethod @debug_decorator def validate_command(command: str) -> Optional[Tuple[dict, int]]: @@ -58,10 +139,22 @@ def validate_session() -> Optional[Tuple[dict, int]]: @staticmethod @debug_decorator def capture_output_and_logs(params: Any, command: str) -> Tuple[Any, str, str]: - """Capture both stdout/stderr and logging output from command execution.""" - captured_stdout = io.StringIO() - captured_stderr = io.StringIO() - captured_logs = io.StringIO() + """Capture command output, bounding bulk JSON responses in Service Mode.""" + max_response_bytes = ( + MAX_SERVICE_MODE_JSON_RESPONSE_BYTES + if CommandExecutor._is_bulk_json_command(command) + else None + ) + if max_response_bytes is not None: + response_budget = _OutputByteBudget(max_response_bytes) + captured_stdout = _BoundedStringIO(response_budget) + captured_stderr = _BoundedStringIO(response_budget) + captured_logs = _TruncatedStringIO(MAX_SERVICE_MODE_JSON_LOG_BYTES) + else: + response_budget = None + captured_stdout = io.StringIO() + captured_stderr = io.StringIO() + captured_logs = io.StringIO() # Create a temporary log handler to capture command logs temp_handler = logging.StreamHandler(captured_logs) @@ -84,6 +177,8 @@ def capture_output_and_logs(params: Any, command: str) -> Tuple[Any, str, str]: root_logger.setLevel(logging.INFO) return_value = cli.do_command(params, command) + if response_budget is not None and response_budget.exceeded: + raise ServiceModeOutputLimitExceeded stdout_content = captured_stdout.getvalue() stderr_content = captured_stderr.getvalue() @@ -98,8 +193,12 @@ def capture_output_and_logs(params: Any, command: str) -> Tuple[Any, str, str]: combined_output = stderr_clean + '\n' + stdout_clean else: combined_output = stderr_clean or stdout_clean + if getattr(captured_logs, 'truncated', False): + log_clean = f'{log_clean}\n[Service Mode diagnostic logs truncated]'.strip() return return_value, combined_output, log_clean + except ServiceModeOutputLimitExceeded: + raise except Exception as e: # If there's an exception, capture any error output stderr_content = captured_stderr.getvalue() @@ -161,6 +260,7 @@ def execute(cls, command: str, temp_files: Optional[list] = None) -> Tuple[Any, return validation_error from ..core.globals import ensure_params_loaded + params = None try: params = ensure_params_loaded() # Set service mode flag to bypass master password enforcement @@ -268,14 +368,46 @@ def blocked(error): return CommandExecutor.encrypt_response(err), 500 response = CommandExecutor.encrypt_response(response) + if cls._is_bulk_json_command(command): + response_size = ( + len(response) + if isinstance(response, bytes) + else len(json.dumps(response).encode('utf-8')) + ) + if response_size > MAX_SERVICE_MODE_JSON_RESPONSE_BYTES: + return { + "status": "error", + "error": ( + 'Command response exceeds the 10 MiB Service Mode limit ' + 'and was discarded. Reduce the requested scope and retry.' + ), + }, 413 logger.debug(f"Command executed successfully") return response, status_code + except ServiceModeOutputLimitExceeded: + return { + "status": "error", + "error": ( + 'Command response exceeds the 10 MiB Service Mode limit and was discarded. ' + 'Reduce the requested scope using supported command filters and retry.' + ), + }, 413 except CommandExecutionError as e: # Return the actual command error instead of generic "server busy" logger.error(f"Command execution error: {sanitize_debug_data(str(e))}") if is_throttle_error(e): return throttle_error_response(str(e)) return {"status": "error", "error": str(e)}, 400 + except CommandError as e: + if (params is not None + and getattr(params, 'service_mode', False) + and cls._is_bulk_json_command(command)): + logger.warning( + f"Service Mode command rejected: {sanitize_debug_data(str(e))}" + ) + return {"status": "error", "error": str(e)}, 400 + logger.error(f"Unexpected error during command execution: {sanitize_debug_data(str(e))}") + return {"status": "error", "error": f"Unexpected error: {str(e)}"}, 500 except KeeperApiError as e: if is_throttle_error(e): return throttle_error_response(e.message or str(e), e.result_code) diff --git a/keepercommander/service/util/config_reader.py b/keepercommander/service/util/config_reader.py index 7819ee986..544c22111 100644 --- a/keepercommander/service/util/config_reader.py +++ b/keepercommander/service/util/config_reader.py @@ -10,7 +10,6 @@ # from typing import Any -from ..decorators.logging import logger class ConfigReader: _service_config = None @@ -26,6 +25,10 @@ def _get_service_config(cls) -> Any: @classmethod def read_config(cls, service_config_param: str, api_key: str = "") -> Any: """Read configuration parameter from service config file.""" + # Import lazily: decorators.__init__ imports auth, which imports this + # module. A module-level import here would create a ConfigReader cycle. + from ..decorators.logging import logger + service_config = cls._get_service_config() try: @@ -72,4 +75,4 @@ def read_config(cls, service_config_param: str, api_key: str = "") -> Any: except (FileNotFoundError, Exception) as e: default_value = 5000 if service_config_param == 'port' else "" logger.debug(f"Using default value for {service_config_param}: {default_value}") - return default_value \ No newline at end of file + return default_value diff --git a/keepercommander/service/util/parse_keeper_response.py b/keepercommander/service/util/parse_keeper_response.py index a9c2c44c8..9a226cc30 100644 --- a/keepercommander/service/util/parse_keeper_response.py +++ b/keepercommander/service/util/parse_keeper_response.py @@ -832,8 +832,18 @@ def _parse_json_format_command(command: str, response: str) -> Dict[str, Any]: Returns: Dict[str, Any]: Structured response with standard format """ + # Export may have safe selector flags before the format flag + # (e.g. `export --owned-only --format=json`); keep its command name + # stable instead of returning the preceding options as the command. + from .command_util import is_pam_debug_dump_command + command_parts = command.split() + command_name = command_parts[:1] + if command_name and command_name[0].lower() == 'export': + base_command = 'export' + elif is_pam_debug_dump_command(command_parts): + base_command = 'pam action debug dump' # Extract base command by removing --format=json or --format json - if ' --format=json' in command: + elif ' --format=json' in command: base_command = command.split(' --format=json')[0] elif ' --format json' in command: base_command = command.split(' --format json')[0] diff --git a/keepercommander/service/util/protected_records.py b/keepercommander/service/util/protected_records.py index 9d3bbbced..8bbff542e 100644 --- a/keepercommander/service/util/protected_records.py +++ b/keepercommander/service/util/protected_records.py @@ -34,7 +34,9 @@ # Raw + derived folder caches every folder-resolving command reads through, # directly or via subfolder.try_resolve_path/get_folder_uids. -_GUARDED_FOLDER_CACHE_ATTRS = ('folder_cache', 'shared_folder_cache', 'subfolder_cache') +_GUARDED_FOLDER_CACHE_ATTRS = ( + 'folder_cache', 'shared_folder_cache', 'subfolder_cache', 'nested_share_folders', +) # Commander's own session (config.json) and Service Mode's own runtime (service_config.json) # config files -- always attached under these exact, hardcoded names, never user-choosable. @@ -105,20 +107,36 @@ def get_protected_record_uids(params) -> Dict[str, str]: continue found[uid] = label - if params is None or not isinstance(getattr(params, 'record_cache', None), dict) or not params.record_cache: + if params is None: return found from ... import vault + from ...commands.pam_import.record_loader import ( + load_pam_record, + ) protected_titles = get_protected_record_title_set() - # One load per record_cache entry, no more -- a FileRecord attachment target is itself an - # entry in this same cache, so its name is picked up by this same pass rather than a second, - # per-attachment load + # Scan classic and NSF caches. A FileRecord attachment target is itself an + # accessible UID, so its name is picked up in the same pass. reserved_file_uids: Set[str] = set() pending_attachments: Dict[str, list] = {} - for uid in params.record_cache: + record_cache = getattr(params, 'record_cache', None) + record_uids = list(record_cache) if isinstance(record_cache, dict) else [] + seen_record_uids = set(record_uids) + for attr in ('nested_share_records', 'nested_share_record_data'): + cache = getattr(params, attr, None) + if isinstance(cache, dict): + for uid in cache: + if uid not in seen_record_uids: + seen_record_uids.add(uid) + record_uids.append(uid) + + for uid in record_uids: try: - record = vault.KeeperRecord.load(params, uid) + if isinstance(record_cache, dict) and uid in record_cache: + record = vault.KeeperRecord.load(params, uid) + else: + record = load_pam_record(params, uid) except Exception as e: logger.debug(f'protected_records: could not load record {uid} ({type(e).__name__}); skipping') continue @@ -130,7 +148,7 @@ def get_protected_record_uids(params) -> Dict[str, str]: reserved_file_uids.add(uid) continue - if record.title.lower() in protected_titles: + if (record.title or '').lower() in protected_titles: found[uid] = record.title elif _has_reserved_legacy_attachment(record): found[uid] = '' @@ -150,16 +168,22 @@ def get_protected_record_uids(params) -> Dict[str, str]: def get_protected_folder_uids(params, protected_record_uids: Dict[str, str]) -> Set[str]: - """Folders directly containing an already-protected record, via subfolder_record_cache (folder_uid -> set of record UIDs) -- derived from record protection rather than a separate per-integration title list, so it stays correct even if a folder is renamed.""" - subfolder_record_cache = getattr(params, 'subfolder_record_cache', None) - if params is None or not protected_record_uids or not isinstance(subfolder_record_cache, dict): + """Folders directly containing protected records, in classic vault or NSF maps.""" + if params is None or not protected_record_uids: return set() record_uids = protected_record_uids.keys() - return { - folder_uid for folder_uid, uids in subfolder_record_cache.items() - if folder_uid and isinstance(uids, (set, frozenset)) and uids & record_uids - } + protected_folders = set() + for attr in ('subfolder_record_cache', 'nested_share_folder_records'): + folder_records = getattr(params, attr, None) + if not isinstance(folder_records, dict): + continue + protected_folders.update( + folder_uid for folder_uid, uids in folder_records.items() + if folder_uid and isinstance(uids, (set, frozenset, list, tuple)) + and set(uids) & record_uids + ) + return protected_folders def _sync_down_exempt_commands() -> Dict[str, str]: @@ -227,6 +251,17 @@ def hide_from_record_cache(params, protected_uids: Dict[str, str]): removed_from_folders[folder_uid] = hit uids -= hit + nsf_folder_records = getattr(params, 'nested_share_folder_records', None) + removed_from_nsf_folders: Dict[str, set] = {} + if isinstance(nsf_folder_records, dict): + for folder_uid, uids in nsf_folder_records.items(): + if not isinstance(uids, set): + continue + hit = uids & protected_uid_set + if hit: + removed_from_nsf_folders[folder_uid] = hit + uids -= hit + try: yield finally: @@ -253,6 +288,18 @@ def hide_from_record_cache(params, protected_uids: Dict[str, str]): except Exception as e: logger.debug(f'hide_from_record_cache: failed to restore subfolder {folder_uid} ({type(e).__name__})') + if isinstance(nsf_folder_records, dict): + for folder_uid, hit in removed_from_nsf_folders.items(): + try: + uids = nsf_folder_records.get(folder_uid) + if isinstance(uids, set): + uids |= hit + except Exception as e: + logger.debug( + f'hide_from_record_cache: failed to restore nested shared folder ' + f'{folder_uid} ({type(e).__name__})' + ) + @contextlib.contextmanager def hide_from_folder_cache(params, protected_folder_uids: Set[str]): diff --git a/keepercommander/service/util/verified_command.py b/keepercommander/service/util/verified_command.py index 5fe9d9d0a..71c77e033 100644 --- a/keepercommander/service/util/verified_command.py +++ b/keepercommander/service/util/verified_command.py @@ -1,3 +1,4 @@ +import argparse import contextlib import io import os @@ -50,16 +51,28 @@ class Verifycommand: # writes a host file and is NOT enumerated here is allowed by default. # Adding a new command with local file I/O? Add it here, or it silently # bypasses Service Mode's "no host filesystem access" boundary. + # `export` is excluded because its JSON-to-response form has a separate, + # strict allowlist validator; every other export form is rejected there. # # Commands that always read/write host files (no safe Service Mode form). _HOST_FS_COMMANDS = frozenset({ 'run-batch', 'run', - 'export', 'download-membership', 'download-record-types', 'apply-membership', 'load-record-types', }) + _EXPORT_SERVICE_MODE_MSG = ( + 'Local filesystem access is not permitted through Service Mode. ' + "Only JSON export to the API response is allowed; use 'export --format=json' " + '(optional: --folder , --owned-only). ' + 'Do not specify an output filename or any other export options.' + ) + _PAM_DEBUG_DUMP_SERVICE_MODE_MSG = ( + 'Service Mode allows PAM debug dump only as JSON in the API response. ' + "Use 'pam action debug dump --format=json' " + '(optionally with --recursive); file output is not permitted.' + ) # Positional file input; FILEDATA is rewritten to a temp path before execute. _FILE_INPUT_COMMANDS = frozenset({'import', 'enterprise-push'}) # import --format values that name an account/API/URL source, not a local @@ -102,6 +115,8 @@ def validate_service_mode_restrictions(command_tokens, request_temp_dir=None): Verifycommand.validate_service_mode_download_attachment_command, Verifycommand.validate_service_mode_upload_attachment_command, Verifycommand.validate_service_mode_record_file_attachment_command, + Verifycommand.validate_service_mode_export_command, + Verifycommand.validate_service_mode_pam_debug_dump_command, Verifycommand.validate_service_mode_host_filesystem_command, Verifycommand.validate_service_mode_host_path_args, Verifycommand.validate_service_mode_file_input_command, @@ -257,6 +272,90 @@ def validate_service_mode_host_filesystem_command(command_tokens, request_temp_d return None return Verifycommand._HOST_FS_MSG + @staticmethod + def validate_service_mode_export_command(command_tokens, request_temp_dir=None): + """Allow only JSON export to captured output, with a tightly scoped option set. + + The regular export command supports local output files, ZIP archives, and + other file-oriented options. Service Mode accepts a deliberately smaller + grammar so it can only use the JSON exporter with no output filename. + """ + if not command_tokens or command_tokens[0].lower() != 'export': + return None + + parser = argparse.ArgumentParser(prog='export', add_help=False, allow_abbrev=False) + parser.add_argument('--format', choices=('json',), required=True) + parser.add_argument('--folder') + parser.add_argument('--owned-only', action='store_true') + + try: + with contextlib.redirect_stderr(io.StringIO()): + parsed = parser.parse_args(command_tokens[1:]) + except SystemExit: + return Verifycommand._EXPORT_SERVICE_MODE_MSG + except Exception: + return Verifycommand._EXPORT_SERVICE_MODE_MSG + + # argparse accepts repeated options and keeps the last value. Fail closed + # rather than permit ambiguous combinations in this security-sensitive + # command. Both --option value and --option=value are accepted. + seen_options = set() + value_options = {'--format', '--folder'} + i = 1 + while i < len(command_tokens): + token = command_tokens[i] + option = token.partition('=')[0] + if option not in {'--format', '--folder', '--owned-only'}: + return Verifycommand._EXPORT_SERVICE_MODE_MSG + if option in seen_options: + return Verifycommand._EXPORT_SERVICE_MODE_MSG + seen_options.add(option) + if option in value_options and '=' not in token: + i += 2 + else: + i += 1 + + if parsed.folder is not None and not parsed.folder.strip(): + return Verifycommand._EXPORT_SERVICE_MODE_MSG + return None + + @staticmethod + def validate_service_mode_pam_debug_dump_command(command_tokens, request_temp_dir=None): + """Allow the folder-scoped PAM debug dump only when it emits JSON to stdout.""" + from .command_util import is_pam_debug_dump_command + if not is_pam_debug_dump_command(command_tokens or []): + return None + + parser = argparse.ArgumentParser( + prog='pam action debug dump', add_help=False, allow_abbrev=False) + parser.add_argument('folder_uid') + parser.add_argument('--recursive', '-r', action='store_true') + parser.add_argument('--format', choices=('json',), required=True) + try: + with contextlib.redirect_stderr(io.StringIO()): + parsed = parser.parse_args(command_tokens[4:]) + except SystemExit: + return Verifycommand._PAM_DEBUG_DUMP_SERVICE_MODE_MSG + except Exception: + return Verifycommand._PAM_DEBUG_DUMP_SERVICE_MODE_MSG + + if not parsed.folder_uid.strip(): + return Verifycommand._PAM_DEBUG_DUMP_SERVICE_MODE_MSG + + # Do not let argparse's last-option-wins behavior make repeated flags + # ambiguous. Positional values and any non-allowlisted options have + # already been rejected by parse_args. + seen_options = set() + for token in command_tokens[4:]: + option = token.partition('=')[0] + if option in ('--format', '--recursive', '-r'): + if option in seen_options: + return Verifycommand._PAM_DEBUG_DUMP_SERVICE_MODE_MSG + seen_options.add(option) + if '--recursive' in seen_options and '-r' in seen_options: + return Verifycommand._PAM_DEBUG_DUMP_SERVICE_MODE_MSG + return None + @staticmethod def validate_service_mode_host_path_args(command_tokens, request_temp_dir=None): """Block host-path flags (--output, --filename, …); error or None.""" diff --git a/unit-tests/pam/test_pam_debug_nsf.py b/unit-tests/pam/test_pam_debug_nsf.py index edbe9d3ee..6572709f0 100644 --- a/unit-tests/pam/test_pam_debug_nsf.py +++ b/unit-tests/pam/test_pam_debug_nsf.py @@ -1,4 +1,5 @@ import json +import io import tempfile import unittest from types import SimpleNamespace @@ -8,8 +9,9 @@ from keepercommander import utils, vault from keepercommander.commands.discover import GatewayContext -from keepercommander.commands.pam_debug import load_pam_record +from keepercommander.commands.pam_debug import get_connection, load_pam_record from keepercommander.commands.pam_debug.dump import PAMDebugDumpCommand +from keepercommander.error import CommandError from keepercommander.subfolder import NestedShareFolderNode, RootFolderNode # Note: test_acl_uses_load_pam_record_for_nsf_uids() and test_link_uses_load_pam_record_for_nsf_resource() @@ -96,6 +98,156 @@ def test_dump_collects_nsf_folder_records(self): titles = {row['data'].get('title') for row in data} self.assertEqual(titles, {'NSF Machine', 'NSF User'}) + def test_dump_json_stdout_does_not_write_a_file(self): + params = _params() + params.service_mode = True + output = io.StringIO() + with patch('keepercommander.commands.pam_debug.dump.get_connection'), \ + patch('keepercommander.commands.pam_debug.dump.DAG'), \ + patch('sys.stdout', output), \ + patch('builtins.open', side_effect=AssertionError('response mode must not open a file')): + PAMDebugDumpCommand().execute( + params, + folder_uid='nsf_folder', + recursive=True, + format='json', + ) + + data = json.loads(output.getvalue()) + self.assertEqual({row['uid'] for row in data}, {'machine_uid', 'user_uid'}) + + def test_service_mode_dump_rejects_save_as(self): + params = _params() + params.service_mode = True + with self.assertRaisesRegex(CommandError, 'API response'): + PAMDebugDumpCommand().execute( + params, + folder_uid='nsf_folder', + format='json', + save_as='/tmp/blocked-dump.json', + ) + + def test_dump_obeys_enterprise_export_restriction(self): + params = _params() + params.enforcements = { + 'booleans': [{'key': 'restrict_export', 'value': True}], + } + + with self.assertRaisesRegex(CommandError, 'export restrictions'): + PAMDebugDumpCommand().execute( + params, + folder_uid='nsf_folder', + format='json', + ) + + def test_service_mode_rejects_local_dag_connection(self): + params = _params() + params.service_mode = True + + with patch.dict('os.environ', {'USE_LOCAL_DAG': 'true'}): + with self.assertRaisesRegex(CommandError, 'local DAG engine'): + get_connection(params) + + def test_dump_includes_graph_load_failures_in_record_errors(self): + params = _params() + params.nested_share_folder_records = {'nsf_folder': {'machine_uid'}} + params.record_rotation_cache = { + 'machine_uid': {'configuration_uid': 'config_uid'}, + } + with tempfile.TemporaryDirectory() as tmp: + out = f'{tmp}/dump.json' + with patch('keepercommander.commands.pam_debug.dump.get_connection'), \ + patch( + 'keepercommander.commands.pam_debug.dump.DAG', + side_effect=RuntimeError('backend details should not be returned'), + ): + PAMDebugDumpCommand().execute( + params, + folder_uid='nsf_folder', + format='json', + save_as=out, + ) + with open(out, encoding='utf-8') as fh: + data = json.load(fh) + + row = next(item for item in data if item['uid'] == 'machine_uid') + self.assertTrue(row['errors']) + self.assertEqual(row['errors'][0]['stage'], 'graph_sync') + self.assertNotIn('backend details', json.dumps(row['errors'])) + + def test_cli_dump_skips_unavailable_record_and_preserves_other_results(self): + params = _params() + params.nested_share_folder_records = {'nsf_folder': {'unavailable_uid'}} + output = io.StringIO() + with patch('sys.stdout', output): + PAMDebugDumpCommand().execute( + params, + folder_uid='nsf_folder', + format='json', + ) + self.assertEqual(json.loads(output.getvalue()), []) + + def test_service_mode_reports_unavailable_record_without_echoing_uid(self): + params = _params() + params.service_mode = True + params.nested_share_folder_records = {'nsf_folder': {'unavailable_uid'}} + output = io.StringIO() + with patch('sys.stdout', output): + PAMDebugDumpCommand().execute( + params, + folder_uid='nsf_folder', + format='json', + ) + data = json.loads(output.getvalue()) + self.assertEqual(len(data), 1) + self.assertIsNone(data[0]['uid']) + self.assertEqual(data[0]['errors'][0]['stage'], 'record_data') + self.assertNotIn('unavailable_uid', json.dumps(data)) + + def test_service_mode_dump_limits_folder_record_count(self): + params = _params() + params.service_mode = True + + with patch( + 'keepercommander.commands.pam_debug.dump.MAX_SERVICE_MODE_DUMP_RECORDS', + 1, + ), self.assertRaisesRegex(CommandError, 'limited to 1 folder records'): + PAMDebugDumpCommand().execute( + params, + folder_uid='nsf_folder', + format='json', + ) + + def test_service_mode_dump_limits_configuration_count(self): + params = _params() + params.service_mode = True + params.record_rotation_cache = { + 'machine_uid': {'configuration_uid': 'config_uid'}, + 'user_uid': {'configuration_uid': 'config_two_uid'}, + } + params.nested_share_records['config_two_uid'] = { + 'version': 6, + 'revision': 1, + 'shared': False, + } + params.nested_share_record_data['config_two_uid'] = { + 'data_json': { + 'type': 'pamNetworkConfiguration', + 'title': 'Second NSF Config', + 'fields': [], + }, + } + + with patch( + 'keepercommander.commands.pam_debug.dump.MAX_SERVICE_MODE_DUMP_CONFIGS', + 1, + ), self.assertRaisesRegex(CommandError, 'limited to 1 PAM configurations'): + PAMDebugDumpCommand().execute( + params, + folder_uid='nsf_folder', + format='json', + ) + def test_gateway_context_includes_nsf_shared_folders(self): folder_uid = 'zytjEAw5RTUJsF-PPx9wqA' params = _params() diff --git a/unit-tests/service/test_command.py b/unit-tests/service/test_command.py index c1ffdbf1b..45796aa13 100644 --- a/unit-tests/service/test_command.py +++ b/unit-tests/service/test_command.py @@ -4,8 +4,12 @@ from unittest import TestCase, mock from flask import Flask from keepercommander import params as params_module, vault +from keepercommander.error import CommandError from keepercommander.subfolder import RootFolderNode, SharedFolderNode -from keepercommander.service.util.command_util import CommandExecutor +from keepercommander.service.util.command_util import ( + CommandExecutor, + ServiceModeOutputLimitExceeded, +) from keepercommander.service.util.exceptions import CommandExecutionError from keepercommander.service.util.parse_keeper_response import parse_keeper_response from keepercommander.service.util.protected_records import get_protected_record_uids @@ -122,6 +126,21 @@ def test_response_parsing(self): self.assertIsInstance(parsed["data"], dict) self.assertIn("tree", parsed["data"]) + export_response = json.dumps({"records": [{"uid": "one"}]}) + parsed = parse_keeper_response( + "export --owned-only --format=json", export_response + ) + self.assertEqual(parsed["command"], "export") + self.assertEqual(parsed["data"], {"records": [{"uid": "one"}]}) + + dump_response = json.dumps([{"uid": "record-one", "graph_sync": {}}]) + parsed = parse_keeper_response( + "pam action debug dump FOLDER_UID --recursive --format=json", + dump_response, + ) + self.assertEqual(parsed["command"], "pam action debug dump") + self.assertEqual(parsed["data"], [{"uid": "record-one", "graph_sync": {}}]) + def test_capture_output(self): """Test command output capture""" test_command = "ls" @@ -132,6 +151,96 @@ def test_capture_output(self): return_value, output, logs = CommandExecutor.capture_output_and_logs(mock_params, test_command) self.assertEqual(return_value, expected_output) + def test_bulk_json_output_is_capped_before_capture_grows_unbounded(self): + with mock.patch( + 'keepercommander.service.util.command_util.MAX_SERVICE_MODE_JSON_RESPONSE_BYTES', + 8, + ), mock.patch( + 'keepercommander.cli.do_command', + side_effect=lambda _params, _command: print('123456789'), + ): + with self.assertRaises(ServiceModeOutputLimitExceeded): + CommandExecutor.capture_output_and_logs( + {}, 'export --format=json' + ) + + def test_bulk_json_limit_remains_exceeded_if_command_swallows_write_error(self): + def swallow_output_errors(_params, _command): + import sys + + for value in ('123456789', 'x'): + try: + sys.stdout.write(value) + except Exception: + pass + + with mock.patch( + 'keepercommander.service.util.command_util.MAX_SERVICE_MODE_JSON_RESPONSE_BYTES', + 8, + ), mock.patch( + 'keepercommander.cli.do_command', + side_effect=swallow_output_errors, + ): + with self.assertRaises(ServiceModeOutputLimitExceeded): + CommandExecutor.capture_output_and_logs( + {}, 'export --format=json' + ) + + def test_bulk_json_output_limit_returns_payload_too_large(self): + params = _params_with_protected_and_normal_record() + with mock.patch( + 'keepercommander.service.core.globals.ensure_params_loaded', + return_value=params, + ), mock.patch.object( + CommandExecutor, + 'capture_output_and_logs', + side_effect=ServiceModeOutputLimitExceeded, + ): + response, status_code = CommandExecutor.execute('export --format=json') + + self.assertEqual(status_code, 413) + self.assertEqual(response['status'], 'error') + self.assertIn('10 MiB', response['error']) + + def test_final_encrypted_bulk_response_is_checked_against_wire_limit(self): + params = _params_with_protected_and_normal_record() + with mock.patch( + 'keepercommander.service.util.command_util.MAX_SERVICE_MODE_JSON_RESPONSE_BYTES', + 8, + ), mock.patch( + 'keepercommander.service.core.globals.ensure_params_loaded', + return_value=params, + ), mock.patch.object( + CommandExecutor, + 'capture_output_and_logs', + return_value=(None, '{}', ''), + ), mock.patch.object( + CommandExecutor, + 'encrypt_response', + return_value=b'x' * 9, + ): + response, status_code = CommandExecutor.execute('export --format=json') + + self.assertEqual(status_code, 413) + self.assertIn('discarded', response['error']) + + def test_command_error_status_change_is_limited_to_bulk_json_commands(self): + params = _params_with_protected_and_normal_record() + with mock.patch( + 'keepercommander.service.core.globals.ensure_params_loaded', + return_value=params, + ), mock.patch.object( + CommandExecutor, + 'capture_output_and_logs', + side_effect=CommandError('export', 'restricted'), + ): + bulk_response, bulk_status = CommandExecutor.execute('export --format=json') + legacy_response, legacy_status = CommandExecutor.execute(f'get {NORMAL_UID}') + + self.assertEqual(bulk_status, 400) + self.assertEqual(legacy_status, 500) + self.assertIn('Unexpected error', legacy_response['error']) + @unittest.skip def test_integration_command_flow(self): """Test the complete command execution flow""" diff --git a/unit-tests/service/test_protected_records.py b/unit-tests/service/test_protected_records.py index 57b8b9fae..9d81643c3 100644 --- a/unit-tests/service/test_protected_records.py +++ b/unit-tests/service/test_protected_records.py @@ -203,6 +203,21 @@ def test_removed_record_is_no_longer_found(self): second = get_protected_record_uids(p) self.assertEqual(set(second.keys()), {'UID_CONFIG'}) + def test_protected_title_is_found_in_nsf_record_cache(self): + p = params_module.KeeperParams() + p.nested_share_records = {'NSF_CONFIG': {'version': 3}} + p.nested_share_record_data = { + 'NSF_CONFIG': { + 'data_json': { + 'type': 'login', + 'title': PROTECTED_TITLE, + 'fields': [], + }, + }, + } + + self.assertIn('NSF_CONFIG', get_protected_record_uids(p)) + class TestHideFromRecordCache(TestCase): def test_hides_protected_uid_inside_the_block(self): @@ -248,6 +263,20 @@ def test_no_protected_uids_is_a_noop(self): with hide_from_record_cache(p, {}): self.assertIn('NORMAL', p.record_cache) + def test_hides_and_restores_protected_uid_in_nsf_folder_membership(self): + p = params_module.KeeperParams() + p.nested_share_folder_records = { + 'NSF_FOLDER': {'PROTECTED', 'NORMAL'}, + } + + with hide_from_record_cache(p, {'PROTECTED': PROTECTED_TITLE}): + self.assertEqual(p.nested_share_folder_records['NSF_FOLDER'], {'NORMAL'}) + + self.assertEqual( + p.nested_share_folder_records['NSF_FOLDER'], + {'PROTECTED', 'NORMAL'}, + ) + def test_params_none_is_a_noop(self): with hide_from_record_cache(None, {'PROTECTED': PROTECTED_TITLE}): pass @@ -423,6 +452,16 @@ def test_missing_subfolder_record_cache_is_ignored(self): p = params_module.KeeperParams() self.assertEqual(get_protected_folder_uids(p, {'PROTECTED': 'x'}), set()) + def test_returns_nsf_folder_containing_a_protected_record(self): + p = params_module.KeeperParams() + p.nested_share_folder_records = { + 'NSF_FOLDER': {'PROTECTED', 'NORMAL'}, + } + self.assertEqual( + get_protected_folder_uids(p, {'PROTECTED': PROTECTED_TITLE}), + {'NSF_FOLDER'}, + ) + def test_renamed_folder_is_still_found(self): """Derived from record containment, not a title list -- a rename doesn't lose protection.""" p = _params_with_folder(folder_uid='FOLDER1', record_uid='PROTECTED') @@ -440,6 +479,15 @@ def test_hides_protected_folder_from_all_three_caches_inside_the_block(self): self.assertNotIn('FOLDER1', p.shared_folder_cache) self.assertNotIn('FOLDER1', p.subfolder_cache) + def test_hides_and_restores_protected_nsf_folder(self): + p = params_module.KeeperParams() + p.nested_share_folders = { + 'NSF_FOLDER': {'name': 'Service Config Folder', 'parent_uid': None}, + } + with hide_from_folder_cache(p, {'NSF_FOLDER'}): + self.assertNotIn('NSF_FOLDER', p.nested_share_folders) + self.assertIn('NSF_FOLDER', p.nested_share_folders) + def test_strips_uid_from_root_folder_subfolders_during_the_block(self): p = _params_with_folder(folder_uid='FOLDER1') with hide_from_folder_cache(p, {'FOLDER1'}): diff --git a/unit-tests/service/test_verified_command.py b/unit-tests/service/test_verified_command.py index 2615b7adb..2796a3cb5 100644 --- a/unit-tests/service/test_verified_command.py +++ b/unit-tests/service/test_verified_command.py @@ -114,7 +114,6 @@ def test_host_filesystem_commands_blocked(self): for cmd in ( 'run-batch --dry-run /etc/passwd', 'run --dry-run ~/.keeper/config.json', - 'export --format=json /tmp/out.json', 'download-membership --source=keeper /tmp/m.json', 'download-record-types --source=keeper /tmp/rt.json', 'apply-membership /tmp/m.json', @@ -125,6 +124,67 @@ def test_host_filesystem_commands_blocked(self): self.assertIsNotNone(err) self.assertIn(ban, err) + def test_service_mode_export_only_allows_json_to_response(self): + check = Verifycommand.validate_service_mode_restrictions + for cmd in ( + 'export --format=json', + 'export --format json', + 'export --format=json --folder "Team Vault"', + 'export --format=json --folder=FOLDER_UID --owned-only', + 'export --owned-only --format=json --folder=FOLDER_UID', + ): + with self.subTest(cmd=cmd): + self.assertIsNone(check(_tokens(cmd))) + + def test_service_mode_export_rejects_file_output_and_unapproved_options(self): + check = Verifycommand.validate_service_mode_restrictions + ban = 'Local filesystem access' + for cmd in ( + 'export', + 'export --format=csv', + 'export --format=keepass', + 'export --format=json /tmp/out.json', + 'export --format=json --zip', + 'export --format=json --max-size=1M', + 'export --format=json --save-in-vault', + 'export --format=json --keepass-key-file=/tmp/key', + 'export --format=json --folder FOLDER_UID --force', + 'export --format=json --unexpected', + 'export --format=json --form=json', + 'export --format=json --format=json', + 'export --format=json --folder=FOLDER_UID --folder=OTHER_UID', + 'export --format=json --folder=', + 'export --format=json --include-dag', + ): + with self.subTest(cmd=cmd): + err = check(_tokens(cmd)) + self.assertIsNotNone(err) + self.assertIn(ban, err) + self.assertIn('export --format=json', err) + + def test_service_mode_pam_debug_dump_allows_only_json_response(self): + check = Verifycommand.validate_service_mode_restrictions + for cmd in ( + 'pam action debug dump FOLDER_UID --format=json', + 'pam action debug dump FOLDER_UID --recursive --format json', + 'pam a debug d FOLDER_UID -r --format=json', + ): + with self.subTest(cmd=cmd): + self.assertIsNone(check(_tokens(cmd))) + + for cmd in ( + 'pam action debug dump FOLDER_UID --recursive -s textdump.json', + 'pam action debug dump FOLDER_UID --save-as=/tmp/textdump.json --format=json', + 'pam action debug dump FOLDER_UID --recursive', + 'pam action debug dump FOLDER_UID --format=csv', + 'pam action debug dump FOLDER_UID --format=json --unexpected', + 'pam a debug d FOLDER_UID -s /tmp/out.json --format=json', + ): + with self.subTest(cmd=cmd): + err = check(_tokens(cmd)) + self.assertIsNotNone(err) + self.assertIn('API response', err) + def test_host_path_output_args_blocked(self): check = Verifycommand.validate_service_mode_restrictions ban = 'Local filesystem access' diff --git a/unit-tests/test_importer.py b/unit-tests/test_importer.py index aaa203611..de59c5bff 100644 --- a/unit-tests/test_importer.py +++ b/unit-tests/test_importer.py @@ -1,11 +1,14 @@ +import io +import json import os import tempfile from unittest import TestCase, mock, skipUnless from data_vault import get_synced_params, get_connected_params from helper import KeeperApiHelper -from keepercommander import vault -from keepercommander.importer import importer, commands +from keepercommander import params as params_module, vault +from keepercommander.error import CommandError +from keepercommander.importer import importer, commands, imp_exp try: from keepercommander.importer.keepass.keepass import KeepassExporter, PyKeePass @@ -80,6 +83,136 @@ def mock_read(): with mock.patch('os.path.isfile', return_value=True): cmd_import.execute(param_import, format='json', name='json') + def test_json_export_stdout_does_not_open_output_file(self): + record = importer.Record() + record.uid = 'PAM_RECORD_UID' + record.title = 'PAM record' + record.type = 'pamUser' + output = io.StringIO() + + with mock.patch('sys.stdout', output), \ + mock.patch('builtins.open', side_effect=AssertionError('stdout export must not open a file')): + importer.exporter_for_format('json')().execute(None, [record]) + + exported = json.loads(output.getvalue()) + self.assertEqual(exported['records'][0]['uid'], 'PAM_RECORD_UID') + + def test_service_mode_export_defense_in_depth(self): + params = params_module.KeeperParams() + params.service_mode = True + command = commands.RecordExportCommand() + + for options in ( + {'format': 'json', 'name': '/tmp/export.json'}, + {'format': 'json', 'name': None, 'zip_archive': True}, + {'format': 'json', 'name': None, 'max_size': '1M'}, + {'format': 'csv', 'name': None}, + ): + with self.subTest(options=options), self.assertRaises(CommandError): + command.execute(params, **options) + + with mock.patch('keepercommander.importer.commands.imp_exp.export') as export: + command.execute( + params, format='json', name=None, folder='FOLDER_UID', + owned_only=True, + ) + + export.assert_called_once_with( + params, 'json', None, folder='FOLDER_UID', owned_only=True, + _service_mode_record_limit=imp_exp.SERVICE_MODE_EXPORT_MAX_RECORDS, + _service_mode_input_limit=imp_exp.SERVICE_MODE_EXPORT_MAX_INPUT_BYTES, + ) + + def test_service_mode_export_restriction_is_an_explicit_command_error(self): + params = params_module.KeeperParams() + params.service_mode = True + params.enforcements = { + 'booleans': [{'key': 'restrict_export', 'value': True}], + } + + with self.assertRaisesRegex(CommandError, 'enterprise export restrictions'): + commands.RecordExportCommand().execute( + params, + format='json', + name=None, + ) + + def test_json_export_service_mode_record_limit(self): + params = params_module.KeeperParams() + params.record_cache = { + uid: { + 'record_uid': uid, + 'version': 3, + 'client_modified_time': 0, + 'data_unencrypted': json.dumps({ + 'title': f'Record {uid}', + 'type': 'login', + 'fields': [], + }).encode('utf-8'), + } + for uid in ('ONE', 'TWO') + } + + with mock.patch('keepercommander.importer.imp_exp.sync_down.sync_down'): + with self.assertRaisesRegex(CommandError, 'record processing limit'): + imp_exp.export( + params, + 'json', + None, + _service_mode_record_limit=1, + ) + + def test_json_export_service_mode_input_data_limit(self): + params = params_module.KeeperParams() + params.record_cache = { + 'ONE': { + 'record_uid': 'ONE', + 'version': 3, + 'client_modified_time': 0, + 'data_unencrypted': b'x' * 32, + }, + } + + with mock.patch('keepercommander.importer.imp_exp.sync_down.sync_down'): + with self.assertRaisesRegex(CommandError, 'input data processing limit'): + imp_exp.export( + params, + 'json', + None, + _service_mode_input_limit=8, + ) + + def test_owned_only_export_skips_protected_record_missing_from_record_cache(self): + params = params_module.KeeperParams() + visible_uid = 'VISIBLE_RECORD_UID' + protected_uid = 'PROTECTED_CONFIG_UID' + params.record_cache = { + visible_uid: { + 'record_uid': visible_uid, + 'version': 3, + 'client_modified_time': 0, + 'data_unencrypted': json.dumps({ + 'title': 'Visible record', + 'type': 'login', + 'fields': [], + }).encode('utf-8'), + } + } + # Service Mode hides the protected record payload but its ownership + # metadata remains available for unrelated commands. + params.record_owner_cache = { + visible_uid: params_module.RecordOwner(True, None), + protected_uid: params_module.RecordOwner(True, None), + } + output = io.StringIO() + + with mock.patch('keepercommander.importer.imp_exp.sync_down.sync_down'), \ + mock.patch('sys.stdout', output): + imp_exp.export(params, 'json', None, owned_only=True) + + exported = json.loads(output.getvalue()) + self.assertEqual([record['uid'] for record in exported['records']], [visible_uid]) + @skipUnless(KeepassExporter and PyKeePass, 'pykeepass is not installed') def test_keepass_export_sanitizes_xml_invalid_characters(self): record = importer.Record() From 171ef0b6544481d9f7b61a253d73ce94ad2c6040 Mon Sep 17 00:00:00 2001 From: Sergey Kolupaev Date: Thu, 8 Oct 2026 12:27:23 -0700 Subject: [PATCH 4/4] Release 18.1.8 --- keepercommander/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/keepercommander/__init__.py b/keepercommander/__init__.py index 7743d31aa..3f644bb26 100644 --- a/keepercommander/__init__.py +++ b/keepercommander/__init__.py @@ -10,4 +10,4 @@ # Contact: commander@keepersecurity.com # -__version__ = '18.1.7' +__version__ = '18.1.8'