refactor: add decorator to log audit #885

jamesgetx · 2021-04-27T03:54:03Z

变更点(Changes)
提供装饰器的方案，以支持操作审计日志记录

piglei · 2021-04-27T07:23:50Z

bcs-app/backend/activity_log/audit/auditors.py

+        self.audit_context = audit_context
+
+    def log_succeed(self):
+        activity_status = 'succeed'


Nit：感觉可以不定义这个变量，直接使用 'succeed'

有一个 ActivityStatus 是常量但是使用的dict，是否考虑下改成枚举类？也可以在这里使用

bk-bcs-saas/bcs-app/backend/activity_log/constants.py

Line 32 in 2a65ad9

ActivityStatus = dict(

有一个 ActivityStatus 是常量但是使用的dict，是否考虑下改成枚举类？也可以在这里使用

bk-bcs-saas/bcs-app/backend/activity_log/constants.py

Line 32 in 2a65ad9

ActivityStatus = dict(

ActivityStatus中有4中状态，感觉其中的succeed和failed两种状态就可以。考虑先保持常量，不复用ActivityStatus

piglei · 2021-04-27T07:27:59Z

bcs-app/backend/activity_log/audit/context.py

+            if v:
+                setattr(self, f.name, v)
+
+    def update_fields(self, **kwargs):


Nit: 这俩的区别是不是一个合并对象，一个合并字典？感觉名字改成 merge / merge_dict 似乎更精确一点。

piglei · 2021-04-27T07:28:20Z

bcs-app/backend/activity_log/audit/decorator.py

@@ -0,0 +1,102 @@
+# -*- coding: utf-8 -*-


Nit：感觉复数的 decorators 更好一点。

piglei · 2021-04-27T07:30:31Z

bcs-app/backend/activity_log/audit/decorator.py

+
+def log_audit_on_view(auditor_cls: Type[Auditor], activity_type: str):
+    """
+    用于 view 的操作审计装饰器


建议更详细的描述下这装饰器的功能、使用场景。

已做调整

piglei · 2021-04-27T07:32:15Z

bcs-app/backend/activity_log/audit/decorator.py

+                return resp
+            finally:
+                # 生成默认的audit_ctx
+                extra = dict(**kwargs)


下面这段逻辑，我感觉抽象出另一个函数似乎好一点，这样装饰器不会显得太复杂。

做了代码调整

piglei · 2021-04-27T07:34:13Z

bcs-app/backend/activity_log/audit/decorator.py

+
+                auditor = auditor_cls(audit_ctx)
+                if err_msg:
+                    auditor.log_failed(err_msg)


直接记录所有异常信息的做法，总觉得不太妥当，很容易把敏感的错误信息暴露给用户。如果只是把一些特定异常转成字符串存储（比如 ValidatonError 之流），其他就不记录或者说未知错误，感觉更妥当。

这里的err_msg 捕获的是view中raise 出来的异常信息，可以在view中做这个约定

但是 view 也没法确定自己会抛出什么吧？异常的问题主要就是不可预知，假如网络出问题了，某个底层模块抛了一个没捕获的 RequestError，里面带了内网地址，不就是会被记录上吗？

通过以下方式解决

# 记录原始错误信息的异常列表 err_msg_exceptions = (APIError, ValidationError)

piglei · 2021-04-27T07:38:00Z

bcs-app/backend/activity_log/audit/decorator.py

+                audit_ctx.activity_type = activity_type
+
+                auditor = auditor_cls(audit_ctx)
+                if err_msg:


看到这个判断重复出现后，我在想，是不是目前来看，只提供一个方法就够了？有 err_msg 就是失败，没有就是成功

重新做了优化

piglei · 2021-04-27T07:39:58Z

bcs-app/backend/tests/activity_log/audit/test_decorator.py

+        assert activity_log.activity_status == 'succeed'
+        assert activity_log.description == 'list templatesets succeed'
+
+        request_post = factory.post('/', data={'version': '1.6.0'})


考虑拆分为两个独立的测试方法。

ifooth · 2021-04-27T08:02:27Z

bcs-app/backend/activity_log/audit/decorator.py

+                # 生成默认的audit_ctx
+                extra = dict(**kwargs)
+                if hasattr(request, 'data'):
+                    extra.update(request.data)


几个问题：

requests.data可能不是json，用update可能异常（直接加一个request_data key?), data body可能比较大，应该要限制下长度

资源类型，名称，ID，操作类型这些是如何写入的

如果操作db异常，是否会影响到API返回

这里其实是提供了默认的extra的生成。我考虑优化下

资源类型用装饰器时可以写入。如果有名称和ID的需求，需要在view中设置，如
request.audit_ctx.update_fields(resource_id=XX, resource='nginx')

如果操作db异常，考虑到db是关键服务。如果忽略，可能要通过日志的方式记录，或者发出告警。如果是这样，那主动报错，会不会更好些?

ifooth · 2021-04-27T08:08:48Z

bcs-app/backend/activity_log/audit/auditors.py

+        UserActivityLog.objects.create(**asdict(self.audit_context))
+
+
+class TemplatesetsAuditor(Auditor):


这里是审计类型吗？如果只是一个名称，直接在装饰器使用choices的变量是否会更好

是每种资源类型，有一个初始化？这样除了resource_type, 其它应该一样

目前用于指定resource_type，也考虑子类可能有自己的逻辑。
如果不定义子类，现阶段可以直接传Auditor，再主动设置audit_ctx.resource_type

ifooth · 2021-04-27T08:22:51Z

bcs-app/backend/activity_log/audit/decorator.py

+from .context import AuditContext
+
+
+def log_audit_on_view(auditor_cls: Type[Auditor], activity_type: str):


还有个场景需要支持下

slz.is_valid(raise_exception=True) 这个在很多view之前会检测，这类应该需要忽略，所以加个参数，忽略掉特定的异常（默认ValidateError应该要忽略的)？

加上忽略异常的逻辑，会不会太复杂了。比如参数异常，但是用户确实点了资源下发，只是没有到集群，这种是否也可能要审计下来，一个是方便他回溯错误，另一个是他有操作行为。不过可以考虑下看

可以忽略掉某些异常

narasux · 2021-04-27T08:50:44Z

bcs-app/backend/activity_log/models.py

@@ -38,6 +39,11 @@ class UserActivityLog(models.Model):
    description = models.TextField(help_text='描述', null=True, blank=True)
    extra = models.TextField(help_text='扩展')


extra 如果都是json格式的数据，是否考虑用JsonField，可以不用每次手动转换？

这次pr先不调整这个，因为涉及到其他地方有json.loads(extra)的逻辑

ifooth · 2021-04-27T09:04:08Z

bcs-app/backend/bcs_web/permissions.py

@@ -85,6 +87,8 @@ def has_permission(self, request, view):
        if project:
            request.project = project
            self._set_ctx_project_cluster(request, project.project_id, view.kwargs.get('cluster_id', ''))
+            # 设置操作审计 context
+            request.audit_ctx = AuditContext(user=request.user.username, project_id=project.project_id)


在装饰器处理这个逻辑

装饰器中增加了_pre_audit_ctx方法实现

dellkeji · 2021-04-27T08:40:52Z

bcs-app/backend/activity_log/audit/auditors.py

+        activity_status = 'failed'
+        self._log(activity_status, err_msg)
+
+    def _gen_default_description(self, activity_status: str, err_msg):


err_msg 添加类型

dellkeji · 2021-04-27T08:55:50Z

bcs-app/backend/activity_log/audit/auditors.py

+        UserActivityLog.objects.create(**asdict(self.audit_context))
+
+
+class TemplatesetsAuditor(Auditor):


是每种资源类型，有一个初始化？这样除了resource_type, 其它应该一样

dellkeji · 2021-04-27T09:03:27Z

bcs-app/backend/activity_log/audit/decorator.py

+from .context import AuditContext
+
+
+def log_audit_on_view(auditor_cls: Type[Auditor], activity_type: str):


可以忽略掉某些异常

narasux · 2021-04-29T02:11:50Z

bcs-app/backend/activity_log/audit/decorators.py

+            if isinstance(request.data, dict):
+                extra.update(request.data)
+            elif isinstance(request.data, str):
+                extra['body'] = request.data


如果是特殊字段用 __body__ 会不会好些，否则比如 request.data = {'body': 'xxxxx'} ，request.data = 'xxxxx' 结果是一样的

你的意思是，如果请求数据本身就有body key，会与这里的混淆对吧。body=>request_body

ifooth · 2021-05-06T03:55:01Z

bcs-app/backend/activity_log/audit/decorators.py

+        auditor_cls: Type[Auditor] = type(Auditor),
+        activity_type: str = '',
+        auto_audit: bool = True,
+        ignore_exception_classes: Optional[List[Type[Exception]]] = None,


直接ignore_exception_classes -> ignore_exceptions 感觉还顺畅点

ok，这里有类型注解，可以简写下

ifooth · 2021-05-06T03:59:06Z

bcs-app/backend/activity_log/audit/decorators.py

+                return ret
+            except Exception as e:
+                # 如果是 ignore_exception_classes 中的异常，不做审计记录
+                if self.ignore_exception_classes and type(e) in self.ignore_exception_classes:


这个会有BUG，exception是可以继承的，可能要抽象一个函数，用for循环后，使用isinstance(e, exc)这样判断下

def is_ignore_exception(self, e) -> bool: if not self.ignore_exceptions: return False for exc in self.ignore_exceptions: if isinstance(e, exc): return True return False

这里的逻辑其实是精确忽略，不考虑一并忽略子类的情况

如果考虑指定父类后一并忽略子类，可能会这样写

def _ignore_exception(self, e: Exception) -> bool: if not self.ignore_exceptions: return False try: raise e except self.ignore_exceptions: return True except: return False

直接调整为

try: ret = func(*args, **kwargs) return ret except self.ignore_exceptions: # 如果是 ignore_exceptions 中的异常，不做审计记录 self.auto_audit = False raise except Exception as e: err_msg = str(e) raise finally: if self.auto_audit: ...

piglei · 2021-05-06T09:06:49Z

bcs-app/backend/activity_log/audit/auditors.py

@@ -0,0 +1,58 @@
+# -*- coding: utf-8 -*-


要不我们直接按照新模块结构，把这个放在 bcs_web 里去？

backend.bcs_web.activity_log.audit？

本pr先不调整，等到模块重组时调整

piglei · 2021-05-06T09:13:11Z

bcs-app/backend/activity_log/audit/decorators.py

+        self.auditor_cls = auditor_cls
+        self.activity_type = activity_type
+        self.auto_audit = auto_audit
+        self.ignore_exception_classes = ignore_exception_classes


Nit：参数名可以短点，ignored_exceptions 感觉就够了

piglei · 2021-05-06T09:13:34Z

bcs-app/backend/activity_log/audit/decorators.py

+    - auditor_cls: 执行审计记录的类, 默认为 Auditor
+    - activity_type: 操作类型，默认为''。可在 audit_ctx 中覆盖
+    - auto_audit: 是否记录审计，默认为记录
+    - ignore_exception_classes: 忽略审计的异常类列表


建议使用 Sphinx 格式的参数说明

piglei · 2021-05-06T09:15:15Z

bcs-app/backend/tests/activity_log/audit/test_decorators.py

+        activity_log = UserActivityLog.objects.get(project_id=project_id, user=bk_user.username, resource_type='helm')
+        assert activity_log.activity_type == 'install'
+        assert activity_log.description == 'test install helm'
+        assert json.loads(activity_log.extra)['chart'] == 'http://example.chart.com/nginx/nginx1.12.tgz'


我认为可以直接调整一部分现有审计代码，用来测试新工具的能力，直接把这部分改动和工具一起提交。

本pr先不调整，等到模块重组时调整

dellkeji · 2021-05-06T09:14:26Z

bcs-app/backend/activity_log/audit/auditors.py

+        self.audit_context = audit_context
+
+    def log_succeed(self):
+        self._log('succeed')


成功时，是否也允许记录一些额外的description信息；
比如，页面展示的时候，可以直接展示出来，比如操作了增加操作的集群或者其它信息

def _log(self, activity_status: str, err_msg: str = ''): if not self.audit_context.description: self.audit_context.description = self._gen_default_description(activity_status, err_msg) self.audit_context.activity_status = activity_status UserActivityLog.objects.create(**asdict(self.audit_context))

可以主动设置description，这样会直接使用。另外，集群和其他信息，是不是在resource, 和resource_id中设置会更合适

dellkeji · 2021-05-06T09:16:49Z

bcs-app/backend/activity_log/audit/auditors.py

+class TemplatesetsAuditor(Auditor):
+    def __init__(self, audit_context: AuditContext):
+        super().__init__(audit_context)
+        self.audit_context.resource_type = 'templatesets'


Nit: 这里templatesets是不是单数即可

jamesgetx and others added 16 commits December 2, 2020 22:26

Merge remote-tracking branch 'upstream/master'

873ca95

Merge branch 'master' of https://github.com/jamesgetx/bk-bcs-saas

6f74f35

Merge remote-tracking branch 'upstream/master'

07c5e41

Merge branch 'master' of https://github.com/jamesgetx/bk-bcs-saas

5163dcb

Merge remote-tracking branch 'upstream/master'

2daf453

Merge remote-tracking branch 'upstream/master'

4b57cae

Merge remote-tracking branch 'upstream/master'

74a5572

Merge branch 'master' of https://github.com/jamesgetx/bk-bcs-saas

43216c0

Merge remote-tracking branch 'upstream/master'

c273ded

Merge remote-tracking branch 'upstream/master'

1ac4f7f

Merge remote-tracking branch 'upstream/master'

7d67694

Merge remote-tracking branch 'upstream/master'

ce537f9

Merge remote-tracking branch 'upstream/master'

6ab1dbe

Merge remote-tracking branch 'upstream/master'

30aa7e5

refactor(WIP): audit logic

8fdd1d7

refactor: audit logic

a7953e6

piglei reviewed Apr 27, 2021

View reviewed changes

ifooth reviewed Apr 27, 2021

View reviewed changes

narasux reviewed Apr 27, 2021

View reviewed changes

ifooth reviewed Apr 27, 2021

View reviewed changes

dellkeji reviewed Apr 27, 2021

View reviewed changes

jamesgetx and others added 2 commits April 28, 2021 00:43

refactor(WIP): audit decorators

208b88c

refactor: audit decorators

1b348be

narasux reviewed Apr 29, 2021

View reviewed changes

ifooth approved these changes May 6, 2021

View reviewed changes

piglei reviewed May 6, 2021

View reviewed changes

dellkeji reviewed May 6, 2021

View reviewed changes

jamesgetx added 2 commits May 6, 2021 20:44

refactor: audit decorators

4869669

refactor: audit decorators

d082c87

piglei approved these changes May 7, 2021

View reviewed changes

dellkeji approved these changes May 7, 2021

View reviewed changes

dellkeji merged commit 3711c21 into Tencent:master May 7, 2021

jamesgetx linked an issue May 17, 2021 that may be closed by this pull request

重构操作审计模块 #854

Closed

		UserActivityLog.objects.create(**asdict(self.audit_context))


		class TemplatesetsAuditor(Auditor):

		from .context import AuditContext


		def log_audit_on_view(auditor_cls: Type[Auditor], activity_type: str):

		@@ -38,6 +39,11 @@ class UserActivityLog(models.Model):
		description = models.TextField(help_text='描述', null=True, blank=True)
		extra = models.TextField(help_text='扩展')

refactor: add decorator to log audit #885

refactor: add decorator to log audit #885

Conversation

jamesgetx commented Apr 27, 2021

Choose a reason for hiding this comment

narasux Apr 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piglei May 6, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesgetx Apr 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesgetx Apr 28, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narasux Apr 29, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesgetx May 6, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narasux Apr 27, 2021 •

edited

piglei May 6, 2021 •

edited

jamesgetx Apr 27, 2021 •

edited

jamesgetx Apr 28, 2021 •

edited

narasux Apr 29, 2021 •

edited

jamesgetx May 6, 2021 •

edited