SITE_URL should support multiple domains about django-browserid HOT 28 CLOSED

SITE_URL as a callable, just in general, is a +1 from me.

from django-browserid.

SITE_URL as a callable would be okay in general, but it would be rather useless if it couldn't accept arguments... Accepting the request would be great, but finding a way to support the Sites framework would be even better, as lots of sites use it.

from django-browserid.

Hm yeah I am in favor of supporting the Sites framework and falling back to SITE_URL.

from django-browserid.

What's the vulnerability if SITE_ID is not properly set? Is it possible to set it for a master domain, so it authenticates all subdomains so that I can set it for example.com to get authentication on test.example.com? That would take care of many use cases.

from django-browserid.

SITE_URL is sent to browserid.org during verification. browserid.org compares it to the intended audience stored inside the assertion to make sure that the assertion you were given is intended for your site. If it doesn't match, browserid rejects the verification.

This means that you could, for example, intercept an assertion for google.com and send an invalid audience to browserid.org, which would confirm the assertion. This doesn't really get you much of anything sensitive though.

You can't really use a master domain and have it assert for all subdomains (at least as far as I can see in any docs). Nor can you use the same domain for several sites (currently), because our get_audience code validates SITE_URL against the request. You could call verify directly and send the same audience across all your sites, but by that point you've cut SITE_URL out of the loop and this whole discussion is moot.

from django-browserid.

Hmm, I see... However, I did try "http://*.example.com" and it worked for all my domains, so that's odd.

from django-browserid.

Huh. Did not know that was possible. Will have to look into that further.

from django-browserid.

@callahad, can you shed some light on if wildcards are accepted in the audience string?

from django-browserid.

I don't think wildcards are supposed to be accepted. Weird.

from django-browserid.

Okay, the audience explicitly should not accept wildcards. That was a bug on our end.

from django-browserid.

Just a note. It is unclear from the documentation whether SITE_URL is a Django setting, which sent me searching the docs, or a special setting required by browser id. It also unclear how this setting is related (or not) to the SITE_ID setting found in Django docs.

PS. This is my first browser id integration.
PS2. @callahad Good talk at PyCon. It inspired me to look into this.

from django-browserid.

Thanks! I have no idea how to use Django, so I'm starting to learn me some django-browserid too. :)

This whole multiple-domain thing is going to come up again for GNU Mailman. You could probably get around this by setting audience to whatever the request environment says it is, but filtering that through a domain-specific whitelist.

E.g., "The browser says it's talking to example.com. My site believes it can be ("example.com", "example.net", "example.org"). Thus, I can go ahead and send this to the verifier.

from django-browserid.

This could probably leverage the ALLOWED_HOSTS setting, though that may assume too much (all hosts are valid log-in hosts and the contrapositive).

from django-browserid.

+1 for leveraging ALLOWED_HOSTS. Though if SITE_URL accepts lists, we could simply set it equal to ALLOWED_HOSTS to maintain configurability.

from django-browserid.

Working on a pull-request. What are your thoughts on making SITE_URL a list and calling it SITE_URLS? Seems like like it could solve a more general case.

from django-browserid.

Keep in mind that ALLOWED_HOSTS requires a domain only, without scheme and a port (these can't even be optionally specified). As far as I'm aware, assertion validation checks scheme and a port. Maybe, to avoid duplicated configs, you could use SITE_URLS and expose a simple function to convert SITE_URLS to ALLOWED_HOSTS?

from django-browserid.

On one hand I don't want to break existing sites by changing the SITE_URL setting to SITE_URLS and forcing them to change their settings. On the other hand SITE_URL accepting a list feels weird since it's singular. And I don't want to support two settings for the same thing if we can do it in one.

Supporting a list does seem like the simplest way to go, though. IMO keeping it as SITE_URL and supporting iterables and single strings is the way to go.

@willkg @peterbe : Thoughts?

from django-browserid.

I don't even understand how SITE_URL could be more than one because if it's depending on the request it's suddenly insecure because the whole point was to make it an additional step.

However, I can see how some people might want to have Person on "editor.mydomain.com" AND "www.mydomain.com" so perhaps you could do:

SITE_URL = ['editor.mydomain.com', 'www.mydomain.com']

And then we can do something in django-browserid to the effect of picking one that matches request.get_host() and barfs if it doesn't match any of them.

from django-browserid.

And then we can do something in django-browserid to the effect of picking one that matches request.get_host() and barfs if it doesn't match any of them.

Yeah, that's pretty much the suggestion. When we decide which domain to send to Persona we use the get_audience method and pass it the request, and that's where we currently barf if the request host doesn't match SITE_URL. The proposed change would alter that to do pretty much what you said in order to support a single Django installation that serves multiple domains.

from django-browserid.

And then we can do something in django-browserid to the effect of picking one that matches request.get_host() and barfs if it doesn't match any of them.

Right. I'm almost finished with a pull-request implementing that.

IMO keeping it as SITE_URL and supporting iterables and single strings is the way to go.

I think that's our best option.

Now.

Any thoughts about interacting with ALLOWED_HOSTS? It's an interesting case of repeated code. I don't have the answer, but what if SITE_URL defaulted to a domain converted ALLOWED_HOSTS?

It might be a more sensible, yet reasonably unbiased, default.

from django-browserid.

@wrr made the point above that SITE_URL can't draw from ALLOWED_HOSTS because it requires a port and protocol, whereas ALLOWED_HOSTS doesn't take either of those.

As for going the other way... eh. Seems like feature creep that isn't our job.

from django-browserid.

And ALLOWED_HOSTS can contain wildcards I think.

from django-browserid.

Having talked to @Kalail who's done a great jump starting a really promising pull request...

I think I have a preference for this issue. settings.SITE_URL should be allowed to be:

• a string
• a tuple or a list of strings

And that's it.

Why not a callable (with the request being passed)?
Because it's overkill. It would just lead to people's settings.py getting messy with a bunch of business logic. settings.py should just be settings and at most one or two simple if statements.

Now, the idea of using the Site framework is indeed very tempting! Unlike ALLOWED_HOSTS the Site framework will contain the full domain (plus optional port) and there are no wildcards.
However, this would have to be a new separate issue to integrate. I could see how useful this would be but generally I don't use the Site framework much. At most I use it for getting absolute URLs in offline generated email bodies that have links in them.

What do you think @Osmose ?

from django-browserid.

And that's it.

+1. We haven't been very good at knowing when to avoid adding something to the API, but I think this is a great place to avoid adding unnecessary stuff.

A string or an iterable should cover every reasonable situation. If it becomes a huge problem for someone in the future, we can considering adding it then.

As for the Sites framework, adding support would be interesting, but honestly I'm not a fan of the Sites framework in general. It's awkward to use IMO. I'm fine with not supporting it for the time being.

from django-browserid.

@Kalail the o mighty overlord @Osmose has spoken. Would you mind simplifying your pull request a bit?

from django-browserid.

Will do.

from django-browserid.

The pull request is merged. Can this be closed now?

from django-browserid.

Yes! Nice work @Kalail ! 🍰

from django-browserid.