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

No user_id is recorded #165

Open
hasahmad opened this issue Nov 3, 2020 · 10 comments
Open

No user_id is recorded #165

hasahmad opened this issue Nov 3, 2020 · 10 comments

Comments

@hasahmad
Copy link

hasahmad commented Nov 3, 2020

I am using this with Django rest framework, using OAuth2, and it's not recording user_id
Are you only checking for users in session cookie?

@jheld
Copy link
Collaborator

jheld commented Nov 4, 2020

I think that's a fairly accurate statement. Will accept PR to change that. We do support general usage of the login_user signal (e.g. manually telling Django, and I've seen that work for JWT login via DRF hooks). If there is a way for the library to more seamlessly hook into the general system, that would be good. Note: that is only for login. Once logged in, easy audit model signals will otherwise grab the user via it's Middleware.

Curious which version of django and python? If django 3.1, async or sync views/Middleware? Easy audit only uses sync Middleware (which should be fine and we have unit tests).

@wmantly
Copy link

wmantly commented Feb 26, 2021

@hasahmad did you find a workaround? I am having the same issue?

@z-sega
Copy link

z-sega commented Mar 16, 2022

@hasahmad @wmantly
I have the same issue, did you figure it out?

@wmantly
Copy link

wmantly commented Mar 16, 2022

@hasahmad, I posted this on behalf of one of my clients. The issue was due to him not authenticating some requests. Simply fixing some calls he was making anonymously fixed this issue(and a security hole he had)

@z-sega
Copy link

z-sega commented Mar 16, 2022

@hasahmad, I posted this on behalf of one of my clients. The issue was due to him not authenticating some requests. Simply fixing some calls he was making anonymously fixed this issue(and a security hole he had)

What do you mean by that '... not authenticating some requests. Simply fixing some calls ...'? My project uses a lot of interconnected models and most (if not all) are exposed through authenticated ViewSets using the DRF's IsAuthenticated permission class.

Are you able to provide some detail about what he did specifically?

@mschoettle
Copy link
Contributor

Which DRF authentication classes are you using in your project?

@mschoettle
Copy link
Contributor

mschoettle commented Mar 17, 2022

I just tested it with TokenAuthentication and it does not record the user in the request event. I think it is due to the fact that DRF does not plug their authentication mechanisms into Django's authentication system (it works with SessionAuthentication because it uses the standard Django session). I noticed recently that when doing a request with TokenAuthentication, the user is AnonymousUser (see my comment here). Not totally sure whether this is a bug in DRF or not.

Edit: It does work for CRUD events, though.

@mschoettle
Copy link
Contributor

Turns out that there are already some solutions, unfortunately in DRF so everyone could benefit from them. One of them is DRF JWT: jpadilla/django-rest-framework-jwt#45 (comment)

Basically, DRF authenticates in the view layer. The workaround is to supply a custom middleware that does the authentication. However, it leads to authenticating twice and thus increasing the number of DB queries.

I looked at how the request is logged in this plugin (see https://github.com/soynatan/django-easy-audit/blob/master/easyaudit/signals/request_signals.py#L38). I wonder whether instead the request_finished signal could be reacted to (at that point the user should be there). It would require to fix how the user is retrieved though (see https://github.com/soynatan/django-easy-audit/blob/master/easyaudit/signals/request_signals.py#L62-L83).

What do you think @jheld?

@jheld
Copy link
Collaborator

jheld commented Apr 18, 2022

For reference, the django docs on the request_finished signal.

I'm not sure that I fully understand whether the DRF JWT snippet in the link solved the problem, but regarding this library, I'm open to accepting some changes, like:

  • pluggable application handler logic JIT in the request started
  • similarly, for the request finished handler (request finished signal handler is not currently hooked, so that will have to be added)
  • seeing if we can leverage additional authentication strategies/backends in the handler within the library itself -- can we? why aren't we? etc, and if we can & should, then making the change to do so.

@mschoettle how does the above sound, or did I miss something?

@mschoettle
Copy link
Contributor

mschoettle commented Dec 10, 2022

@jheld: Took me a while to get back to this.

I am playing around with the signals and EasyAuditMiddleware. What I found out is the following:

  • the request_started signal is triggered before the Django session middleware processes the request and sets request.user. I believe this is why easyaudit currently needs to manually check the session cookie.
  • looking at the request in the request_finished signal provides the user via request.user. This is the case for a regular request via session authentication as well as for a request authenticated via Django REST framework (I used TokenAuthentication for this)

So it seems that handling requests in request_finished would be more appropriate. At the same time it makes me wonder: Why not handle request events in the easy audit middleware directly? request signals don't receive the request requiring the workaround via thread locals. Whereas the request and response are available in the middleware.

It looks like this would solve a few of the issues reported.

What do you think?

Update: Did a quick test where I moved the request started signal handler code into the middleware.

def __call__(self, request: WSGIRequest):
    self.process_request(request)
    # gather request data
    response = self.get_response(request)
    # get user from request.user
    audit_logger.request(...)
    self.process_response(request, response)

    return response

Based on preliminary results this works. I can create a PR for this for review @jheld.

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

No branches or pull requests

5 participants