search

mozilla / django-browserid

Django application for adding BrowserID support.
Mozilla Public License 2.0
180 stars 80 forks source link

SITE_URL doesn't accept a path, although it sounds like it should. #159

Closed sebasmagri closed 10 years ago

sebasmagri commented 11 years ago

Currently, get_audience[0] fails if SITE_URL contains a path in addition to the protocol and domain (https://domain/path/). The request path is ignored when comparing the request URL and the site URL.

OTOH, the exception message have a typo in 'Settting'. ;)

ImproperlyConfigured: SITE_URL incorrect. Settting is `https://domain/path/`, but request was `https://domain`

[0] https://github.com/mozilla/django-browserid/blob/master/django_browserid/base.py#L31

thekashifmalik commented 11 years ago

Isn't that intended behaviour? I thought SITE_URL was supposed to be used for checking domains. Do we want to check more specifically for the path?

Currently on Master SITE_URL can take an iterable or a string. I don't know if that's helpful at all, but I think the question to ask is;

'What should SITE_URL be used for; checking the request domain or the full request path?'

Osmose commented 11 years ago

Ah, I misread this bug at first, thinking it was for changing the exception message (saying that it suggested something with a path when that didn't work). I may also have been sleep-deprived.

SITE_URL is used for validating that the assertion we got from Persona is being sent to the correct site (assertions contain which domain they're meant for). It shouldn't have a path in it. However, it is understandable that you'd expect for it to support that because of the name of the setting.

I'm on the fence; SITE_URL is a rather common setting (despite being non-standard, I've seen it pop up a few times) so we want to support that, but it'd be better to rename it to something more explicit, or at least add a note to the docs mentioning this.

Should we rename it, or just update the docs?

thekashifmalik commented 11 years ago

I vote rename it. It's the cleaner solution. Plus this came up before too with the name SITE_URL not hinting at support for multiple urls.

sebasmagri commented 11 years ago

I also vote on renaming it. SITE_URL is pretty common and its concept is broad, and django-browserid's behaviour regarding its value is pretty specific.

Thanks!

Osmose commented 10 years ago

SITE_URL has been renamed/replaced with BROWSERID_AUDIENCES as of #202. So we should be good! Thanks everyone!