Audit log: log invitations #9607

stsewd · 2022-09-16T00:33:03Z

Had to do some refactoring now that we are introducing more actions to logs, there are:

Add a json field (data) to the auditlog model, this is so we can store additional information about the log entry
(useful to store information about the invitation)
Use an enum for the action choices (https://docs.djangoproject.com/en/4.1/ref/models/fields/#enumeration-types), they are easier to manage without duplicating things.
Change the tests from organizations/test_forms to test things at the view level, this is to avoid mocking more things from the request object.
Changed the tests from security logs to test the expected queryset instead of just the count, so we don't need to re-calculate everything every time we add a new thing.
Had to change how we display log entries, since we now have more actions and it gets complicated having one paragraph for each case (action / with/out user / with/out project, etc). This was suggested by Anthony on slack.

Screenshots

Closes #9456.

📚 Documentation previews 📚

User's documentation (docs): https://docs--9607.org.readthedocs.build/en/9607/

Developer's documentation (dev): https://dev--9607.org.readthedocs.build/en/9607/

We are consuming more things from the request object, instead of mocking more attributes, let's just test at the view level.

stsewd · 2022-09-20T18:35:32Z

readthedocs/audit/models.py

+    PAGEVIEW = Actions.PAGEVIEW
+    DOWNLOAD = Actions.DOWNLOAD
+    AUTHN = Actions.AUTHN
+    AUTHN_FAILURE = Actions.AUTHN_FAILURE
+    LOGOUT = Actions.LOGOUT
+    INVITATION_SENT = Actions.INVITATION_SENT
+    INVITATION_REVOKED = Actions.INVITATION_REVOKED
+    INVITATION_ACCEPTED = Actions.INVITATION_ACCEPTED
+    INVITATION_DECLINED = Actions.INVITATION_DECLINED


Set these here to avoid having to refactor every place where we use them, and it's also easier to write AuditLog.PAGEVIEW than AuditLog.Actions.PAGEVIEW. We could also move this outside the model, so we can just write Actions.PAGEVIEW, but that will require changing every usage too, so.

readthedocs/audit/models.py

stsewd · 2022-09-20T18:38:42Z

readthedocs/audit/templates/audit/list_logs.html


        <span class="quiet right" title="{{ log.created|date:"DATETIME_FORMAT" }}">
          {% blocktrans trimmed with log.created|timesince as date %}
            {{ date }} ago
          {% endblocktrans %}
        </span>
+
+        {# Data about the log entry #}


GitHub displays logs as

They show the IP and location

This matches what I was describing the other day. The main translated string is "Country changed from your previous session", without introducing variables, and metadata is listed below.

readthedocs/invitations/models.py

agjohnson

I didn't do a thorough job looking at Python, so would be good to get secondary review there. I'll have a lot of updates for this view when we get to reworking this UI, but for now, my inclination is probably to keep this UI very simple, and add more complex UI with new templates.

Addressing what we're storing and displaying is a great focus for this stage. We can come back to this UI with more complex additions, like filtering and search.

readthedocs/audit/models.py

readthedocs/audit/templates/audit/list_logs.html

agjohnson · 2022-09-21T19:09:24Z

readthedocs/audit/templates/audit/list_logs.html


        <span class="quiet right" title="{{ log.created|date:"DATETIME_FORMAT" }}">
          {% blocktrans trimmed with log.created|timesince as date %}
            {{ date }} ago
          {% endblocktrans %}
        </span>
+
+        {# Data about the log entry #}


This matches what I was describing the other day. The main translated string is "Country changed from your previous session", without introducing variables, and metadata is listed below.

readthedocs/audit/templates/audit/list_logs.html

humitos

I think this PR is fine. However, I'd invite another person to do a code review as well. To me, there are some new magic added in this PR that we have not been using and it's changing some of the patterns we have for the same things. I usually prefer consistency in these cases.

These new decisions made me spent more time than I would normally require to review this code. Changes aren't huge or complex, but I had to re-read multiple times them to understand the new approaches used for the same things we have solved in a different --and no that magic-- way.

readthedocs/audit/filters.py

readthedocs/audit/models.py

humitos · 2022-10-06T16:29:06Z

readthedocs/invitations/models.py

+        AuditLog.objects.new(
+            action=action,
+            request=request,
+            data=InvitationSerializer(self).data,


This called my attention. I was wondering why we had created the readthedocs/invitations/serializers.py file if we are not exposing invitations via a REST API.

It seems we are using the DRF serializers for a different purpose here and I'm not convinced this is the right approach.

readthedocs/audit/serializers.py

agjohnson · 2022-10-25T18:42:12Z

@stsewd We're doing some sprint clean up, is there anything required on this PR to move forward?

stsewd · 2022-10-26T22:08:11Z

@agjohnson there is a disagreement about using django's enum types for constants/options #9607 (comment).

humitos · 2022-11-16T09:46:16Z

readthedocs/audit/serializers.py

+
+    to_user = UserSerializer()
+    from_user = UserSerializer()
+    object = serializers.SerializerMethodField()


Isnt' it a problem to override the object reserved word?

only when used as variable name, we can change it to obj if we want

humitos · 2022-11-16T09:50:41Z

readthedocs/audit/serializers.py

+    def get_object(self, obj):
+        serializers = {
+            "organization": OrganizationSerializer,
+            "project": ProjectSerializer,
+            "team": TeamSerializer,


It would be good to have a comment here saying that the value of object will depend on the object type and a different serializer will be used.

stsewd added 5 commits September 20, 2022 13:03

Audit log: log invitations

9aae127

Fix tests

6c1e940

We are consuming more things from the request object, instead of mocking more attributes, let's just test at the view level.

Use a enum for action options

d8df13a

Refactor

14d3c1a

Tests

e041d68

stsewd force-pushed the log-invitations branch 3 times, most recently from a2f42a8 to 5e8bc7d Compare September 20, 2022 18:22

Fix tests

dcffedb

stsewd force-pushed the log-invitations branch from 5e8bc7d to dcffedb Compare September 20, 2022 18:32

stsewd marked this pull request as ready for review September 20, 2022 18:33

stsewd requested review from a team as code owners September 20, 2022 18:33

stsewd requested review from agjohnson and humitos September 20, 2022 18:33

auto-assign bot assigned stsewd Sep 20, 2022

stsewd commented Sep 20, 2022

View reviewed changes

agjohnson reviewed Sep 21, 2022

View reviewed changes

stsewd added 2 commits September 22, 2022 10:32

Updates from review

4f77693

Use serializers

df9fccb

humitos reviewed Oct 6, 2022

View reviewed changes

stsewd mentioned this pull request Nov 8, 2022

Modernizing our Django code base #9716

Open

stsewd added 3 commits November 14, 2022 17:21

Merge branch 'main' into log-invitations

beab4dd

Back to constants

5ded037

Updates from review

230f2af

stsewd requested a review from humitos November 14, 2022 23:20

stsewd added 2 commits November 14, 2022 18:28

Silly mistake

1c7813b

Linter

e98cf6e

humitos approved these changes Nov 16, 2022

View reviewed changes

stsewd added 3 commits November 23, 2022 13:21

Linter and updates from review

0e82805

Merge branch 'main' into log-invitations

21deab0

Don't show declined invitations in org logs

f878051

stsewd merged commit 6e42d46 into main Nov 23, 2022

stsewd deleted the log-invitations branch November 23, 2022 18:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Audit log: log invitations #9607

Audit log: log invitations #9607

stsewd commented Sep 16, 2022 •

edited

Loading

stsewd Sep 20, 2022

stsewd Sep 20, 2022

agjohnson Sep 21, 2022

agjohnson left a comment

agjohnson Sep 21, 2022

humitos left a comment

humitos Oct 6, 2022

agjohnson commented Oct 25, 2022

stsewd commented Oct 26, 2022 •

edited

Loading

humitos Nov 16, 2022

stsewd Nov 17, 2022

humitos Nov 16, 2022

Audit log: log invitations #9607

Audit log: log invitations #9607

Conversation

stsewd commented Sep 16, 2022 • edited Loading

Screenshots

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agjohnson commented Oct 25, 2022

stsewd commented Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsewd commented Sep 16, 2022 •

edited

Loading

stsewd commented Oct 26, 2022 •

edited

Loading