Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DSIP-26][Audit log] Audit log improvement design #15554

Merged
merged 87 commits into from
Apr 15, 2024

Conversation

qingwli
Copy link
Member

@qingwli qingwli commented Feb 2, 2024

qingwli and others added 15 commits January 15, 2024 15:34
# Conflicts:
#	dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/controller/ClusterController.java
#	dolphinscheduler-dao/src/test/java/org/apache/dolphinscheduler/dao/mapper/AuditLogMapperTest.java
# Conflicts:
#	dolphinscheduler-dao/src/main/resources/sql/upgrade/3.3.0_schema/mysql/dolphinscheduler_ddl.sql
#	dolphinscheduler-dao/src/main/resources/sql/upgrade/3.3.0_schema/postgresql/dolphinscheduler_ddl.sql
@qingwli qingwli self-assigned this Feb 2, 2024
@github-actions github-actions bot added UI ui and front end related backend labels Feb 2, 2024
@qingwli qingwli added this to the 3.3.0 milestone Feb 2, 2024
@qingwli qingwli added improvement make more easy to user or prompt friendly 3.3.0 labels Feb 2, 2024
@ruanwenjun ruanwenjun changed the title [DSIP-26] Audit log Improvement [DSIP-26][Audit log] Audit log improvement design Apr 1, 2024
Comment on lines 2011 to 2013
`object_id` bigint(20) DEFAULT NULL COMMENT 'object id',
`object_name` varchar(100) DEFAULT NULL COMMENT 'object id',
`object_type` varchar(100) NOT NULL COMMENT 'object type',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`object_id` bigint(20) DEFAULT NULL COMMENT 'object id',
`object_name` varchar(100) DEFAULT NULL COMMENT 'object id',
`object_type` varchar(100) NOT NULL COMMENT 'object type',
`object_id` bigint(20) DEFAULT NULL COMMENT 'object id',
`object_name` varchar(100) DEFAULT NULL COMMENT 'object id',
`object_type` varchar(100) NOT NULL COMMENT 'object type',

Use domain_id or module_id is better than object_id?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this part, I defined object id means the object I modified, task instance it's not a module. File is not a domain, I think object is more suit for this, can cover all the stuff we record, WDYT


PROJECT("Project", null), // 1
PROCESS("Process", PROJECT),
PROCESS_INSTANCE("Process instance", PROCESS),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
PROCESS_INSTANCE("Process instance", PROCESS),
PROCESS_INSTANCE("ProcessInstance", PROCESS),

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 79 to 82
// Api don't need record log
if (operatorLog == null) {
return point.proceed();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logPointCut should make sure the method exist @OperatorLog.

return point.proceed();
}

Operation operation = method.getAnnotation(Operation.class);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find the Operation class, is this exist?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Operation(summary = "queryAuditObjectTypeList", description = "QUERY_AUDIT_OBJECT_TYPE_LIST")
    @GetMapping(value = "/audit-log-object-type")
    @ResponseStatus(HttpStatus.OK)
    @ApiException(QUERY_AUDIT_LOG_LIST_PAGING)
    public Result<List<AuditObjectTypeDto>> queryAuditObjectTypeList() {
        return Result.success(AuditObjectTypeDto.getObjectTypeDtoList());
    }

void execute(AuditMessage message);
import org.aspectj.lang.ProceedingJoinPoint;

public interface Operator {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public interface Operator {
public interface AuditOperator {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 48 to 77
long beginTime = System.currentTimeMillis();

MethodSignature signature = (MethodSignature) point.getSignature();
Map<String, Object> paramsMap = OperatorUtils.getParamsMap(point, signature);

User user = OperatorUtils.getUser(paramsMap);

if (user == null) {
log.error("user is null");
return point.proceed();
}

List<AuditLog> auditLogList = OperatorUtils.buildAuditLogList(describe, auditType, user);
setRequestParam(auditType, auditLogList, paramsMap);

Result result = (Result) point.proceed();
if (OperatorUtils.resultFail(result)) {
log.error("request fail, code {}", result.getCode());
return result;
}

setObjectIdentityFromReturnObject(auditType, result, auditLogList);

modifyAuditOperationType(auditType, paramsMap, auditLogList);
modifyAuditObjectType(auditType, paramsMap, auditLogList);

long latency = System.currentTimeMillis() - beginTime;
auditService.addAudit(auditLogList, latency);

return result;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may need to audit the case when proceed throw exception.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

todo #15788

return;
}

Long objId = checkNum(returnObjectMap.get(params[0]).toString());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Long objId = checkNum(returnObjectMap.get(params[0]).toString());
Long objId = NumberUtils.toLong(returnObjectMap.get(params[0]).toString(), -1);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -293,7 +293,7 @@ private void checkMasterExists() {

// no master
if (masterServers.isEmpty()) {
throw new ServiceException(Status.MASTER_NOT_EXISTS);
// throw new ServiceException(Status.MASTER_NOT_EXISTS);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rollback this kind of code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

auditDto.setUserName(auditLog.getUserName());
auditDto.setResourceName(auditLogMapper.queryResourceNameByType(resourceType, auditLog.getResourceId()));
auditDto.setLatency(String.format("%.2f", (double) auditLog.getLatency() / 1000));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
auditDto.setLatency(String.format("%.2f", (double) auditLog.getLatency() / 1000));
auditDto.setLatency(String.format("%.2f", (double) auditLog.getLatency() / 1000));

It's better to use ms.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@ruanwenjun
Copy link
Member

Quality Gate Passed Quality Gate passed

Issues 42 New issues 0 Accepted issues

Measures 0 Security Hotspots 62.8% Coverage on New Code 0.0% Duplication on New Code

See analysis details on SonarCloud

Please take care of the new issues.

ruanwenjun
ruanwenjun previously approved these changes Apr 14, 2024
Copy link
Member

@ruanwenjun ruanwenjun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, please move the ddl to 3.2.2

@qingwli
Copy link
Member Author

qingwli commented Apr 15, 2024

PTAL @ruanwenjun

Copy link

sonarcloud bot commented Apr 15, 2024

@ruanwenjun ruanwenjun merged commit 2263455 into apache:dev Apr 15, 2024
66 of 67 checks passed
@qingwli qingwli deleted the audit-log branch April 15, 2024 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.3.0 backend DSIP improvement make more easy to user or prompt friendly ready-to-merge UI ui and front end related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DSIP-26][Audit log] Audit log improvement design
8 participants