Remove SSN endpoint from Cosmetology API#1489
Open
landonshumway-ia wants to merge 7 commits intocsg-org:mainfrom
Open
Remove SSN endpoint from Cosmetology API#1489landonshumway-ia wants to merge 7 commits intocsg-org:mainfrom
landonshumway-ia wants to merge 7 commits intocsg-org:mainfrom
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 47 minutes and 3 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (26)
📝 WalkthroughWalkthroughRemoved provider SSN read capability across the codebase: SSN schema, handler, endpoint, rate-limiting, permissions/scopes, infra wiring (lambda, grants, alarms, exports), persistence access, smoke/tests, and related snapshots/fixtures. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as API_Gateway
participant Lambda as SSN_Lambda
participant RateDB as RateLimit_DynamoDB
participant ProviderDB as Provider_DynamoDB
participant KMS
participant CloudWatch
Client->>API: GET /providers/{id}/ssn
API->>Lambda: invoke
Lambda->>RateDB: check rate-limit
alt under limit
Lambda->>ProviderDB: query encrypted SSN
ProviderDB-->>Lambda: encrypted SSN
Lambda->>KMS: decrypt
KMS-->>Lambda: plaintext SSN
Lambda->>Client: 200 + SSN
Lambda->>CloudWatch: emit metrics/alarms
else rate limited
Lambda->>Client: 429
Lambda->>CloudWatch: emit throttling metric
end
sequenceDiagram
participant Client
participant API as API_Gateway
participant CloudWatch
Client->>API: GET /providers/{id}/ssn
API->>Client: 404 / Method Not Found (endpoint removed)
Note right of API: No SSN Lambda, no RateDB, no KMS, no SSN metrics/alarms
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend/cosmetology-app/lambdas/python/provider-data-v1/handlers/providers.py`:
- Around line 1-14: Residual readSSN permission artifacts remain after removing
the get_provider_ssn handler; update the PR by adding a clear note (or create a
follow-up ticket) stating this is a phased cleanup and listing the remaining
symbols/integration points to remove later (readSSN scope generation in
staff-user-pre-token lambda, readSSN boolean in API schema/api_model, tests
verifying scope generation, and the utility
user_has_read_ssn_access_for_provider), and reference that staff-user data
migration must complete before final removal so reviewers and future maintainers
are aware of the planned staged deletion.
In `@backend/cosmetology-app/stacks/persistent_stack/ssn_table.py`:
- Around line 257-275: Remove the unused CloudFormation export for
self.api_query_role.role_name (or both exports if no stack imports either) so
phase-2 cleanup isn't blocked; confirm which attribute the cosmetology API stack
imports and keep only self.api_query_role.role_arn if that's the sole consumer,
otherwise drop both exports. Also update the NagSuppressions reason string for
the ApiQuery role (reference self.api_query_role and the existing
NagSuppressions.add_resource_suppressions_by_path call) to: "This role is inert
(nothing assumes it) even though AWSLambdaBasicExecutionRole is attached for
backwards compatibility during staged removal." Ensure the export removal and
updated reason are committed together.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b25c99a6-ad90-4939-a687-2a345ab74f1a
📒 Files selected for processing (15)
backend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/provider/api.pybackend/cosmetology-app/lambdas/python/provider-data-v1/handlers/providers.pybackend/cosmetology-app/lambdas/python/provider-data-v1/tests/function/test_handlers/test_providers.pybackend/cosmetology-app/stacks/api_lambda_stack/provider_management.pybackend/cosmetology-app/stacks/api_stack/v1_api/api.pybackend/cosmetology-app/stacks/api_stack/v1_api/api_model.pybackend/cosmetology-app/stacks/api_stack/v1_api/provider_management.pybackend/cosmetology-app/stacks/persistent_stack/__init__.pybackend/cosmetology-app/stacks/persistent_stack/ssn_table.pybackend/cosmetology-app/tests/app/base.pybackend/cosmetology-app/tests/app/test_api/test_provider_management_api.pybackend/cosmetology-app/tests/smoke/README.mdbackend/cosmetology-app/tests/smoke/smoke_common.pybackend/cosmetology-app/tests/smoke/smoke_tests_env_example.jsonbackend/cosmetology-app/tests/smoke/ssn_read_throttling_smoke_tests.py
💤 Files with no reviewable changes (11)
- backend/cosmetology-app/tests/smoke/README.md
- backend/cosmetology-app/tests/smoke/smoke_tests_env_example.json
- backend/cosmetology-app/tests/smoke/smoke_common.py
- backend/cosmetology-app/stacks/persistent_stack/init.py
- backend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/provider/api.py
- backend/cosmetology-app/stacks/api_stack/v1_api/provider_management.py
- backend/cosmetology-app/stacks/api_stack/v1_api/api.py
- backend/cosmetology-app/tests/app/test_api/test_provider_management_api.py
- backend/cosmetology-app/stacks/api_stack/v1_api/api_model.py
- backend/cosmetology-app/stacks/api_lambda_stack/provider_management.py
- backend/cosmetology-app/tests/smoke/ssn_read_throttling_smoke_tests.py
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/cosmetology-app/lambdas/python/staff-user-pre-token/tests/test_user_scopes.py (1)
23-27:⚠️ Potential issue | 🔴 CriticalRemove stale
READ_SSNreferences fromUserData.The
CCPermissionsActionenum no longer definesREAD_SSN, butUserDatastill references it at lines 57, 68–69, and 91. This will causeAttributeErrorwhen staff tokens are generated in production (called frommain.py:30), breaking the staff-user-pre-token Lambda before any test assertions can run.Required fix in
user_data.py# backend/cosmetology-app/lambdas/python/staff-user-pre-token/user_data.py disallowed_actions = compact_actions - { CCPermissionsAction.READ, CCPermissionsAction.ADMIN, CCPermissionsAction.READ_PRIVATE, - CCPermissionsAction.READ_SSN, } ... -if CCPermissionsAction.READ_SSN in compact_actions: - self.scopes.add(f'{compact_abbr}/{CCPermissionsAction.READ_SSN}') - if CCPermissionsAction.ADMIN in compact_actions: self.scopes.add(f'{compact_abbr}/{CCPermissionsAction.ADMIN}') ... disallowed_actions = jurisdiction_actions - { CCPermissionsAction.WRITE, CCPermissionsAction.ADMIN, CCPermissionsAction.READ_PRIVATE, - CCPermissionsAction.READ_SSN, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/cosmetology-app/lambdas/python/staff-user-pre-token/tests/test_user_scopes.py` around lines 23 - 27, UserData currently references the removed enum member CCPermissionsAction.READ_SSN (at the occurrences in user_data.py corresponding to the checks around the constructor/permission logic), causing AttributeError; open user_data.py and remove all uses of CCPermissionsAction.READ_SSN (lines referenced in the review) and replace those checks with the appropriate existing enum values (e.g., CCPermissionsAction.READ or CCPermissionsAction.READ_PRIVATE) or collapse the condition into the existing permission logic so no code depends on READ_SSN; ensure any unit tests or callers that expected READ_SSN behavior are covered by the new checks and that imports/attribute access to CCPermissionsAction no longer reference READ_SSN.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@backend/cosmetology-app/lambdas/python/staff-user-pre-token/tests/test_user_scopes.py`:
- Around line 23-27: UserData currently references the removed enum member
CCPermissionsAction.READ_SSN (at the occurrences in user_data.py corresponding
to the checks around the constructor/permission logic), causing AttributeError;
open user_data.py and remove all uses of CCPermissionsAction.READ_SSN (lines
referenced in the review) and replace those checks with the appropriate existing
enum values (e.g., CCPermissionsAction.READ or CCPermissionsAction.READ_PRIVATE)
or collapse the condition into the existing permission logic so no code depends
on READ_SSN; ensure any unit tests or callers that expected READ_SSN behavior
are covered by the new checks and that imports/attribute access to
CCPermissionsAction no longer reference READ_SSN.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08d87105-b318-4f9e-a19a-5b325afda54e
📒 Files selected for processing (10)
backend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/common.pybackend/cosmetology-app/lambdas/python/staff-user-pre-token/tests/test_user_scopes.pybackend/cosmetology-app/stacks/api_stack/v1_api/api_model.pybackend/cosmetology-app/tests/app/base.pybackend/cosmetology-app/tests/app/test_pipeline.pybackend/cosmetology-app/tests/resources/snapshots/JURISDICTION_RESOURCE_SERVER_CONFIGURATION.jsonbackend/cosmetology-app/tests/resources/snapshots/PATCH_STAFF_USERS_REQUEST_SCHEMA.jsonbackend/cosmetology-app/tests/resources/snapshots/PATCH_STAFF_USERS_RESPONSE_SCHEMA.jsonbackend/cosmetology-app/tests/resources/snapshots/POST_STAFF_USERS_REQUEST_SCHEMA.jsonbackend/cosmetology-app/tests/resources/snapshots/POST_STAFF_USERS_RESPONSE_SCHEMA.json
💤 Files with no reviewable changes (6)
- backend/cosmetology-app/tests/resources/snapshots/PATCH_STAFF_USERS_RESPONSE_SCHEMA.json
- backend/cosmetology-app/lambdas/python/common/cc_common/data_model/schema/common.py
- backend/cosmetology-app/tests/resources/snapshots/PATCH_STAFF_USERS_REQUEST_SCHEMA.json
- backend/cosmetology-app/tests/resources/snapshots/POST_STAFF_USERS_REQUEST_SCHEMA.json
- backend/cosmetology-app/tests/resources/snapshots/JURISDICTION_RESOURCE_SERVER_CONFIGURATION.json
- backend/cosmetology-app/tests/resources/snapshots/POST_STAFF_USERS_RESPONSE_SCHEMA.json
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/cosmetology-app/tests/app/base.py
- backend/cosmetology-app/stacks/api_stack/v1_api/api_model.py
landonshumway-ia
force-pushed
the
chore/remove-cosm-ssn-reader
branch
from
a0c56fa to
1449447
Compare
Collaborator
Author
|
@jlkravitz This is ready for your review. Thanks |
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
2 participants
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.
The Cosmetology API does not need to support reading the full SSN of a practitioner. As a security measure we are removing that API endpoint and the permissions to query the SSN table. This will require two full deployments to all environments, the first to remove the API resources, then the second to remove the IAM role from the persistent stack.
Closes #1474
Summary by CodeRabbit
Chores
Tests
Documentation