NEEDS TESTING - Support multiple Cognito user groups per node ID - #385
Draft
jordanpadams wants to merge 2 commits into
Draft
NEEDS TESTING - Support multiple Cognito user groups per node ID#385jordanpadams wants to merge 2 commits into
jordanpadams wants to merge 2 commits into
Conversation
Replaces auto-generated single group names with an explicit node_id_to_cognito_groups map in NodeUtil. SBN now maps to both PDS_SBN_USERS and PDS_SBNUMD_USERS. All client callsites send a comma-separated UserGroup header; the Lambda authorizer authorizes if the user belongs to any group in the list. Fixes #384 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jordanpadams
marked this pull request as draft
August 28, 2026 15:21
jordanpadams
changed the title
jordanpadams
changed the title
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
Fixes a 401 Unauthorized error for SBN users whose Cognito account is in
PDS_SBNUMD_USERSrather thanPDS_SBN_USERS. The authorizer previously checked the user's groups against a single expected group name, so any node with more than one Cognito group (e.g. SBN with separate UMD and PSI orgs) would silently reject valid users.Changes:
node_util.py: Replaces the auto-generated single group name pattern with an explicitnode_id_to_cognito_groupsmap.sbnmaps to["PDS_SBN_USERS", "PDS_SBNUMD_USERS"]; all other nodes map to their single group. Thenode_id_to_group_names()method returns the full list.pds_ingress_client.py,pds_status_client.py,log_util.py: AllUserGroupheader callsites updated to send a comma-separated string of all permitted groups for the node.authorizer/index.js: Splits theUserGroupheader on commas and authorizes if the user belongs to any group in the list (permitted_groups.some(g => groups.includes(g))).test_log_util.py: Updated assertion to match the new comma-joined header format.AI assistance: ~90% of the code changes were written with Claude Code assistance.
⚙️ Test Data and/or Report
All 44 existing unit tests pass (
pytest tests/ -x -q). The authorizer change is JavaScript and covered by manual review; the behavior change is minimal (single-group nodes are unaffected since a one-element comma-split is identical to the previous exact match).Integration test against LocalStack with an SBN user in
PDS_SBNUMD_USERSis recommended before merging to production.♻️ Related Issues
Fixes #384
🤓 Reviewer Checklist