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

Make REMOTE_ADDR optional #263

Closed
mschoettle opened this issue Oct 17, 2023 · 2 comments · Fixed by #272
Closed

Make REMOTE_ADDR optional #263

mschoettle opened this issue Oct 17, 2023 · 2 comments · Fixed by #272

Comments

@mschoettle
Copy link
Contributor

mschoettle commented Oct 17, 2023

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:

remote_ip = environ.get(REMOTE_ADDR_HEADER, None)

@mschoettle
Copy link
Contributor Author

@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
Copy link
Collaborator

jheld commented Dec 8, 2023

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

samamorgan added a commit to samamorgan/django-easy-audit that referenced this issue Feb 5, 2024
@jheld jheld closed this as completed in #272 Mar 8, 2024
jheld pushed a commit that referenced this issue Mar 8, 2024
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 a pull request may close this issue.

2 participants