django-easy-audit icon indicating copy to clipboard operation
django-easy-audit copied to clipboard

Make REMOTE_ADDR optional

Open mschoettle opened this issue 2 years ago • 2 comments

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 avatar Oct 17 '23 15:10 mschoettle

@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.

mschoettle avatar Dec 08 '23 15:12 mschoettle

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 :)

jheld avatar Dec 08 '23 16:12 jheld