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

Organizations: show audit logs #8588

Merged
merged 12 commits into from
Nov 8, 2021
Merged

Organizations: show audit logs #8588

merged 12 commits into from
Nov 8, 2021

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Oct 14, 2021

Similar to the user security logs, but from all the organization and according to their plan.
I was able to re-use the listing from the user security log :D.

Didn't want to overload the UI with all the information from each record (but you can get everything with the download button), still you can see some information when hovering over some places.

Screenshot 2021-10-18 at 18-26-58 Security Log - Read the Docs for Business
Screenshot 2021-10-18 at 18-26-43 Security Log - Read the Docs for Business
Screenshot 2021-10-18 at 18-26-31 Security Log - Read the Docs for Business

Base automatically changed from move-orgs-vies to master October 18, 2021 19:55
@stsewd stsewd force-pushed the expose-org-audit-log branch 3 times, most recently from 0233b15 to 9ad62c0 Compare October 18, 2021 21:20
@@ -21,4 +21,18 @@ class UserSecurityLogFilter(FilterSet):

class Meta:
model = AuditLog
fields = ['ip', 'project', 'action']
fields = []
Copy link
Member Author

Choose a reason for hiding this comment

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

this wasn't needed, as these values are defined as attributes, but you still need to declare fields

@stsewd stsewd marked this pull request as ready for review October 18, 2021 23:34
@stsewd stsewd requested a review from a team October 18, 2021 23:34
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This looks like a good start, but I think we definitely want the ability to filter by dates by the user. It doesn't need to be in the UI yet, but at least GET args.

readthedocs/audit/templates/audit/list_logs.html Outdated Show resolved Hide resolved
readthedocs/audit/filters.py Show resolved Hide resolved
{% block edit_content %}

{% if not enabled %}
{% include 'projects/includes/feature_disabled.html' with organization=organization %}
Copy link
Member

Choose a reason for hiding this comment

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

Should we be passing in something like plan=Pro in order to clarify what plan is required?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we could pass the feature type and check from the other template what plan is required?

Copy link
Member

Choose a reason for hiding this comment

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

Yea, not a huge priority though. 👍

readthedocs/organizations/views/private.py Show resolved Hide resolved
readthedocs/organizations/views/private.py Outdated Show resolved Hide resolved
('IP', 'ip'),
('Browser', 'browser'),
]
data = self._get_queryset().values_list(*[value for _, value in values])
Copy link
Member

Choose a reason for hiding this comment

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

This could potentially be huge for a user with pageviews. We are likely going to need something async that emails this to users, or uploads it to storage, or something. We can worry about that later though.

readthedocs/organizations/views/private.py Outdated Show resolved Hide resolved
readthedocs/organizations/views/private.py Show resolved Hide resolved
@stsewd stsewd requested a review from a team as a code owner October 21, 2021 20:59
@stsewd
Copy link
Member Author

stsewd commented Oct 21, 2021

@ericholscher I have added some docs and filtering by dates (date_before/date_after query args).
Some questions:

  • Should we document the available query args and mention they can be combined? currently, they are implicit
  • Should the download button respect the current filters? Right now it always downloads all records, without using the filters.
    I saw that github does respects the filters when you click on "export" https://github.com/settings/security-log.

@ericholscher
Copy link
Member

@ericholscher I have added some docs and filtering by dates (date_before/date_after query args). Some questions:

Awesome! I looked over it, and it seems good to me.

  • Should we document the available query args and mention they can be combined? currently, they are implicit

I think it's fine for now that we don't document them. I do think we should probably make them work with a GET arg though.

  • Should the download button respect the current filters? Right now it always downloads all records, without using the filters.

I think so. I think otherwise some users won't be able to download anything because the view will time out, and it will be confusing to filter the data and not have the download respect that.

I saw that github does respects the filters when you click on "export" https://github.com/settings/security-log.

Yea -- I think updating the export/download link is probably the easiest way to handle this.

docs/security-log.rst Outdated Show resolved Hide resolved
User security log
-----------------

We store user security logs from the last 90 days, and track the following events:
Copy link
Member

Choose a reason for hiding this comment

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

Does this vary by plan on .com? If so, we should probably put this in a small table comparing each site. I dislike having the tabs where you have to click to see the Commercial one, so a small table seems best?

Copy link
Member Author

Choose a reason for hiding this comment

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

User securtiy logs are 90 days for both, we could respect the max period set by the organizations they belong on .com, but I have some questions if that happens, should we show those type of logs to org owners as well? #8620 (comment)

docs/security-log.rst Outdated Show resolved Hide resolved
docs/security-log.rst Outdated Show resolved Hide resolved
docs/security-log.rst Outdated Show resolved Hide resolved
docs/security-log.rst Outdated Show resolved Hide resolved
docs/security-log.rst Outdated Show resolved Hide resolved
days_ago = timezone.now() - timezone.timedelta(days=retention_limit)
return creation_date
start_date = timezone.now().date() - timezone.timedelta(days=retention_limit)
# The max we can go back is to the creation of the organization.
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -189,9 +187,17 @@ def get(self, request, *args, **kwargs):
return self._get_csv_data()
return super().get(request, *args, **kwargs)

def _get_start_date(self):
Copy link
Member

Choose a reason for hiding this comment

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

Seems like we want to get the request.GET.start_date here?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's already handled by the filter, the filter acts on top of this query.

stsewd and others added 2 commits November 1, 2021 12:05
Co-authored-by: Eric Holscher <25510+ericholscher@users.noreply.github.com>
@stsewd
Copy link
Member Author

stsewd commented Nov 1, 2021

Download button now uses the current filters. But had to use a link instead of a form, as the form removes all query args from the forms' action attribute. To make the link look like a button, used the button class, but looks like that class doesn't exist on .com, so we need to figure out that first before merging...

Screenshot 2021-11-01 at 14-15-15 Security Log - Read the Docs for Business

@stsewd
Copy link
Member Author

stsewd commented Nov 1, 2021

Fixed :D
It overlaps a little, but that looks like that happens in other elements on .com.

Screenshot 2021-11-01 at 16-47-32 Organization Security Log - Read the Docs for Business

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

Changes look good 👍

Let's get this shipped and deployed.

@stsewd stsewd enabled auto-merge (squash) November 8, 2021 22:25
@stsewd stsewd merged commit 4e87dfc into master Nov 8, 2021
@stsewd stsewd deleted the expose-org-audit-log branch November 8, 2021 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants