Comment 17 for bug 1468422

Revision history for this message
Dan Wells (dbw2) wrote :

We've got this code up and running (with AuthProxy modifications) in production, and it's working well so far. Excellent work, Bill!

Having put it through it's paces for this type of use, I do have a few questions.

First, the easier ones. The current auth API uses 'type' and 'org' for argument names, but the auth_internal code expects 'login_type' and 'org_unit' for the same values. Should we consider keeping the names the same instead? It's slightly more predictable, but if we are instead trying to purposefully improve the names, I can see that side as well. Just wanted to point it out and see what others felt.

Second, we may want to consider splitting apart or branching some parts of oilsAuthComplete() a little more. When AuthProxy.pm was a straight-up wrapper for the real authentication, we got a lot of value for free. In short, we got all of what I would call user "validation" (card is active, user not barred, user active, user has login perm). In the current setup, we would have to redo all that in the proxy layer (not a huge deal, but clumsy).

One possibility is to shove more of these checks (and probably stats, etc.) down into the auth_internal layer (either in the session.create call, or (maybe better) in a separate call or some kind). We probably want to avoid doing any kind of password-less validation in the public side, since anyone could potentially learn (admittedly unexciting) private information about any user account.

Thoughts? I am willing to take a stab at it if needed, but might be barking up the wrong tree.