madjar / pyramid_persona

Pyramid plugin to use persona for authentication
https://pyramid_persona.readthedocs.org/en/latest/
ISC License
19 stars 6 forks source link

Use a pre-built verifier object for lower per-request overhead #3

Closed rfk closed 12 years ago

rfk commented 12 years ago

Great work on this @madjar.

One improvement I would suggest is using an explicit, pre-built browserid.Verifier object to do the verification. This will pre-compile some regular expressions used for audience checking, meaning that the overhead per request will be slightly lower. It would also make it easier for any user who want to plug in a different implementation of the verifier, e.g. to do local verification.

(This PR also includes some tweaks to the way config loading interacts with the tests, the existing setup did not seem to work properly for me)

madjar commented 12 years ago

Great ! Thank.

About the test not working, I think I broke them when adding compatibility with pyramid 1.3, and forgot to rerun them. previously, the csrf check was not part of the view, but was in the view configuration, so it was ignored in the tests.

I should setup some pre-commit hook or travis to avoid this.

madjar commented 12 years ago

By the way, I don't think this will do anything performance-wise, because the default verifier is already cached in PyBrowserID (https://github.com/mozilla/PyBrowserID/blob/master/browserid/__init__.py#L32). The possibility for the user to configure the verifier is nice, though.

rfk commented 12 years ago

It will slightly improve performance, because of the way PyBrowserID does audience checking. If you call browserid.verify(assertion, audience) then the given audience will be compiled into a regexp on every call. Using a dedicated Verifier object and specifying the audience at creation-time allows it to be compiled once and then cached for future use.

Probably the speedup will not be noticeable, but every little bit helps :-)

madjar commented 12 years ago

Oh yeah, indeed, I missed that. Thanks.