Capture when user was last active

[denamwangi]

Not sure how frequently we want to update this field- currently I have this set to hourly and depending on what ops thinks is reasonable without overloading we can get more precise and narrow the window.

[ghost]

	1 Warning
⚠️	PR includes migrations

Migration Checklist

• new columns need to be nullable (unless table is new)
• migration with any new index needs to be done concurrently
• data migrations should not be done inside a transaction
• before merging, check to make sure there aren't conflicting migration ids

Generated by 🚫 danger

[dcramer]

we should use request.user.update(last_active=now) to avoid rewriting the entire row (which is what Django does with save())

[dcramer]

looks like all of this code was copy/pasta and is unused

[ehfeng]

This was to limit allowed_paths, to avoid event or commit ingestion endpoints, which aren't reflective of an active user. However...that might actually just be caught by the user.is_authenticated, given that there's no user in those requests.

Try testing those out locally, see what happened.

[dcramer]

you dont need it at all

this wont work with the API though, you need to do that through their authentication frameworks

[denamwangi]

Ah good point on update vs save- made that change. Took out the allowed paths logic (Eric - double checked events aren't affecting as dcramer said) and added in TokenAuth to pull out the user from the API side. Let me know if there's a different way we handle this.

[dcramer]

you actually should be doing this as part of the existing auth paths rather than in the middleware -- otherwise this is causing duplicate auth logic and it won't work correctly with various API setups

you can do this in api/authentication in authenticate_credentials as its at least closer to limiting the scope. alternatively you could figure out where to stuff in it in api/base.py::Endpoint

[denamwangi]

Hey dcramer- take a look at the screencast of a replay with the auth middleware. It looks like get_user() in auth.py is handling the session auth. Thoughts?

active_user_authMW.zip

[dcramer]

Ok so:

1. if request.user is an authenticated user when they hit an API request thats fired via the javascript app (e.g. loading the stream), then we dont need to do anything special here
2. if request.user is filled in later by SessionAuthentication in the API, we should handle it there
3. in either situation, we dont care about TokenAuth (I forgot what it meant on original review), and we wouldn't want to approach capturing the api auth here even if we did

(ehfeng: adding numbers instead of bullets)

[ehfeng]

I don't understand the need for case 2—the auth.py middleware fires before the SessionAuthentication and if we're not looking to handle TokenAuth, I don't see what requests would not have a user in case 1, but have a user for case 2.

[dcramer]

given that request.user is always set in session API calls, just remove this and then we're good to go

[denamwangi]

Will do! Thanks

…

On Wed, Jun 28, 2017 at 4:48 PM, David Cramer ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/sentry/middleware/user.py <#5630 (comment)>: > @@ -0,0 +1,28 @@ +from __future__ import absolute_import + +from datetime import timedelta + +from django.utils import timezone +from sentry.api.authentication import TokenAuthentication + + +class UserActiveMiddleware(object): + def process_view(self, request, view_func, view_args, view_kwargs): + try: + auth = TokenAuthentication() given that request.user is always set in session API calls, just remove this and then we're good to go — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5630 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/APef3SANVS2XKfL68M0Zqe4_jmFJa3ncks5sIuY1gaJpZM4OFx-d> .

[denamwangi]

Ok- took out all the TokenAuth logic. I had to add in some disallowed paths for a couple of paths we don't need that were breaking tests on static pages. On Wed, Jun 28, 2017 at 4:49 PM, Dena Mwangi <notifications@github.com> wrote:

…

Will do! Thanks On Wed, Jun 28, 2017 at 4:48 PM, David Cramer ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In src/sentry/middleware/user.py > <#5630 (comment)>: > > > @@ -0,0 +1,28 @@ > +from __future__ import absolute_import > + > +from datetime import timedelta > + > +from django.utils import timezone > +from sentry.api.authentication import TokenAuthentication > + > + > +class UserActiveMiddleware(object): > + def process_view(self, request, view_func, view_args, view_kwargs): > + try: > + auth = TokenAuthentication() > > given that request.user is always set in session API calls, just remove > this and then we're good to go > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#5630# pullrequestreview-46997589>, > or mute the thread > <https://github.com/notifications/unsubscribe- auth/APef3SANVS2XKfL68M0Zqe4_jmFJa3ncks5sIuY1gaJpZM4OFx-d> > . > — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#5630 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/APef3RJTbsauUegKvolS9FTB3KH9JuBmks5sIuZ0gaJpZM4OFx-d> .

[dcramer]

you could also do if hasattr(request, 'user') as I assume thats the issue you hit w/ the static paths