soynatan / django-easy-audit

Yet another Django audit log app, hopefully the simplest one.
GNU General Public License v3.0
719 stars 178 forks source link

Make REMOTE_ADDR optional #263

Closed mschoettle closed 6 months ago

mschoettle commented 11 months ago

The user_logged_in signal handler fails during tests when calling client.force_login(...) because the request has no IP (REMOTE_ADDR).

def user_logged_in(sender, request, user, **kwargs):
        try:
            with transaction.atomic(using=DATABASE_ALIAS):
                login_event = audit_logger.login({
                    'login_type': LoginEvent.LOGIN,
                    'username': getattr(user, user.USERNAME_FIELD),
                    'user_id': getattr(user, 'id', None),
>                   'remote_ip': request.META[REMOTE_ADDR_HEADER]
                })
E               KeyError: 'REMOTE_ADDR'

[snip]/easyaudit/signals/auth_signals.py:21: KeyError

Would it be ok to change this to request.META.get(REMOTE_ADDR_HEADER)? remote_ip is already nullable so from the LoginEvent model perspective it is already supported. And the request signal handler already supports this:

https://github.com/soynatan/django-easy-audit/blob/bbdf834d671811043e7bab4aaeb39903ac21f982/easyaudit/signals/request_signals.py#L44

mschoettle commented 9 months ago

@jheld Let me know what you think please. Happy to create a PR for this.

The alternative would be to disable auth events to be watched in tests but it would be nice to stay as close as possible to production.

jheld commented 9 months ago

Should any extra conceptual weirdness be at play here by making this change, it's evading me at this time. Given the examples you've shown in the code where it certainly allows None, I concur with your approach. Yes, please make a PR :)