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

User Agent Detection #31

Closed
smajda opened this Issue Oct 21, 2014 · 5 comments

Comments

Projects
None yet
2 participants
@smajda
Contributor

smajda commented Oct 21, 2014

I've used this package on several sites now and have run into two different, related problems:

First, I built an internal business application that ran into problems because the company's internal firewall was making a GET request to the login code URLs before passing it along to the user's browser, resulting in a 404 because the firewall's request invalidated the login code. This is, of course, exceptionally annoying IT behavior, but my workaround was to customize the login view to detect their firewall's IP and simply return a 200 to it.

Second, I recently found (and blogged about) an issue where applications that use Facebook App Links can cause Facebook to make a GET request to any URL clicked in the application, also invalidating the login code. This is, of course, also exceptionally annoying behavior, and my workaround was to use the user-agents package to return a HttpResponseForbidden to invalid user agents. Here's my modified login_with_code:

def login_with_code(request, login_code):
    ua = user_agents.parse(request.META.get('HTTP_USER_AGENT') or '')
    if not (ua.is_pc or ua.is_mobile or ua.is_tablet):
        return HttpResponseForbidden()
    code = get_object_or_404(LoginCode.objects.select_related('user'), code=login_code)
    return login_with_code_and_username(request, username=get_username(code.user),
                                    login_code=login_code)

I don't know if there's anything that could be done within django-nopassword to handle these situations better. It seems heavy-handed to require user-agents as a dependency (along with it's dependencies, as it uses ua-parser), but the alternative would be to bundle some basic user agent detection into django-nopassword, which seems outside of the scope. On the other: this could effect a lot of people using this package so maybe it's worth it?

Likewise, the first situation above, that of the evil corporate firewall, could be solved by possibly adding a ip address blacklist setting. On the other hand, that also could be seen as outside the scope of this package.

Maybe the solution is just documentation explaining how to work around each of these scenarios?

What do you think? This has affected me on multiple occasions so I'm happy to help out with some solution, but I'm not sure what the best solution would be.

@relekang relekang added the ★★★ label Oct 21, 2014

@relekang

This comment has been minimized.

Show comment
Hide comment
@relekang

relekang Oct 21, 2014

Owner

Thank you for reporting this! I think that some blame should go to django-nopassword for not following the http standard. It currently changes state on a GET request, which is not allowed and at least not optimal. I just haven't thought about it before now.

The first solution fixing that problem that I can come up with is to do an ajax POST request on the page and use window.location to redirect the page after the login is complete. That should solve both of your situations and probably some others too. What do you think?

Owner

relekang commented Oct 21, 2014

Thank you for reporting this! I think that some blame should go to django-nopassword for not following the http standard. It currently changes state on a GET request, which is not allowed and at least not optimal. I just haven't thought about it before now.

The first solution fixing that problem that I can come up with is to do an ajax POST request on the page and use window.location to redirect the page after the login is complete. That should solve both of your situations and probably some others too. What do you think?

smajda added a commit to smajda/django-nopassword that referenced this issue Oct 21, 2014

@smajda

This comment has been minimized.

Show comment
Hide comment
@smajda

smajda Oct 21, 2014

Contributor

That's a good point about stateless GETs I hadn't considered either.

I just pushed a commit showing a simple quick implementation of this. What do you think?

Would it be better to have this be optional?

Contributor

smajda commented Oct 21, 2014

That's a good point about stateless GETs I hadn't considered either.

I just pushed a commit showing a simple quick implementation of this. What do you think?

Would it be better to have this be optional?

@relekang

This comment has been minimized.

Show comment
Hide comment
@relekang

relekang Oct 21, 2014

Owner

Looks good. It would be nice if it was optional, but turned on by default. What about NOPASSWORD_POST_REDIRECT with True as default?

Owner

relekang commented Oct 21, 2014

Looks good. It would be nice if it was optional, but turned on by default. What about NOPASSWORD_POST_REDIRECT with True as default?

@relekang relekang added this to the 1.1 milestone Oct 21, 2014

smajda added a commit to smajda/django-nopassword that referenced this issue Oct 21, 2014

@smajda

This comment has been minimized.

Show comment
Hide comment
@smajda

smajda Oct 21, 2014

Contributor

Easy enough to add and may help some unforeseen use cases where the "POST with javascript" option isn't viable.

Contributor

smajda commented Oct 21, 2014

Easy enough to add and may help some unforeseen use cases where the "POST with javascript" option isn't viable.

@smajda smajda referenced this issue Oct 24, 2014

Merged

Login with POST #32

@relekang

This comment has been minimized.

Show comment
Hide comment
@relekang

relekang Oct 24, 2014

Owner

Fixed by #32

Owner

relekang commented Oct 24, 2014

Fixed by #32

@relekang relekang closed this Oct 24, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment