Merge pull request #27 from PostHog/tom/approver-groups

Allow specifying groups of approvers

Author: Piccirello

docs/CONFIGURATION.md

@@ -14,7 +14,8 @@ The fields in the configuration dictionary are:
 - **Resource**: This field defines the specific resource(s) being requested. It accepts either a single string or a list of strings. Setting this field to "*" allows the rule to match all resources associated with the specified `ResourceType`.
 - **PermissionSet**: Here, you indicate the permission set(s) being requested. This can be either a single string or a list of strings. You can specify permission sets by **name** (e.g., `"AdministratorAccess"`) or by **ARN** (e.g., `"arn:aws:sso:::permissionSet/ssoins-1234567890abcdef/ps-1234567890abcdef"`). Using ARNs is recommended for Terraform users as it reduces API calls and allows direct reference to `aws_ssoadmin_permission_set.*.arn`. If set to "*", the rule matches all permission sets available for the defined `Resource` and `ResourceType`.
 - **Approvers**: This field lists the potential approvers for the request. It accepts either a single string or a list of strings representing different approvers.
-- **AllowSelfApproval**: This field can be a boolean, indicating whether the requester, if present in the `Approvers` list, is permitted to approve their own request. It defaults to `None`.
+- **ApproverGroups**: This field lists Slack usergroup IDs whose members can approve the request. It accepts either a single string or a list of strings. Members of the specified Slack usergroups are resolved at request time and added to the list of approvers.
+- **AllowSelfApproval**: This field can be a boolean, indicating whether the requester, if present in the `Approvers` list or a member of an `ApproverGroups` group, is permitted to approve their own request. It defaults to `None`.
 - **ApprovalIsNotRequired**: This field can also be a boolean, signifying whether the approval can be granted automatically, bypassing the approvers entirely. The default value is `None`.
 - **RequiredGroupMembership**: This field restricts the rule to only users who are members of at least one of the specified SSO groups. Accepts a single group ID or a list of group IDs. If empty or omitted, the rule applies to all users.
 
@@ -26,16 +27,16 @@ In the system, an explicit denial in any statement overrides any approvals. For
 
 Requests will be approved automatically if either of the following conditions are met:
 
-- AllowSelfApproval is set to true and the requester is in the Approvers list.
+- AllowSelfApproval is set to true and the requester is in the Approvers list or a member of an ApproverGroups group.
 - ApprovalIsNotRequired is set to true.
 
 ## Aggregation of Rules
 
-The approval decision and final list of reviewers will be calculated dynamically based on the aggregate of all rules. If you have a rule that specifies that someone is an approver for all accounts, then that person will be automatically added to all requests, even if there are more detailed rules for specific accounts or permission sets.
+The approval decision and final list of reviewers will be calculated dynamically based on the aggregate of all rules. Approvers from both `Approvers` and `ApproverGroups` are combined. If you have a rule that specifies that someone is an approver for all accounts, then that person will be automatically added to all requests, even if there are more detailed rules for specific accounts or permission sets.
 
 ## Single Approver
 
-If there is only one approver and AllowSelfApproval is not set to true, nobody will be able to approve the request.
+If there is only one approver (whether from `Approvers` or `ApproverGroups`) and AllowSelfApproval is not set to true, nobody will be able to approve the request.
 
 ## Request Processing Diagram
 
@@ -110,6 +111,7 @@ Require explicit approval for production admin access:
   "Resource" : ["prod_account_id", "prod_account_id2"],
   "PermissionSet" : "AdministratorAccess",
   "Approvers" : ["manager@corp.com", "ciso@corp.com"],
+  "ApproverGroups" : ["SAZ94GDB8"],  # Slack usergroup ID for the SRE team
   "ApprovalIsNotRequired" : false,
   "AllowSelfApproval" : false,
 }

docs/DEPLOYMENT.md

@@ -260,7 +260,7 @@ settings:
 | `chat:write` | Post messages to Slack |
 | `users:read.email` | Find user's email address for AWS account assignments |
 | `users:read` | Read user info for mentions in requests |
-| `channels:history` | Find old messages for "discard button" events |
+| `channels:history` | Find old messages for request expiration events |
 | `channels:read` | Determine if requester is in the channel |
 | `groups:read` | Same as above but for private channels |
 | `im:write` | Send direct messages to users not in the channel |

docs/FEATURES.md

@@ -214,7 +214,7 @@ All events include `application: "aws-sso-elevator"` as a property.
 |-------|---------|------------|
 | `aws_access_requested` | User submits access request | account_id, permission_set, requester_email, granted, duration_hours |
 | `aws_access_approved` | Access approved | account_id, permission_set, approver_email, duration_hours, self_approved |
-| `aws_access_denied` | Request denied/discarded | account_id, permission_set, approver_email, requester_email |
+| `aws_access_denied` | Request denied | account_id, permission_set, approver_email, requester_email |
 | `aws_access_revoked_early` | Early revocation | account_id, permission_set, revoker_email, reason |
 | `aws_group_access_requested` | Group access request | group_id, group_name, requester_email |
 | `aws_group_access_approved` | Group access approved | group_id, group_name, duration_hours, self_approved |

docs/GROUP_ACCESS.md

@@ -32,7 +32,8 @@ group_config = [
       "Resource" : ["44445555-3333-2222-1111-555557777777"], #ProdAdminAccess
       "Approvers" : [
         "email@gmail.com"
-      ]
+      ],
+      "ApproverGroups" : ["SAZ94GDB8"],  # Slack usergroup ID
     },
 ]
 ```

examples/complete/main.tf

@@ -71,11 +71,11 @@ module "aws_sso_elevator" {
     target_prefix = "some_prefix_for_access_logs"
   }
 
-  # "Resource", "PermissionSet", "Approvers" can be a string or a list of strings
+  # "Resource", "PermissionSet", "Approvers", "ApproverGroups" can be a string or a list of strings
   # "Resource" & "PermissionSet" can be set to "*" to match all
 
   # Request will be approved automatically if:
-  # - "AllowSelfApproval" is set to true, and requester is in "Approvers" list
+  # - "AllowSelfApproval" is set to true, and requester is in "Approvers" list or a member of an "ApproverGroups" group
   # - "ApprovalIsNotRequired" is set to true
 
   # If there is only one approver, and "AllowSelfApproval" isn't set to true, nobody will be able to approve the request
@@ -112,6 +112,7 @@ module "aws_sso_elevator" {
       "Resource" : "account_id",
       "PermissionSet" : ["ReadOnlyPlus", "AdministratorAccess"],
       "Approvers" : ["email@gmail.com"],
+      "ApproverGroups" : ["SAZ94GDB8"], # Slack usergroup ID
       "AllowSelfApproval" : true,
     },
     {

src/access_control.py

@@ -1,6 +1,6 @@
 import datetime
 from enum import Enum
-from typing import FrozenSet
+from typing import Callable, FrozenSet
 
 import boto3
 
@@ -36,6 +36,7 @@ class AccessRequestDecision(BaseModel):
     reason: DecisionReason
     based_on_statements: FrozenSet[Statement] | FrozenSet[GroupStatement]
     approvers: FrozenSet[str] = frozenset()
+    approver_groups: FrozenSet[str] = frozenset()
 
 
 def determine_affected_statements(
@@ -65,6 +66,8 @@ def make_decision_on_access_request(  # noqa: PLR0911, PLR0913
     group_id: str | None = None,
     user_group_ids: set[str] | None = None,
     permission_set_arn: str | None = None,
+    requester_slack_id: str | None = None,
+    approver_group_resolver: Callable[[frozenset[str]], set[str]] | None = None,
 ) -> AccessRequestDecision:
     """Make a decision on an access request.
 
@@ -78,6 +81,9 @@ def make_decision_on_access_request(  # noqa: PLR0911, PLR0913
             If None, group-based filtering is skipped (backwards compatible).
             If an empty set, only statements without required_group_membership will match.
         permission_set_arn: The ARN of the permission set (optional, for matching ARN-based config).
+        requester_slack_id: The Slack ID of the requester (for self-approval via group membership).
+        approver_group_resolver: Function that resolves approver group IDs to Slack user IDs.
+            Used for checking self-approval eligibility via group membership.
     """
     # Filter statements by user's group membership eligibility if user_group_ids provided
     # This is only applicable to Statement (not GroupStatement)
@@ -88,9 +94,21 @@ def make_decision_on_access_request(  # noqa: PLR0911, PLR0913
 
     decision_based_on_statements: set[Statement] | set[GroupStatement] = set()
     potential_approvers = set()
+    potential_approver_groups: set[str] = set()
+
+    # Helper to check if requester is an approver for a statement (individual or via group)
+    def is_requester_approver_for_statement(stmt: Statement | GroupStatement) -> bool:
+        if requester_email in stmt.approvers:
+            return True
+        # Check group membership if resolver is available
+        if requester_slack_id and approver_group_resolver and stmt.approver_groups:
+            group_user_ids = approver_group_resolver(stmt.approver_groups)
+            if requester_slack_id in group_user_ids:
+                return True
+        return False
 
     explicit_deny_self_approval = any(
-        statement.allow_self_approval is False and requester_email in statement.approvers for statement in affected_statements
+        statement.allow_self_approval is False and is_requester_approver_for_statement(statement) for statement in affected_statements
     )
     explicit_deny_approval_not_required = any(statement.approval_is_not_required is False for statement in affected_statements)
 
@@ -101,7 +119,8 @@ def make_decision_on_access_request(  # noqa: PLR0911, PLR0913
                 reason=DecisionReason.ApprovalNotRequired,
                 based_on_statements=frozenset([statement]),  # type: ignore # noqa: PGH003
             )
-        if requester_email in statement.approvers and statement.allow_self_approval and not explicit_deny_self_approval:
+        # Check self-approval: requester must be an approver (individual or via group)
+        if is_requester_approver_for_statement(statement) and statement.allow_self_approval and not explicit_deny_self_approval:
             return AccessRequestDecision(
                 grant=True,
                 reason=DecisionReason.SelfApproval,
@@ -110,6 +129,7 @@ def make_decision_on_access_request(  # noqa: PLR0911, PLR0913
 
         decision_based_on_statements.add(statement)  # type: ignore # noqa: PGH003
         potential_approvers.update(approver for approver in statement.approvers if approver != requester_email)
+        potential_approver_groups.update(statement.approver_groups)
 
     if not decision_based_on_statements:
         return AccessRequestDecision(
@@ -118,7 +138,7 @@ def make_decision_on_access_request(  # noqa: PLR0911, PLR0913
             based_on_statements=frozenset(decision_based_on_statements),
         )
 
-    if not potential_approvers:
+    if not potential_approvers and not potential_approver_groups:
         return AccessRequestDecision(
             grant=False,
             reason=DecisionReason.NoApprovers,
@@ -129,6 +149,7 @@ def make_decision_on_access_request(  # noqa: PLR0911, PLR0913
         grant=False,
         reason=DecisionReason.RequiresApproval,
         approvers=frozenset(potential_approvers),
+        approver_groups=frozenset(potential_approver_groups),
         based_on_statements=frozenset(decision_based_on_statements),
     )
 
@@ -172,11 +193,38 @@ def make_decision_on_approve_request(  # noqa: PLR0913
     account_id: str | None = None,
     group_id: str | None = None,
     permission_set_arn: str | None = None,
+    approver_slack_id: str | None = None,
+    approver_group_resolver: Callable[[frozenset[str]], set[str]] | None = None,
 ) -> ApproveRequestDecision:
+    """Make a decision on an approval request.
+
+    Args:
+        action: The action being taken (Approve or Deny)
+        statements: The statements to evaluate
+        approver_email: Email of the approver
+        requester_email: Email of the requester
+        permission_set_name: Name of the permission set
+        account_id: AWS account ID
+        group_id: SSO group ID for group-based access
+        permission_set_arn: ARN of the permission set
+        approver_slack_id: Slack ID of the approver (for checking group membership)
+        approver_group_resolver: Function that resolves approver group IDs to Slack user IDs.
+            Takes a frozenset of group IDs and returns a set of Slack user IDs.
+            Resolution is done per-statement to prevent cross-statement authorization bypass.
+    """
     affected_statements = determine_affected_statements(statements, account_id, permission_set_name, group_id, permission_set_arn)
 
     for statement in affected_statements:
-        if approver_email in statement.approvers:
+        is_individual_approver = approver_email in statement.approvers
+
+        # Check if approver is in THIS statement's approver groups (not globally)
+        is_group_approver = False
+        if approver_slack_id is not None and approver_group_resolver is not None and statement.approver_groups:
+            # Resolve only THIS statement's groups
+            statement_group_user_ids = approver_group_resolver(statement.approver_groups)
+            is_group_approver = approver_slack_id in statement_group_user_ids
+
+        if is_individual_approver or is_group_approver:
             is_self_approval = approver_email == requester_email
             if is_self_approval and statement.allow_self_approval or not is_self_approval:
                 return ApproveRequestDecision(

src/config.py

@@ -83,6 +83,7 @@ def to_set_if_list_or_str(v: list | str) -> frozenset[str]:
             "permission_set": to_set_if_list_or_str(_dict["PermissionSet"]),
             "resource": to_set_if_list_or_str(_dict["Resource"]),
             "approvers": to_set_if_list_or_str(_dict.get("Approvers", set())),
+            "approver_groups": to_set_if_list_or_str(_dict.get("ApproverGroups", set())),
             "resource_type": _dict.get("ResourceType"),
             "approval_is_not_required": _dict.get("ApprovalIsNotRequired"),
             "allow_self_approval": _dict.get("AllowSelfApproval"),
@@ -101,6 +102,7 @@ def to_set_if_list_or_str(v: list | str) -> frozenset[str]:
         {
             "resource": to_set_if_list_or_str(_dict["Resource"]),
             "approvers": to_set_if_list_or_str(_dict.get("Approvers", set())),
+            "approver_groups": to_set_if_list_or_str(_dict.get("ApproverGroups", set())),
             "approval_is_not_required": _dict.get("ApprovalIsNotRequired"),
             "allow_self_approval": _dict.get("AllowSelfApproval"),
         }
@@ -160,7 +162,7 @@ class Config(BaseSettings):
 
     waiting_result_emoji: str = ":large_yellow_circle:"
     bad_result_emoji: str = ":red_circle:"
-    discarded_result_emoji: str = ":white_circle:"
+    denied_result_emoji: str = ":white_circle:"
 
     # Status badges for Slack messages
     pending_status: str = ":hourglass_flowing_sand: *AWAITING APPROVAL*"

src/entities/slack.py

@@ -5,7 +5,7 @@
 
 class ApproverAction(Enum):
     Approve = "approve"
-    Discard = "discard"
+    Deny = "deny"
     EarlyRevoke = "early_revoke"