[CDAP-21105] Collecting Audit Log Meta Data independently at ACH's readChannel's finally and AuditLogSetterHook. And Create AuditLogRequest in write/close call. #15790
Conversation
This file contains 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
sahusanket
force-pushed
the
CDAP-21105_fix_addArtifact_auditlog
branch
6 times, most recently
from
c694c25 to
34bdaa7
Compare
tivv
reviewed
Jan 8, 2025
| /** | ||
| * Sets | ||
| * Sets audit log metadata to {@link SecurityRequestContext} |
| endTime | ||
| ) | ||
| ); | ||
| SecurityRequestContext.setAuditMetaDataInMap(AuditLogRequest.PropKey.OP_RESP_CODE, status.code()); |
There was a problem hiding this comment.
Could you please create a builder class for AutditLogRequest and use it here? It would be much better than using a Map. You can have different methods set different fields in the builder and later you just call build.
sahusanket
force-pushed
the
CDAP-21105_fix_addArtifact_auditlog
branch
from
842a8a3 to
2628ce9
Compare
tivv
approved these changes
Jan 8, 2025
| auditLogWriter.publish((AuditLogRequest) auditLogRequestObj); | ||
| ctx.channel().attr(AttributeKey.valueOf(AUDIT_LOG_REQ_ATTR_NAME)).set(null); | ||
| AuditLogRequest auditLogRequest = getAuditLogRequest(ctx); | ||
| if (auditLoggingEnabled && auditLogRequest != null ) { |
There was a problem hiding this comment.
Could you skip getAuditLogRequest work if it's not enabled?
| @@ -118,31 121,35 @@ public void testWriteWithAuditLogging() throws Exception { | |||
| public void testCloseWithAuditLogging() throws Exception { | |||
There was a problem hiding this comment.
Could you please add a test with other order of the calls, so that we test the use ca that was failing?
There was a problem hiding this comment.
Added for both cases.
sahusanket
force-pushed
the
CDAP-21105_fix_addArtifact_auditlog
branch
from
2628ce9 to
2d8d092
Compare
|
…adChannel's finally and AuditLogSetterHook. And Create AuditLogRequest in write/close call.
sahusanket
force-pushed
the
CDAP-21105_fix_addArtifact_auditlog
branch
from
2d8d092 to
2044739
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 Existing flow / Expectation :
ABCHttpHandler(gets AuditLogs in SecurityRequestContext[SRC] )->
AuditLogSetterHook(populates Metadata gets AuditLogs from SRC and creates AuditLogReq and sets in SRC)-->
AuthChannelHandler#readChannel's Finally(Sets the AuditLogRequest in Attribute of channel)--->
AuthChannelHandler#write(gets the AuditLogRequest from Attribute of channel and publishes)Problem incase of ArtifactHttpHandler#addArtifact:
The
AuditLogSetterHookis called afterAuthChannelHandler#readChannel's FinallyArtifactHttpHandler(gets AuditLogs in SecurityRequestContext[SRC] )->
AuthChannelHandler#readChannel's Finally(Sets the AuditLogRequest in Attribute of channelBUT THIS IS NULL)-->
AuditLogSetterHook(populates Metadata gets AuditLogs from SRC and creates AuditLogReq and sets in SRC)--->
AuthChannelHandler#write(gets the AuditLogRequest from Attribute of channel and publishes)The Fix :
Independently Collect info in
AuditLogSetterHookandAuthChannelHandler#readChannel's Finallyand Later construct theAuditLogRequestby collecting info stored by above methods.Testing :