Repository navigation
Use the federated user id as the session audit log initiator when log masking is enabled - #8373
RushanNanayakkara wants to merge 1 commit into
Conversation
… masking is enabled For federated users, including organization SSO users, the session audit log initiator fell back to the masked subject, so the line could not be attributed to a user. Use the user id already set on the authenticated user instead. It is an internal identifier and does not expose the subject. The masked subject is still used when no user id is set.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ⬇️ Low Change: Bug fix Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check |  |
The description clearly explains the purpose, implementation, affected flows, fallback behavior, issue reference, and testing results. However, it omits most template sections, including Goals, User s… | Add the missing template sections. Mark non-applicable sections as N/A with a brief explanation, and provide the required security checks, automation test details, release note, documentation impact, test environment, and related PR informa… |
| Docstring Coverage | |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly describes the main change: using the federated user ID as the session audit log initiator when log masking is enabled. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Description check
-
Explanation
The description clearly explains the purpose, implementation, affected flows, fallback behavior, issue reference, and testing results. However, it omits most template sections, including Goals, User stories, Release note, Documentation, Training, Certification, Marketing, Automation tests, Security checks, Samples, Related PRs, Migrations, and Learning.
Resolution
Add the missing template sections. Mark non-applicable sections as N/A with a brief explanation, and provide the required security checks, automation test details, release note, documentation impact, test environment, and related PR information.
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
|
PR builder started |
|
PR builder completed |
jenkins-is-staging
left a comment
jenkins-is-staging
left a comment
There was a problem hiding this comment.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/39588134633
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #8373 +/- ##
=========================================
Coverage 54.90% 54.91%
Complexity 22746 22746
=========================================
Files 2277 2277
Lines 133919 133925 +6
Branches 22204 22206 +2
=========================================
+ Hits 73534 73542 +8
+ Misses 51562 51560 -2
Partials 8823 8823
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
Purpose
Related issue: wso2/product-is#28491
With
[masking_logs] enabled = true, the session audit log lines (StoreSession,UpdateSession) give a local user an unmasked user ID inInitiator, so the line can be attributed to a user. Federated users get the masked subject in bothInitiatorandAuthenticatedUser, so the line cannot be attributed to anyone. Organization SSO users (sub-organization users logging in to a root organization application with enhanced organization login disabled) take the same path, because the framework marks them as federated.What this changes
DefaultAuthenticationRequestHandler.addAuditLogs: for a federated user, when log masking is enabled,Initiatoris set to the user ID already set on theAuthenticatedUser. If no user ID is set, the masked subject is used, as before.The username lookup is still skipped for federated users, so the subject from the external IdP is never logged unmasked. The user ID used instead does not expose it:
AuthenticatedUserIDN_AUTH_USER, set byDefaultStepHandlerafter the federated stepNo user ID is looked up or stored by this change; it only reads the field when
isUserIdExists()is true.AuthenticatedUserstays masked. Behaviour with log masking disabled is unchanged.The
Loginaudit line is written by identity-data-publisher-authentication and is not changed here.Testing
Measured on the IS 7.4.0 nightly
wso2is-7.4.0-20261020.072933-234(framework7.11.236, H2, JDK 25) by driving logins end to end and readingaudit.log.DefaultAuthenticationRequestHandleris the same inv7.11.236and on master. The unmodified class compiles to the same bytecode, line and local variable tables as the shipped one, so the stock and patched runs differ only by this change.InitiatorInitiatorsubthe application receives)AuthenticatedUseris masked in every masked run. With enhanced organization login enabled, the user logs in directly in the sub-organization and no federated session line is written, so that flow is not affected.The fallback (federated user with no user ID set, masked subject kept) was exercised on IS 7.1.0 with the same code, by clearing the user ID in a debugger.
Framework module unit tests could not be run meaningfully on the test machine: it has JDK 25 only, and Mockito inline mocks fail there. With
net.bytebuddy.experimental=true, base and this change give the same result (961 run, the same 28 failures), butDefaultAuthenticationRequestHandlerTestis skipped in both because itssetUpfails in Mockito. The CI run on this pull request is the unit test result for this change.