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

Audit: track user events #8379

Merged
merged 9 commits into from
Aug 4, 2021
Merged

Audit: track user events #8379

merged 9 commits into from
Aug 4, 2021

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Jul 28, 2021

This is to allow us to offer users some security logs around log in users and page views.

I realized that some events can be attached to a user, and others could also be attached to a project (so we are able to filter events per project, like page views). I'm not sure if we should keep these after the attached project is deleted, maybe we do?

Another thing is that creating these events on doc serving will trigger a lot of other resources and becomes quite noisy
Screenshot 2021-07-28 at 18-10-37 Select audit log to change Django site admin

So, I think it makes sense to attach this event to the analytics endpoint, but a downside is that someone can skip that easily.

This is missing tests, but wanted to confirm those points before writing them

This is another screenshot using another browser

Screenshot 2021-07-28 at 18-10-48 Select audit log to change Django site admin

@stsewd stsewd requested a review from ericholscher July 28, 2021 23:25
@ericholscher
Copy link
Member

@stsewd I think we want it attached to proxito still for auditing, but we don't want to record media queries. I think we can filter by HTML & PDF, which should be enough. I'm guessing we already have this logic somewhere in the codebase I imagine, but *.html might do the trick for the basic version. Eventually we could make the file extensions configurable perhaps.

@stsewd
Copy link
Member Author

stsewd commented Jul 29, 2021

@ericholscher that did the trick! What about my other question, should we delete the logs attached to a project (page views for now) after a project is deleted? or keep them the same way when a user is deleted?

@ericholscher
Copy link
Member

I think we want to keep everything if possible. So probably a similar approach to the user. For auditing we definitely don't want people to be able to cover their tracks by deleting something they have access to.

@stsewd
Copy link
Member Author

stsewd commented Jul 29, 2021

ok, I think another step would be to have a task to clean those after some time, pageviews will generate a lot of records.

@ericholscher
Copy link
Member

@stsewd yea -- we're only going to turn this on for specific paid plans, not everyone. Part of what they'll be paying for is storage. If it becomes super large, we could always record these in a secondary DB, but we're a long way off from that :)

@stsewd stsewd force-pushed the audit-log branch 5 times, most recently from aaba51b to 1cb3c0f Compare July 29, 2021 18: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 👍 Excited to get the login/logout audit log, and then I think we have the basics ready to ship and test out, along with billing integration on .com

readthedocs/proxito/views/mixins.py Outdated Show resolved Hide resolved
readthedocs/proxito/views/mixins.py Outdated Show resolved Hide resolved
from readthedocs.acl.constants import BACKEND_REQUEST_KEY


def get_auth_backend(request):
Copy link
Member

Choose a reason for hiding this comment

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

I feel like we already have this code on .com -- this is porting it right?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I'm porting this piece from .com, so we have all code here.

readthedocs/proxito/views/mixins.py Outdated Show resolved Hide resolved
@stsewd stsewd marked this pull request as ready for review July 29, 2021 23:30
@stsewd stsewd requested a review from ericholscher July 29, 2021 23:30
@stsewd
Copy link
Member Author

stsewd commented Jul 29, 2021

This should be ready for review now, I'm only missing writing some tests on .com.

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. Curious what our plan for rolling this out is. I'd like to figure out a way to turn it on for one of our projects first, so maybe we can do that on .com. Currently there's no way to enable it on .org -- should we add a feature flag or something simple to enable it for testing?

@stsewd
Copy link
Member Author

stsewd commented Aug 2, 2021

should we add a feature flag or something simple to enable it for testing?

I was thinking that maybe we can find a way to have a mapping of some features flags to plan features, so we don't have to change our code in .org/.com or leave it as a method that only returns true/false every time we have to do a check like this, but only check for project.has_feature(Feature) and check for feature flags and the plans mapping internally.

But the real testing here is when using the auth backends on page views, log in and log out are enabled on both sites (we could also kill it on .org if we consider)

@ericholscher
Copy link
Member

ericholscher commented Aug 4, 2021

Sounds good. I think we can move forward with testing this on .com for now 👍

@stsewd stsewd merged commit 5c51074 into master Aug 4, 2021
@stsewd stsewd deleted the audit-log branch August 4, 2021 23:00
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