feat: Enhanced Audit System with Special Operations Tracking#782
feat: Enhanced Audit System with Special Operations Tracking#782Katerina-Charakhovich wants to merge 2 commits intodevelopmentfrom
Conversation
Dependency ReviewThe following issues were found:
License Issuessettings.gradle
OpenSSF Scorecard
Scanned Files
|
Katerina-Charakhovich
changed the title
…al-Operations-Tracking
Dependency ReviewThe following issues were found:
License Issuessettings.gradle
OpenSSF Scorecard
Scanned Files
|
| AdminSettings, | ||
| ImportConfig, | ||
| ExportConfig, | ||
| ToolResource, |
There was a problem hiding this comment.
May be ToolSetResource
| .build(); | ||
| } | ||
|
|
||
| @Transactional |
There was a problem hiding this comment.
Why is it removed?
| resourceId = StringUtils.isNotBlank(resourceId) ? resourceId : entity.getActivityId().toString(); | ||
| entity.setResourceId(resourceId); | ||
| auditActivityJpaRepository.save(entity); | ||
| entityManager.flush(); |
There was a problem hiding this comment.
Why is that needed?
| private final EntityManager entityManager; | ||
|
|
||
| @Transactional | ||
| private UUID startParentOperation(ActivityType activityType, |
There was a problem hiding this comment.
Let's specify public method before private ones
| } | ||
|
|
||
| @Transactional | ||
| public void logAuditOperation(ActivityType activityType, |
There was a problem hiding this comment.
should it be public?
| } | ||
|
|
||
| @Transactional | ||
| public void importConfigTransactional(Config config, ConfigImportOptions importOptions) { |
There was a problem hiding this comment.
should it be public?
| @Transactional | ||
| public UUID logImportOperation(String scope, ConfigImportOptions importOptions) { | ||
| var metaJson = AuditMetaBuilder.with(objectMapper) | ||
| .put("scope", scope) |
There was a problem hiding this comment.
Considering that possible values are "core" or "admin" I think it should be "format"/"type" rather than "scope"
| String resourceId, | ||
| String operationMetadataJson) { | ||
| var entity = createAuditEntity(activityType, resourceType, resourceId, operationMetadataJson); | ||
| resourceId = StringUtils.isNotBlank(resourceId) ? resourceId : entity.getActivityId().toString(); |
There was a problem hiding this comment.
Can resourceId be not null in certain cases?
| private final ObjectMapper objectMapper; | ||
| private final EntityManager entityManager; | ||
|
|
||
| @Transactional |
There was a problem hiding this comment.
@Transactional on private method doesn't work
| } | ||
|
|
||
| @Transactional | ||
| public UUID logImportOperation(String scope, ConfigImportOptions importOptions) { |
There was a problem hiding this comment.
I think logging of operations like import should not start transaction on their own if transaction doesn't exist, they should just support existing transaction or throw exception if transaction doesn't exist
| public StreamingResponseBody exportConfig(ExportRequest request) { | ||
| ExportFormat exportFormat = request.getExportFormat(); | ||
| ExportConfig config = configExporter.getConfig(request); | ||
| auditActivityLogService.logConfigExport(request); |
There was a problem hiding this comment.
Since transaction is read only, I think new activity record will not be saved in DB
| Export, | ||
| ExportRawConfig, | ||
| PublicationApprove, | ||
| PublicationUpdate, |
There was a problem hiding this comment.
Can we use existing Create, Update, Delete actions?
3 participants
Applicable issues
Description of changes
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.