692709 - add support for authenticating via repoze.who

Status: VERIFIED FIXED

(Cloud Services :: Server: Core, enhancement)

Description

[Ryan Kelly [:rfkelly]]

Attachment: patch adding authentication via repoze.who

Here's my work so far on adding repoze.who support in server-core:

There's a new Authentication class services.whoauth.WhoAuthentication that
uses a repoze.who plugin stack for authenticating.  In its default configuration, it acts identically to the base Authenticaion class (i.e. basic-auth against configured auth backend).

However, you can set the config key "auth.who_config_file" to point to a repoze.who configuration file in the format described here:

    http://docs.repoze.org/who/2.0/configuration.html#configuring-repoze-who-via-config-file

The WhoAuthentication class will pick that up and use it instead of the default config.  Third-party installs of sync could use this to auth against an existing password database just by tweaking their config file.

This is basically all the auth work I intend to do in server-core for the moment - the next steps are to build out alternative auth mechanisms as repoze.who plugins that can be shared between server-core and pyramid.

(I considered trying to make WhoAuthentication the *only* auth mechanism in server-core, replacing the "auth_class" argument of SyncServerApp with a repoze.who config file.  But small changes are probably better at this stage)

Comment 1

[Tarek Ziadé (:tarek)]

Comment on attachment 565442 [details] [diff] [review]
patch adding authentication via repoze.who

That looks good. Any chance we can keep a single config file ?  Sync repoze.who uses ConfigParser, maybe we could add prefixed sections
and pass it to it.


...
>+    def _find_backend_plugins(self, api_factory):
>+        """Return set of BackendAuthPlugin objects used by the api factory."""
>+        # Find any configured instances of BackendAuthPlugin.
>+        backend_plugins = set()

Superfluous parenthesis, can be:

  for name, plugin in api_factory.identifiers:


>+        for (name, plugin) in api_factory.identifiers:
>+            if isinstance(plugin, BackendAuthPlugin):
>+                backend_plugins.add(plugin)
>+        for (name, plugin) in api_factory.authenticators:
>+            if isinstance(plugin, BackendAuthPlugin):
>+                backend_plugins.add(plugin)
>+        for (name, plugin) in api_factory.challengers:
>+            if isinstance(plugin, BackendAuthPlugin):
>+                backend_plugins.add(plugin)
>+        for (name, plugin) in api_factory.mdproviders:
>+            if isinstance(plugin, BackendAuthPlugin):
>+                backend_plugins.add(plugin)
>+        return backend_plugins

...
>+    def authenticate(self, environ, identity):
>+        # Get the username and password from the identity.
>+        # If we don't have them, we can't authenticate.

typo: username

(if the tests don't break on this, there's a missing test)


>+        username = identity.get("userame", identity.get("login", None))
>+        password = identity.get("password", None)
>+        if username is None or password is None:
>+            return None

Comment 2

[Ryan Kelly [:rfkelly]]

> Any chance we can keep a single config file?
> Sync repoze.who uses ConfigParser, maybe we could add prefixed
> sections and pass it to it.

I looked into this briefly but didn't come up with a solution (short of duplicating all the config-parsing logic outselves).  It would definitely be an improvement so I'll look harder.

> typo: username
> (if the tests don't break on this, there's a missing test)

Good catch, thanks.  The repoze.who Basic-Auth plugin actually provides "login" as the key, which is why this wasn't failing the tests.

I personally dislike using "login" as the key but maybe I should probably just go with what repoze gives us, which would make the code:

+        username = identity.get("login", None)
+        password = identity.get("password", None)
+        if username is None or password is None:
+            return None

Comment 3

[Tarek Ziadé (:tarek)]

Yeah I agree that having two names for the same thing would be painful. So I guess we could adopt "login" or we'd need to convert "login" to username before our code sees/uses it

Comment 4

[Rob Miller [:rmiller]]

Comment on attachment 565442 [details] [diff] [review]
patch adding authentication via repoze.who

Review of attachment 565442 [details] [diff] [review]:
-----------------------------------------------------------------

This all looks fine to me, but with the same comments that Tarek already gave and w/ one additional trivial docstring typo fix.

::: services/whoauth/backendauth.py
@@ +34,5 @@
> +# the terms of any one of the MPL, the GPL or the LGPL.
> +#
> +# ***** END LICENSE BLOCK *****
> +"""
> +repoze.who IAuthentictor plugin that auths to services auth backend.

typo: "IAuthentictor"

Comment 5

[Ryan Kelly [:rfkelly]]

Attachment: updated patch for repoze.who authentication

Thanks for the feedback, I have updated accordingly.  I've also made two semantic changes:

* WhoAuthentication.acknowledge now gives all IIdentifier plugins a chance to remember the successful login, instead of just the one that extracted the credentials.  This mirrors the login in the repoze.who "login" method - calling it directly would re-check the already authenticated credentials.

* likewise, WhoAuthentication._raise_challenge now gives all IIdentifier plugins a chance to forget the invalid login.  This directly calls the repoze.who "logout" method.

I've used these updates to add a signed-cookie auth layer in Bug 693533, and it seems to be working out nicely.  So I think the core APIs defined in this patch are now ready to go.

Comment 6

[Ryan Kelly [:rfkelly]]

> So I think the core APIs defined in this patch are now ready to go.

Oh, except for Tarek's comment about using the existing configuration file instead of linking to a separate file.

Using prefixed sections like [who:plugin:basicauth] sounds like a good idea to me.  Unfortunately the repoze.who config-file loader doesn't look very extensible, so I'm not sure we can do that without duplicating large chunks of the parsing logic.

Comment 7

[Toby Elliott [:telliott]]

(In reply to Ryan Kelly [:rfkelly] from comment #6)
>Unfortunately the repoze.who config-file loader doesn't look very
> extensible, so I'm not sure we can do that without duplicating large chunks
> of the parsing logic.

Monkeypatchable?

Comment 8

[Ryan Kelly [:rfkelly]]

Comment on attachment 566134 [details] [diff] [review]
updated patch for repoze.who authentication

> Monkeypatchable?

At worst, we could render the who config into a stand-alone ini file in memory, then pass it in as a StringIO. 

But it sounds like Tarek has a patch that is worth submitting upstream, so I'll coordinate with him and defer this until we know how to move forward on that front.

Comment 9

[Ryan Kelly [:rfkelly]]

Attachment: updated patch for repoze.who authentication

I have updated this patch to parse the who config sections out of the existing config file, and to use the modified backend API from Bug 705988.

Comment 10

[Rob Miller [:rmiller]]

Comment on attachment 577497 [details] [diff] [review]
updated patch for repoze.who authentication

Review of attachment 577497 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly this looks great, just some small improvements that could be made.

::: services/whoauth/__init__.py
@@ +103,5 @@
> +                    pass
> +                else:
> +                    section = section.replace(".", ":")
> +                    who_ini_lines.append("[%s]" % (section,))
> +                    who_ini_lines.append("%s = %s" % (var, value))

Our config object has some "get_section" helper methods that I think could simplify this code a bit.

@@ +225,5 @@
> +                self._add_basicauth_settings(config)
> +            if "digest" in supported_schemes:
> +                self._add_digestauth_settings(config)
> +
> +    def _add_basicauth_settings(self, config):

Side effect-y mutation of the config object smells a little funny to me.  Maybe the code above could be changed to:

if "basic" in supported_schemes:
    config.update(self._basicauth_settings(config))
if "digest" in supported_schemes:
    config.update(self._digestauth_settings(config))

and the _*auth_settings methods can just return the update dictionary?

Comment 11

[Ryan Kelly [:rfkelly]]

Attachment: updated patch for repoze.who authentication

Thanks Rob.  I've changed it to use config.get_section("who") to pull all the who-related config into a separate dict.  Unfortunately I don't think I can use get_section for pulling out the individual who-config sections, since we don't necessarily know what all the sections will be named.

I have also removed the support for digest-auth at this stage.  I'll add it back in as a separate patch once we resolve the issues raised in Bug 705989.

Comment 12

[Ryan Kelly [:rfkelly]]

Committed in http://hg.mozilla.org/services/server-core/rev/8cc51c063922