feat: machine auth eligibility API - #3
Open
raphapr wants to merge 6 commits into
Open
Conversation
- New Lambda function to get user eligibility by email - Returns accounts, permissions, maxDuration, approvalRequired - Supports machine auth via api/admin scope - Also adds getPermissions machine auth support
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: loadsmart/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughIntroduces a new GraphQL query Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant AppSync as AppSync<br/>(GraphQL)
participant Lambda as Lambda<br/>(teamgetEligibilityForUser)
participant SSO as AWS SSO<br/>Instance
participant IdentityStore as Identity Store<br/>(User/Groups)
participant DynamoDB as DynamoDB<br/>(Entitlements)
participant Orgs as AWS Organizations<br/>(OUs→Accounts)
User->>AppSync: getEligibilityForUser(email)
AppSync->>Lambda: invoke with email
Lambda->>SSO: get_sso_instance()
SSO-->>Lambda: Identity Store ID
Lambda->>IdentityStore: get_user_by_email(email)
IdentityStore-->>Lambda: UserId
Lambda->>IdentityStore: list_group_memberships(UserId)
IdentityStore-->>Lambda: [GroupIds]
Lambda->>DynamoDB: get_entitlements(UserId)
DynamoDB-->>Lambda: entitlements_user
loop For each GroupId
Lambda->>DynamoDB: get_entitlements(GroupId)
DynamoDB-->>Lambda: entitlements_group
end
loop For each OU in entitlements
Lambda->>Orgs: list_accounts_for_ou(OU)
Orgs-->>Lambda: [AccountIds]
end
Lambda->>Lambda: merge_eligibility(all_entitlements)
Lambda-->>AppSync: EligibilityResult
AppSync-->>User: {accounts, permissions, maxDuration, approvalRequired}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
raphapr
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
amplify/backend/function/teamgetEligibilityForUser/src/index.py (1)
132-134: Normalize email before lookup.Identity Center email lookup via
emails.valueis typically case-sensitive on stored value. Consider trimming and lower-casing the input to match common provisioning normalization, and validating minimally that it looks like an email before calling downstream APIs.- email = event.get('arguments', {}).get('email') - if not email: - raise Exception("Email is required") + email = (event.get('arguments', {}).get('email') or '').strip().lower() + if not email or '@' not in email: + raise Exception("A valid email is required")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@amplify/backend/function/teamgetEligibilityForUser/src/index.py` around lines 132 - 134, Normalize and validate the provided email before performing the lookup: in the handler where you read the argument into the email variable (email = event.get('arguments', {}).get('email')), trim whitespace and convert to lowercase (e.g., email = email.strip().lower()), then perform a minimal validation (ensure it contains an '@' and a domain portion) and raise the existing "Email is required" or a validation Exception if it fails; use the normalized email for all downstream calls to Identity Center/teams lookup to avoid case-sensitivity mismatches.amplify/backend/function/teamgetEligibilityForUser/teamgetEligibilityForUser-cloudformation-template.json (1)
197-219: Verify Identity Store/Organizations scoping.
sso:ListInstances,sso-admin:ListInstances,identitystore:*, andorganizations:ListAccountsForParentdon't support resource-level ARNs, so"*"is correct. Consider adding anaws:ResourceAccountcondition on the Organizations action to pin to the management account if this Lambda runs outside it, but not a blocker for the current deployment model.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@amplify/backend/function/teamgetEligibilityForUser/teamgetEligibilityForUser-cloudformation-template.json` around lines 197 - 219, The policy allows "organizations:ListAccountsForParent" with "Resource": "*" — add a condition to scope it to the management account by including an "Condition" block (e.g., "StringEquals": {"aws:ResourceAccount": "<management-account-id>"} ) on the statement that contains the "organizations:ListAccountsForParent" action; leave the SSO and Identity Store statements as-is since they require "*" resources. Locate the statement containing the "organizations:ListAccountsForParent" action and add the Condition there, replacing "<management-account-id>" with the actual management account ID.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@amplify/backend/function/teamgetEligibilityForUser/src/index.py`:
- Around line 13-17: The get_sso_instance function currently assumes
response['Instances'][0] exists and will raise IndexError if Identity Center
isn't configured; update get_sso_instance to check that 'Instances' exists and
is non-empty on the response before indexing, and if empty raise a clear,
explicit exception (e.g., ValueError or a custom exception) with a descriptive
message like "No Identity Center (SSO) instances found in this account/region"
or return an explicit None; ensure callers handle this new error/None and keep
the boto3 client/list_instances call and function name get_sso_instance
unchanged.
- Around line 96-103: The current int(entitlement.get('duration', 0)) is brittle
and can raise for None or non-numeric strings; change to defensively coerce
duration from entitlement: pull raw = entitlement.get('duration'), treat
None/empty as 0, then try converting with int(raw), falling back to
int(float(raw)) in case of decimal strings, and catch ValueError/TypeError to
skip/ignore malformed values (optionally log a warning) before
comparing/updating max_duration; leave the approvalRequired check
(entitlement.get('approvalRequired', True)) as-is. Ensure you update the code
that references entitlement, duration, and max_duration accordingly.
- Around line 129-147: Logs currently print raw PII (email) in the handler;
replace any direct prints of the event and email (the print call and the line
that prints f"User {email} is in groups...") with non-PII identifiers such as
the resolved user_id from get_user_by_email or a one-way hash/truncated digest
of the email (e.g., sha256 and first N chars) before logging; ensure you still
log useful context like group_ids and use the functions get_sso_instance,
get_user_by_email, and list_group_memberships to obtain and log only the safe
identifier.
- Around line 67-82: list_accounts_for_ou currently only returns direct-child
accounts; update it to recursively traverse child OUs using
organizations:list_organizational_units_for_parent and aggregate accounts from
each OU (call list_accounts_for_parent for each OU and recurse into nested OUs)
so nested accounts are included; also ensure the IAM policy (e.g., the
teamPublishOUs/CloudFormation role) grants
organizations:ListOrganizationalUnitsForParent (or organizations:List* as used
elsewhere) so the recursive calls succeed.
---
Nitpick comments:
In `@amplify/backend/function/teamgetEligibilityForUser/src/index.py`:
- Around line 132-134: Normalize and validate the provided email before
performing the lookup: in the handler where you read the argument into the email
variable (email = event.get('arguments', {}).get('email')), trim whitespace and
convert to lowercase (e.g., email = email.strip().lower()), then perform a
minimal validation (ensure it contains an '@' and a domain portion) and raise
the existing "Email is required" or a validation Exception if it fails; use the
normalized email for all downstream calls to Identity Center/teams lookup to
avoid case-sensitivity mismatches.
In
`@amplify/backend/function/teamgetEligibilityForUser/teamgetEligibilityForUser-cloudformation-template.json`:
- Around line 197-219: The policy allows "organizations:ListAccountsForParent"
with "Resource": "*" — add a condition to scope it to the management account by
including an "Condition" block (e.g., "StringEquals": {"aws:ResourceAccount":
"<management-account-id>"} ) on the statement that contains the
"organizations:ListAccountsForParent" action; leave the SSO and Identity Store
statements as-is since they require "*" resources. Locate the statement
containing the "organizations:ListAccountsForParent" action and add the
Condition there, replacing "<management-account-id>" with the actual management
account ID.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: loadsmart/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0db32849-27b4-4e5f-ad5e-976bf79f4af9
📒 Files selected for processing (5)
amplify/backend/api/team/schema.graphqlamplify/backend/backend-config.jsonamplify/backend/function/teamgetEligibilityForUser/function-parameters.jsonamplify/backend/function/teamgetEligibilityForUser/src/index.pyamplify/backend/function/teamgetEligibilityForUser/teamgetEligibilityForUser-cloudformation-template.json
Comment on lines
+13
to
+17
| def get_sso_instance(): | ||
| """Get SSO instance details""" | ||
| client = boto3.client('sso-admin') | ||
| response = client.list_instances() | ||
| return response['Instances'][0] |
There was a problem hiding this comment.
Guard against empty Instances list.
response['Instances'][0] raises IndexError (surfaced to the caller as a 500) if Identity Center is not configured in the account/region. A clearer error message aids operator debugging.
🛡️ Proposed fix
def get_sso_instance():
"""Get SSO instance details"""
client = boto3.client('sso-admin')
response = client.list_instances()
- return response['Instances'][0]
+ instances = response.get('Instances') or []
+ if not instances:
+ raise Exception("No IAM Identity Center instance found in this account/region")
+ return instances[0]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def get_sso_instance(): | |
| """Get SSO instance details""" | |
| client = boto3.client('sso-admin') | |
| response = client.list_instances() | |
| return response['Instances'][0] | |
| def get_sso_instance(): | |
| """Get SSO instance details""" | |
| client = boto3.client('sso-admin') | |
| response = client.list_instances() | |
| instances = response.get('Instances') or [] | |
| if not instances: | |
| raise Exception("No IAM Identity Center instance found in this account/region") | |
| return instances[0] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@amplify/backend/function/teamgetEligibilityForUser/src/index.py` around lines
13 - 17, The get_sso_instance function currently assumes
response['Instances'][0] exists and will raise IndexError if Identity Center
isn't configured; update get_sso_instance to check that 'Instances' exists and
is non-empty on the response before indexing, and if empty raise a clear,
explicit exception (e.g., ValueError or a custom exception) with a descriptive
message like "No Identity Center (SSO) instances found in this account/region"
or return an explicit None; ensure callers handle this new error/None and keep
the boto3 client/list_instances call and function name get_sso_instance
unchanged.
Comment on lines
+67
to
+82
| def list_accounts_for_ou(ou_id): | ||
| """List all accounts in an OU""" | ||
| client = boto3.client('organizations') | ||
| accounts = [] | ||
| try: | ||
| paginator = client.get_paginator('list_accounts_for_parent') | ||
| pages = paginator.paginate(ParentId=ou_id) | ||
| for page in pages: | ||
| for account in page.get('Accounts', []): | ||
| accounts.append({ | ||
| 'id': account['Id'], | ||
| 'name': account['Name'] | ||
| }) | ||
| except ClientError as e: | ||
| print(f"Error listing accounts for OU {ou_id}: {e}") | ||
| return accounts |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether other Lambdas in the repo already implement OU→accounts expansion,
# to see what the established behavior/contract is for `ous` in Eligibility records.
rg -nP -C3 '\blist_accounts_for_parent\b|\blist_organizational_units_for_parent\b'
rg -nP -C3 "'ous'|\"ous\"" --type=pyRepository: loadsmart/iam-identity-center-team
Length of output: 5685
🏁 Script executed:
#!/bin/bash
# Check CloudFormation templates for current policy grants
fd -e yaml -e yml -e json | xargs grep -l "Organizations\|organizations" | head -20Repository: loadsmart/iam-identity-center-team
Length of output: 1612
🏁 Script executed:
#!/bin/bash
# Search for CloudFormation policy definitions related to organizations
fd -e yaml -e yml -e json | xargs grep -l "organizations\|ListAccounts\|ListOrganizational" 2>/dev/nullRepository: loadsmart/iam-identity-center-team
Length of output: 1530
🏁 Script executed:
#!/bin/bash
# Look for comments or TODOs about OU recursion or nested structures
rg -i "nested|recursiv|ou.*tree|organizational.*unit" --type=py -B2 -A2Repository: loadsmart/iam-identity-center-team
Length of output: 2406
🏁 Script executed:
#!/bin/bash
# Check the policy for teamgetEligibilityForUser
cat amplify/backend/function/teamgetEligibilityForUser/custom-policies.json 2>/dev/null || echo "File not found"Repository: loadsmart/iam-identity-center-team
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Check the policy for teamPublishOUs which has the recursive implementation
cat amplify/backend/function/teamPublishOUs/custom-policies.jsonRepository: loadsmart/iam-identity-center-team
Length of output: 193
🏁 Script executed:
#!/bin/bash
# Also check the main CF template for teamgetEligibilityForUser
cat amplify/backend/function/teamgetEligibilityForUser/teamgetEligibilityForUser-cloudformation-template.jsonRepository: loadsmart/iam-identity-center-team
Length of output: 6202
list_accounts_for_ou does not recurse into nested OUs.
organizations:list_accounts_for_parent returns only accounts that are direct children of the given parent; accounts nested under child OUs are not included. When Eligibility records reference parent OUs with nested structures, users will silently receive an incomplete account list. This is a systemic issue affecting teamgetEligibilityForUser, teamRouter, and teamgetEntitlement — all use the same non-recursive pattern.
Consider implementing recursion via list_organizational_units_for_parent and aggregating results, or document that only direct-child accounts are supported. If recursing, grant organizations:ListOrganizationalUnitsForParent in the CloudFormation policy (example: teamPublishOUs grants both permissions via wildcard organizations:List*).
♻️ Sketch of a recursive expansion
def list_accounts_for_ou(ou_id):
- """List all accounts in an OU"""
+ """Recursively list all accounts in an OU (including nested OUs)."""
client = boto3.client('organizations')
accounts = []
try:
- paginator = client.get_paginator('list_accounts_for_parent')
- pages = paginator.paginate(ParentId=ou_id)
- for page in pages:
+ acc_pages = client.get_paginator('list_accounts_for_parent').paginate(ParentId=ou_id)
+ for page in acc_pages:
for account in page.get('Accounts', []):
accounts.append({'id': account['Id'], 'name': account['Name']})
+ ou_pages = client.get_paginator('list_organizational_units_for_parent').paginate(ParentId=ou_id)
+ for page in ou_pages:
+ for child in page.get('OrganizationalUnits', []):
+ accounts.extend(list_accounts_for_ou(child['Id']))
except ClientError as e:
print(f"Error listing accounts for OU {ou_id}: {e}")
return accounts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@amplify/backend/function/teamgetEligibilityForUser/src/index.py` around lines
67 - 82, list_accounts_for_ou currently only returns direct-child accounts;
update it to recursively traverse child OUs using
organizations:list_organizational_units_for_parent and aggregate accounts from
each OU (call list_accounts_for_parent for each OU and recurse into nested OUs)
so nested accounts are included; also ensure the IAM policy (e.g., the
teamPublishOUs/CloudFormation role) grants
organizations:ListOrganizationalUnitsForParent (or organizations:List* as used
elsewhere) so the recursive calls succeed.
Comment on lines
+96
to
+103
| # Track max duration | ||
| duration = int(entitlement.get('duration', 0)) | ||
| if duration > max_duration: | ||
| max_duration = duration | ||
|
|
||
| # If any entitlement doesn't require approval, set to False | ||
| if not entitlement.get('approvalRequired', True): | ||
| approval_required = False |
There was a problem hiding this comment.
int(duration) is fragile against None / non-numeric values.
If a DynamoDB item stores duration as null or a non-integer string (e.g. ISO-8601 "PT4H" or a decimal), int(...) raises and the whole query fails. The schema types Eligibility.duration as String, so any string shape is accepted on write. Coerce defensively:
🛡️ Proposed fix
- # Track max duration
- duration = int(entitlement.get('duration', 0))
- if duration > max_duration:
- max_duration = duration
+ # Track max duration, tolerating missing / non-numeric values.
+ raw_duration = entitlement.get('duration') or 0
+ try:
+ duration = int(raw_duration)
+ except (TypeError, ValueError):
+ print(f"Ignoring non-numeric duration: {raw_duration!r}")
+ duration = 0
+ if duration > max_duration:
+ max_duration = duration🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@amplify/backend/function/teamgetEligibilityForUser/src/index.py` around lines
96 - 103, The current int(entitlement.get('duration', 0)) is brittle and can
raise for None or non-numeric strings; change to defensively coerce duration
from entitlement: pull raw = entitlement.get('duration'), treat None/empty as 0,
then try converting with int(raw), falling back to int(float(raw)) in case of
decimal strings, and catch ValueError/TypeError to skip/ignore malformed values
(optionally log a warning) before comparing/updating max_duration; leave the
approvalRequired check (entitlement.get('approvalRequired', True)) as-is. Ensure
you update the code that references entitlement, duration, and max_duration
accordingly.
Comment on lines
+129
to
+147
| print(f"Received event: {event}") | ||
|
|
||
| # Extract email from arguments | ||
| email = event.get('arguments', {}).get('email') | ||
| if not email: | ||
| raise Exception("Email is required") | ||
|
|
||
| # Get SSO instance | ||
| sso_instance = get_sso_instance() | ||
| identity_store_id = sso_instance['IdentityStoreId'] | ||
|
|
||
| # Look up user by email | ||
| user_id = get_user_by_email(identity_store_id, email) | ||
| if not user_id: | ||
| raise Exception(f"User with email '{email}' not found in Identity Center") | ||
|
|
||
| # Get user's group memberships | ||
| group_ids = list_group_memberships(identity_store_id, user_id) | ||
| print(f"User {email} is in groups: {group_ids}") |
There was a problem hiding this comment.
Avoid logging user email (PII).
print(f"Received event: {event}") and print(f"User {email} is in groups: {group_ids}") emit the user's email to CloudWatch Logs. This persists PII in log storage and can trigger GDPR/CCPA retention obligations. Log a hashed/truncated identifier or just the user_id after lookup.
🔒 Proposed fix
- print(f"Received event: {event}")
+ # Avoid logging full event (contains user email).
+ print("Received getEligibilityForUser event")
@@
- # Get user's group memberships
group_ids = list_group_memberships(identity_store_id, user_id)
- print(f"User {email} is in groups: {group_ids}")
+ print(f"User {user_id} is in {len(group_ids)} group(s)")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@amplify/backend/function/teamgetEligibilityForUser/src/index.py` around lines
129 - 147, Logs currently print raw PII (email) in the handler; replace any
direct prints of the event and email (the print call and the line that prints
f"User {email} is in groups...") with non-PII identifiers such as the resolved
user_id from get_user_by_email or a one-way hash/truncated digest of the email
(e.g., sha256 and first N chars) before logging; ensure you still log useful
context like group_ids and use the functions get_sso_instance,
get_user_by_email, and list_group_memberships to obtain and log only the safe
identifier.
- Add email format validation (regex check) - Change error message to generic to prevent user enumeration - Remove logging of email, group IDs, and eligibility results
raphapr
force-pushed
the
feat/machine-auth-eligibility-api
branch
from
April 23, 2026 19:53
9aba28e to
a5c83f3
Compare
- Guard against empty Instances list in get_sso_instance() - Defensive coercion for duration (handles None/non-numeric)
- Replace event logging with generic message - Log user_id instead of email - Log counts instead of full eligibility result
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds machine-to-machine authentication support for querying user eligibility, enabling machine user for access requests.
Motivation
The existing
createRequestOnBehalfmutation allows machine clients to submit access requests, but they couldn't discover what accounts/permissions a user is eligible for. This PR adds the missing query capability.Changes
getEligibilityForUser(email: String!)query — New Lambda that looks up a user by email in Identity Center, retrieves their group memberships, and returns all eligible accounts, permissions, max duration, and approval requirementsgetPermissionsmachine auth — Addedapi/adminscope to existing query so machine clients can list permission setssso:ListInstancespermission (AWS uses bothsso:andsso-admin:prefixes)Summary by CodeRabbit