search

mozilla / django-browserid

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

Cast audience to string because of possible lazily evaluated variables. #300

Closed jotes closed 7 years ago

jotes commented 7 years ago

Hi @Osmose,

Could you share your thoughts/review this?

Thanks in advance.

jotes commented 7 years ago

Our use-case: mozilla/pontoon#446

Osmose commented 7 years ago

This looks fine and works, nice job!

However, while I was refreshing my brain to be able to properly review this, I thought of two things:

  1. It might be simpler to just make django_browserid.base.get_audience handle the lazy value rather than casting everywhere, although it would be less flexible than your current solution, which handles lazy values for custom backends and such better.
  2. It might even be simpler to have Pontoon use a custom auth backend and override BrowserIDBackend.verify to cast the audience when you receive it. That'd avoid having to do a release for this library, which I'd like to avoid.

Thoughts?

jotes commented 7 years ago

I think the second option would be the best if @mathjazz will approve. I just thought this will be more general case that could be solved in BrowserID-browser. Again, thx for help!

Osmose commented 7 years ago

I just thought this will be more general case that could be solved in BrowserID-browser.

It is, but given the doom surrounding Persona I'm not really up for general improvements to the library.

DOOM

Thanks again for the PR though!

mathjazz commented 7 years ago

I think the second option would be the best if @mathjazz will approve.

I approve anything that allows us to not release a new version of django-browserid 2 months before Persona is doomed. ;)