PAM Workflow changes and improvements

keepercommander/commands/workflow/approver_commands.py

@@ -22,7 +22,7 @@
 from ...proto import workflow_pb2
 from ... import api, crypto, utils
 
-from .helpers import RecordResolver, WorkflowFormatter, sanitize_router_error
+from .helpers import RecordResolver, WorkflowFormatter, sanitize_router_error, fix_dash_uid_args
 
 
 class WorkflowGetApprovalRequestsCommand(Command):
@@ -183,6 +183,10 @@ class WorkflowApproveCommand(Command):
     def get_parser(self):
         return WorkflowApproveCommand.parser
 
+    def execute_args(self, params, args, **kwargs):
+        args = fix_dash_uid_args(self.get_parser(), args)
+        return super().execute_args(params, args, **kwargs)
+
     def execute(self, params: KeeperParams, **kwargs):
         flow_uid = kwargs.get('flow_uid')
         try:
@@ -222,6 +226,10 @@ class WorkflowDenyCommand(Command):
     def get_parser(self):
         return WorkflowDenyCommand.parser
 
+    def execute_args(self, params, args, **kwargs):
+        args = fix_dash_uid_args(self.get_parser(), args)
+        return super().execute_args(params, args, **kwargs)
+
     def execute(self, params: KeeperParams, **kwargs):
         flow_uid = kwargs.get('flow_uid')
         reason = kwargs.get('reason') or ''
@@ -236,7 +244,14 @@ def execute(self, params: KeeperParams, **kwargs):
         if reason:
             reason_bytes = reason.encode('utf-8')
             encrypted = self._encrypt_denial_reason(params, flow_uid_bytes, reason_bytes)
-            denial.denialReason = encrypted if encrypted else reason_bytes
+            if encrypted:
+                denial.denialReason = encrypted
+            else:
+                logging.warning(
+                    'Could not encrypt denial reason for the requester — reason will not be attached. '
+                    'The denial itself will still be sent.'
+                )
+                reason = ''
 
         try:
             _post_request_to_router(params, 'approve_or_deny_workflow_access', rq_proto=denial)

keepercommander/commands/workflow/config_commands.py

@@ -83,8 +83,6 @@ class WorkflowCreateCommand(Command):
                         help='Comma-separated allowed days (e.g., "mon,tue,wed,thu,fri")')
     parser.add_argument('--time-range', type=str,
                         help='Allowed time range in HH:MM-HH:MM format (e.g., "09:00-17:00")')
-    parser.add_argument('--timezone', type=str,
-                        help='Timezone for allowed times (e.g., "America/New_York")')
     parser.add_argument('-u', '--approver', action='append',
                         help='User email to add as an approver. Pass multiple times to '
                              'add several. Required when --approvals-needed > 0. '
@@ -97,6 +95,7 @@ def get_parser(self):
 
     def execute(self, params: KeeperParams, **kwargs):
         record_uid, record = RecordResolver.resolve(params, kwargs.get('record'))
+        RecordResolver.validate_workflow_record_type(record)
         record_uid_bytes = utils.base64_url_decode(record_uid)
 
         # Pre-check: surface the "already exists" condition with a clear,
@@ -155,7 +154,7 @@ def execute(self, params: KeeperParams, **kwargs):
         parameters.accessLength = WorkflowFormatter.parse_duration(kwargs.get('duration', '1d'))
 
         temporal_filter = WorkflowFormatter.build_temporal_filter(
-            kwargs.get('allowed_days'), kwargs.get('time_range'), kwargs.get('timezone'),
+            kwargs.get('allowed_days'), kwargs.get('time_range'),
         )
         if temporal_filter:
             parameters.allowedTimes.CopyFrom(temporal_filter)
@@ -386,8 +385,6 @@ class WorkflowUpdateCommand(Command):
                         help='Comma-separated allowed days (e.g., "mon,tue,wed,thu,fri")')
     parser.add_argument('--time-range', type=str,
                         help='Allowed time range in HH:MM-HH:MM format (e.g., "09:00-17:00")')
-    parser.add_argument('--timezone', type=str,
-                        help='Timezone for allowed times (e.g., "America/New_York")')
     parser.add_argument('--format', dest='format', action='store',
                         choices=['table', 'json'], default='table', help='Output format')
 
@@ -434,7 +431,7 @@ def execute(self, params: KeeperParams, **kwargs):
                 updates_provided = True
 
             temporal_filter = WorkflowFormatter.build_temporal_filter(
-                kwargs.get('allowed_days'), kwargs.get('time_range'), kwargs.get('timezone'),
+                kwargs.get('allowed_days'), kwargs.get('time_range'),
             )
             if temporal_filter:
                 parameters.allowedTimes.CopyFrom(temporal_filter)
@@ -456,6 +453,8 @@ def execute(self, params: KeeperParams, **kwargs):
                 print(f"Record: {record.title} ({record_uid})")
                 print()
 
+        except CommandError:
+            raise
         except Exception as e:
             raise CommandError('', f'Failed to update workflow: {sanitize_router_error(e)}')
 
@@ -582,6 +581,8 @@ def execute(self, params: KeeperParams, **kwargs):
                     print(f"Type: Escalation approver{esc_info}")
                 print()
 
+        except CommandError:
+            raise
         except Exception as e:
             raise CommandError('', f'Failed to add approvers: {sanitize_router_error(e)}')
 
@@ -642,5 +643,7 @@ def execute(self, params: KeeperParams, **kwargs):
                 print(f"Removed {total} approver(s)")
                 print()
 
+        except CommandError:
+            raise
         except Exception as e:
             raise CommandError('', f'Failed to remove approvers: {sanitize_router_error(e)}')

keepercommander/commands/workflow/helpers.py

@@ -10,6 +10,7 @@
 #
 
 import re
+import shlex
 from typing import List, Optional, Tuple
 
 from ...error import CommandError
@@ -24,6 +25,44 @@
 _RESPONSE_CODE_RE = re.compile(r'\s*[Rr]esponse\s+code:\s*\S+\s*$')
 
 
+def fix_dash_uid_args(parser, args):
+    """Insert '--' before a base64url UID that starts with '-' so argparse
+    treats it as a positional value instead of an unknown flag."""
+    if not args or '--' in args:
+        return args
+    try:
+        tokens = shlex.split(args)
+    except ValueError:
+        return args
+    known_opts = set()
+    consumes_value = set()
+    for action in parser._actions:
+        for opt in action.option_strings:
+            known_opts.add(opt)
+            if action.nargs != 0:
+                consumes_value.add(opt)
+
+    result = []
+    skip_next = False
+    for token in tokens:
+        if skip_next:
+            result.append(token)
+            skip_next = False
+            continue
+        if token in known_opts:
+            result.append(token)
+            if token in consumes_value:
+                skip_next = True
+            continue
+        if token.startswith('-'):
+            result.append('--')
+        result.append(token)
+
+    if len(result) != len(tokens):
+        return ' '.join(shlex.quote(t) for t in result)
+    return args
+
+
 def sanitize_router_error(error: Exception) -> str:
     msg = str(error)
     msg = _RESPONSE_CODE_RE.sub('', msg)
@@ -32,51 +71,73 @@ def sanitize_router_error(error: Exception) -> str:
     return msg or 'Unknown error'
 
 
-_ENFORCEMENT_KEY = 'allow_configure_workflow_settings'
-
-
 def print_exempt_message(fmt='table'):
     """Print the standard exemption message in the appropriate format."""
     import json as _json
     from ...display import bcolors as _bc
     if fmt == 'json':
         print(_json.dumps({'status': 'exempt', 'message': 'Workflow not required'}, indent=2))
     else:
-        print(f"\n{_bc.WARNING}You have edit access and workflow management permissions for this record.{_bc.ENDC}\n")
-        print("Workflow is not required — you can access this resource directly.\n")
+        print(f"\n{_bc.WARNING}You are exempt from workflow restrictions on this record.{_bc.ENDC}")
+        print("As a record owner or approver, you can access this resource directly.\n")
 
 
-def is_workflow_exempt(params, record_uid):
-    """Users with edit access AND 'Can manage workflow settings' are exempt from workflow."""
-    enforcements = getattr(params, 'enforcements', None)
-    if not enforcements or 'booleans' not in enforcements:
-        return False
-    can_manage = any(
-        b.get('value') for b in enforcements['booleans']
-        if b.get('key') == _ENFORCEMENT_KEY
-    )
-    if not can_manage:
-        return False
-
+def is_record_owner(params, record_uid):
+    """Check if the current user is the owner of the given record."""
     if record_uid in getattr(params, 'record_owner_cache', {}):
         owner_info = params.record_owner_cache[record_uid]
         if getattr(owner_info, 'owner', False):
             return True
+    return False
 
-    meta = getattr(params, 'meta_data_cache', {}).get(record_uid)
-    if meta and meta.get('can_edit'):
-        return True
 
-    for sf_uid in getattr(params, 'shared_folder_cache', {}):
-        sf = params.shared_folder_cache[sf_uid]
-        for sfr in sf.get('records', []):
-            if sfr.get('record_uid') == record_uid:
-                if sfr.get('owner') or sfr.get('can_edit'):
-                    return True
+def is_on_approver_list(params, config):
+    """Check if the current user appears on the workflow config's approver list,
+    either directly by email or via team membership."""
+    if not config or not config.approvers:
+        return False
 
+    current_user = getattr(params, 'user', '')
+    team_cache = getattr(params, 'team_cache', {})
+
+    for approver in config.approvers:
+        if approver.user and approver.user.lower() == current_user.lower():
+            return True
+        if approver.teamUid:
+            team_uid_b64 = utils.base64_url_encode(approver.teamUid)
+            if team_uid_b64 in team_cache:
+                return True
     return False
 
 
+def is_workflow_exempt(params, record_uid, config=None):
+    """Exempt users: record owner OR on the approver list.
+
+    If *config* is already available (e.g. from a prior read_workflow_config
+    call) pass it to avoid an extra API round-trip.  When *config* is None the
+    function reads it from the router; a transport failure is treated as
+    non-exempt (fail closed).
+    """
+    if is_record_owner(params, record_uid):
+        return True
+
+    if config is None:
+        from ..pam.router_helper import _post_request_to_router
+        try:
+            ref = ProtobufRefBuilder.record_ref(
+                utils.base64_url_decode(record_uid),
+                '',
+            )
+            config = _post_request_to_router(
+                params, 'read_workflow_config',
+                rq_proto=ref, rs_type=workflow_pb2.WorkflowConfig,
+            )
+        except Exception:
+            return False
+
+    return is_on_approver_list(params, config)
+
+
 def is_pam_action_allowed_by_enforcement(params: KeeperParams, enforcement_key: str) -> bool:
     """Per-user enterprise enforcement gate: is this user permitted to perform
     the action by their enterprise enforcement profile?
@@ -305,6 +366,8 @@ def prompt_for_reason_ticket(needs_reason: bool, needs_ticket: bool) -> Tuple[Op
 
 class RecordResolver:
 
+    WORKFLOW_RECORD_TYPES = {'pamMachine', 'pamDirectory', 'pamDatabase', 'pamRemoteBrowser'}
+
     @staticmethod
     def resolve(params, record_input, allow_missing=False):
         if record_input in params.record_cache:
@@ -317,6 +380,19 @@ def resolve(params, record_input, allow_missing=False):
             return None, None
         raise CommandError('', f'Record "{record_input}" not found')
 
+    @staticmethod
+    def validate_workflow_record_type(record):
+        """Raise CommandError if the record type doesn't support workflows."""
+        if not isinstance(record, vault.TypedRecord):
+            raise CommandError('', 'Workflows are only supported on PAM records')
+        if record.record_type not in RecordResolver.WORKFLOW_RECORD_TYPES:
+            supported = ', '.join(sorted(RecordResolver.WORKFLOW_RECORD_TYPES))
+            raise CommandError(
+                '',
+                f'Record "{record.title}" is of type "{record.record_type}" which does not support workflows.\n'
+                f'Supported record types: {supported}'
+            )
+
     @staticmethod
     def get_uid_bytes(params: KeeperParams, record_uid: str) -> bytes:
         uid_bytes = utils.base64_url_decode(record_uid)
@@ -440,10 +516,19 @@ class WorkflowFormatter:
         workflow_pb2.SUNDAY: 'Sunday',
     }
 
+    BLOCKING_CONDITIONS = {workflow_pb2.AC_TIME, workflow_pb2.AC_APPROVAL}
+
     @staticmethod
     def format_stage(stage: int, status=None) -> str:
         if stage == workflow_pb2.WS_READY_TO_START and status is not None:
-            if not status.startedOn and not status.conditions:
+            if status.conditions:
+                has_blocking = any(c in WorkflowFormatter.BLOCKING_CONDITIONS for c in status.conditions)
+                if has_blocking:
+                    return 'Waiting'
+                return 'Ready to Start'
+            if status.approvedBy and not status.startedOn:
+                return 'Ready to Start'
+            if not status.startedOn and not status.approvedBy:
                 return 'Needs Action'
         return WorkflowFormatter.STAGE_MAP.get(stage, f'Unknown ({stage})')
 
@@ -490,8 +575,8 @@ def format_duration(milliseconds: int) -> str:
         return f"{seconds} second{'s' if seconds != 1 else ''}"
 
     @staticmethod
-    def build_temporal_filter(allowed_days_str, time_range_str, timezone_str):
-        if not allowed_days_str and not time_range_str and not timezone_str:
+    def build_temporal_filter(allowed_days_str, time_range_str):
+        if not allowed_days_str and not time_range_str:
             return None
 
         temporal = workflow_pb2.TemporalAccessFilter()
@@ -516,11 +601,38 @@ def build_temporal_filter(allowed_days_str, time_range_str, timezone_str):
             time_range.endTime = end_hhmm
             temporal.timeRanges.append(time_range)
 
-        if timezone_str:
-            temporal.timeZone = timezone_str
+        temporal.timeZone = WorkflowFormatter._get_local_iana_timezone()
 
         return temporal
 
+    @staticmethod
+    def _get_local_iana_timezone():
+        import os
+
+        tz = os.environ.get('TZ')
+        if tz and '/' in tz:
+            return tz
+
+        try:
+            link = os.readlink('/etc/localtime')
+            marker = '/zoneinfo/'
+            idx = link.find(marker)
+            if idx != -1:
+                return link[idx + len(marker):]
+        except (OSError, ValueError):
+            pass
+
+        try:
+            with open('/etc/timezone', 'r') as f:
+                tz = f.read().strip()
+                if tz and '/' in tz:
+                    return tz
+        except (OSError, IOError):
+            pass
+
+        raise CommandError('', 'Could not detect local IANA timezone. '
+                           'Set the TZ environment variable (e.g., TZ=Asia/Kolkata)')
+
     @staticmethod
     def _parse_time_to_hhmm(time_str):
         """Parse 'HH:MM' to the HHMM integer the server stores on