1215314 - Clean up some things about how auth works in BMO

Status: RESOLVED WONTFIX

(bugzilla.mozilla.org :: General, defect)

Description

[Dylan Hardison [:dylan] (he/him)]

There are several ways in which the auth stack code used in BMO can be simplified. BMO does not need to support the various auth methods that regular Bugzilla supports (and in fact, they cannot be used now because of the changes required by 2fa).

Comment 1

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1215314_1.patch

First of two, maybe three patches. This inlines the authenticators into the stack. The auth methods BMO does not use are removed and the parameters that specify them by order are also removed. github and persona gain two params (github_enabled and persona_enabled).

Comment 2

[Dylan Hardison [:dylan] (he/him)]

I should say, the other two will be much smaller -- basically there is no need for the Verify::Stack module (Verify::Github is basically useless, all we need is Verify::DB which can be named just ::Verify) and then I think it may make sense for all the authenticators to be configured in the same admin params section. With this patch reviewed and commited I can complete the firefox accounts work.

Comment 3

[David Lawrence [:dkl]]

Comment on attachment 8685194 [details] [diff] [review]
1215314_1.patch

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

Moving on with functional testing.

::: Bugzilla/Auth/Login/Stack.pm
@@ +31,1 @@
>  use Bugzilla::Constants;

Remove Bugzilla::Hook as no longer needed.

::: Bugzilla/Auth/Verify/Stack.pm
@@ +23,3 @@
>  );
>  
>  use Bugzilla::Hook;

Remove this as well.

Comment 4

[David Lawrence [:dkl]]

Comment on attachment 8685194 [details] [diff] [review]
1215314_1.patch

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

Syslog error (may not be related to patch):
[Wed Nov 18 20:51:29.631474 2015] [cgi:error] [pid 2715] [client 10.211.55.2:51705] AH01215: [Wed Nov 18 20:51:29 2015] editparams.cgi: Use of uninitialized value in string eq at /home/bugzilla/devel/htdocs/1215314/editparams.cgi line 132., referer: http://docker/1215314/editparams.cgi?section=persona
[Wed Nov 18 20:54:43.626475 2015] [cgi:error] [pid 2816] [client 10.211.55.2:51822] AH01215: [Wed Nov 18 20:54:43 2015] editparams.cgi: Use of uninitialized value in string eq at /home/bugzilla/devel/htdocs/1215314/editparams.cgi line 132., referer: http://docker/1215314/editparams.cgi?section=githubauth

Also remove:
Bugzilla/Config/LDAP.pm
Bugzilla/Config/RADIUS.pm
template/en/default/admin/params/ldap.html.tmpl
template/en/default/admin/params/radius.html.tmpl

Also template/en/default/admin/params/auth.html.tmpl needs to be updated to remove references to Env, LDAP, and RADIUS.
Question: Once those are gone, we are left with user_info_class = CGI and user_verify_class = DBI with no other choices.
Should we just then remove those two params completely or turn them into radio selects similar to Persona and Github. I 
am not sure anyone would ever want to run a Bugzilla where they didn't have CGI and DB to fall back to so we could just 
hardcode them to on.

Otherwise functional testing was fine and should be a quick r+ on the second iteration.

dkl

::: Bugzilla/Config/GitHubAuth.pm
@@ +28,5 @@
> +            type    => 't',
> +            default => '',
> +        },
> +        {
> +            name    => 'github_enabled',

Move github_enabled to top as well.

::: Bugzilla/Config/Persona.pm
@@ +33,5 @@
> +            type    => 't',
> +            default => '',
> +        },
> +        {
> +            name    => 'persona_enabled',

Nit: I would like to see persona_enable at the top since it will be the first thing we would want to set.

Comment 5

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1215314_4.patch

This should address the review concerns plus the issue I mentioned in our 1:1

Comment 6

[David Lawrence [:dkl]]

Comment on attachment 8689824 [details] [diff] [review]
1215314_4.patch

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

extensions/Persona/Config.pm:
Need to add JSON and LWP::UserAgent (already required) to list of required modules for persona feature in Bugzilla/Install/Requirements.pm

Otherwise looks/works good. Fixes on commit. r=dkl

::: template/en/default/account/auth/login.html.tmpl
@@ +109,5 @@
> +[% IF Param('persona_includejs_url') && Param('persona_enabled') %]
> +<p>
> +  <img src="images/persona_sign_in.png" width="185" height="25" onclick="persona_sign_in()">
> +</p>
> +[% END %]

Nit: Fix indentation.

Comment 7

[:glob ✱]

Comment on attachment 8689824 [details] [diff] [review]
1215314_4.patch

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

we need to migrate from the old stack params to persona_enabled/github_enabled to ensure environments continue to run without requiring reconfiguration.

::: Bugzilla/Config/Persona.pm
@@ +36,5 @@
> +        {
> +            name    => 'persona_proxy_url',
> +            type    => 't',
> +            default => '',
> +        },

since you're touching this code .. persona_proxy_url should be deleted; we should use 'proxy_url' instead