[Representative Status] 135311: Log Details about POA Unauthorized Errors#27788
[Representative Status] 135311: Log Details about POA Unauthorized Errors#27788kristen-brown merged 4 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances observability around 403 authorization failures for the Representative Status “Power of Attorney” endpoint by emitting structured logs from the PowerOfAttorneyPolicy when access is denied, along with unit test updates to validate the logging behavior.
Changes:
- Add structured
Rails.logger.infologging whenPowerOfAttorneyPolicy#access?denies access. - Include LOA and presence signals for LOA3/ICN/participant_id in the denial log payload.
- Update the policy spec to assert the new logging behavior for denial cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| app/policies/power_of_attorney_policy.rb | Adds a denial logging helper invoked when the policy denies access. |
| spec/policies/power_of_attorney_policy_spec.rb | Extends unit tests to assert logging on denials and no logging on success. |
|
|
||
| it 'grants access' do | ||
| it 'grants access and does not log' do | ||
| expect(Rails.logger).not_to receive(:info) |
There was a problem hiding this comment.
expect(Rails.logger).not_to receive(:info) is very broad and can make this spec brittle if any unrelated info log happens during policy evaluation (now or in the future). It’s safer to assert specifically that the POA denial log is not emitted (e.g., not_to receive(:info).with('POA ACCESS DENIED', anything) or similar), while allowing other info logs to occur.
| expect(Rails.logger).not_to receive(:info) | |
| expect(Rails.logger).not_to receive(:info).with('POA ACCESS DENIED', anything) |
| def log_access_denied | ||
| Rails.logger.info('POA ACCESS DENIED', | ||
| loa: user.loa&.dig(:current), | ||
| loa3: user.loa3?, | ||
| icn: user.icn.present?, | ||
| participant_id: user.participant_id.present?) |
There was a problem hiding this comment.
log_access_denied unconditionally calls user.participant_id.present?. For LOA3 users where access is denied because user.icn is missing, the original access check short-circuits before touching participant_id, but the new logging will still evaluate it—this can trigger an MVI/MPI profile lookup just to populate a log field, adding latency/external dependency to an already-failing authorization path. Consider deriving the logged booleans from values already computed in access? (or only checking participant_id when icn is present), so denials due to missing ICN/LOA don’t incur an extra MPI call.
There was a problem hiding this comment.
The MPI profile response is cached in Redis, so the point about incurring an extra MPI call isn't really accurate.
I think the changes recommended here would unnecessarily complicate the code, and they would also keep us from logging information that we want to know. If we didn't check the participant_id when icn isn't present, we wouldn't learn when a user has neither present, and that's one of the scenarios we're looking to learn about by adding this log.
| loa: user.loa&.dig(:current), | ||
| loa3: user.loa3?, | ||
| icn: user.icn.present?, | ||
| participant_id: user.participant_id.present?) |
There was a problem hiding this comment.
The log payload uses keys icn: and participant_id: but the values are booleans. In this codebase, presence booleans are typically logged with explicit *_present keys (e.g., icn_present, participant_id_present), which avoids confusion with logs that use icn/participant_id for the actual identifiers and makes it clear no PII is being emitted. Consider renaming these fields (and updating the spec expectations) to icn_present: and participant_id_present: (and optionally making loa more explicit, e.g., loa_current).
| loa: user.loa&.dig(:current), | |
| loa3: user.loa3?, | |
| icn: user.icn.present?, | |
| participant_id: user.participant_id.present?) | |
| loa_current: user.loa&.dig(:current), | |
| loa3: user.loa3?, | |
| icn_present: user.icn.present?, | |
| participant_id_present: user.participant_id.present?) |
tfink419
left a comment
There was a problem hiding this comment.
Still looks good. I have no preference for the log parameter names
Keep your PR as a Draft until it's ready for Platform review. A PR is ready for Platform review when it has a teammate approval and tests, linting, and settings checks pass CI. See these tips on how to avoid common delays in getting your PR merged.
Summary
power_of_attorneyPundit policy to log the following user information when access is denied:403errors in the Representative Status widget.power_of_attorneyPundit policy modified in this PR.Related issue(s)
Testing done
vets-websiteand tested the Representative Status widget and new logging.Screenshots
N/A
What areas of the site does it impact?
This PR impacts the
power_of_attorneyPundit policy, which is currently only used in therepresentation_managementmodule.Acceptance criteria
Requested Feedback
No specific feedback requested for this PR.