503372 - Bugzilla::Login::Env should allow log in based on auth_env_id OR auth_env_email

Status: ASSIGNED

(Bugzilla :: User Accounts, defect)

Description

[Michael Thomas (Mockodin)]

Not all server based auth is equal, some web servers do not use email as the primary means of authentication and provide no provision for looking one up.

This bug is a clone of Bug 320265 but solves the issue in a different manner, for the sake simplicity this was the description of the issue.

Artur Kedzierski (Bug 320265) Said:

Currently Env.pm requires auth_env_email variable to be set. However, it should
be able to lookup the email address when auth_env_id is set.

This would be great for users who don't use their email address as a login
name. They could just set auth_env_id to REMOTE_USER and leave auth_env_email
blank.


Reproducible: Always

Steps to Reproduce:
1. Set auth_env_id to REMOTE_USER
2. Leave auth_env_email blank


Actual Results:  
Login is denied. The email address is not passed nor it is looked up so it
doesn't pass $emailregexp test.

Expected Results:  
Login is accepted. The email address is looked up from the database so it
passes $emailregexp test.

//End Copy//

For this bug I am incorporating this bug and Bug 423612 to provide a more universally usable method of ENV login.

Desired Result:

auth_env_id is matched against extern_id in profiles to provide the login_name to Auth. Optionally (if not passed from ENV) realname is also queried at the same time.

Login is accepted

Bugzilla continues to rely on email as the username and optionally uses extern_id as a look up field against the ENV suplied external id.


Required Changes:

 1. Add 'auth_env_method' to User Authentication parameters possible values
           auth_env_id or auth_env_email (default)

 2. Expose extern_id to admins in editusers.cgi
           But only if 'auth_env_method' set to auth_env_id

 3. Modify Bugzilla::Login::Env to check value of 'auth_env_method' process login based on primary auth type

Comment 1

[Michael Thomas (Mockodin)]

Attachment: v1

Comment 3

[Max Kanat-Alexander]

Comment on attachment 387715 [details] [diff] [review]
v1

The extern_id-editing parts of this should go into bug 423612.

Comment 4

[Michael Thomas (Mockodin)]

Attachment: v2

Removed edits to UI for edituser as those now handled in bug 423612

Modified ENV.pm to conditionally use constant extern_id_used when auth_env_method is set to auth_env_id

Comment 5

[Michael Thomas (Mockodin)]

Attachment: v3

Noticed that 'requires_persistence' is set as true which causes a new cookie to be written on every page request. Since ENV auth is re authenticating the user every time anyhow there is no need to record a 'Bugzilla_logincookie' at all.

Added 'use constant requires_persistence => 0;' to Bugzilla::Auth::Login::Env

Removed changes already being addresses in bug 423612, doesn't appear they were successfully removed from previous patch file..

Comment 6

[Max Kanat-Alexander]

Comment on attachment 410447 [details] [diff] [review]
v3

>Index: bugzilla/Bugzilla/Auth/Login/Env.pm
>+if (Bugzilla->params->{"auth_env_method"} eq 'auth_env_id') {
>+    use constant extern_id_used => 1;
>+}

  "use" is evaluated at compile-time, so you're not conditionally defining a constant here, you're always defining a constant.

  Also, it's not OK to have live code outside of subroutines.

  What you should do here instead is create a sub called extern_id_used. (constants and subs work the same way--that's why we use constants).

>+        my $dbh = Bugzilla->dbh;
>+        my ($db_email, $db_realname) = $dbh->selectrow_array(
>+            "SELECT login_name, realname FROM profiles WHERE extern_id = ?",
>+            undef, $env_id);

  This is not the place to get data from the database--you don't know that the database is the account source, and you may not even have a user yet. You should only be returning what you were sent, not checking anything in the database.

>Index: bugzilla/template/en/default/admin/params/auth.html.tmpl
>+                 "field for environmental authentication if auth_env_method is set to auth_env_id. Leave it blank " _

  Nit: line too long.

>+                    "field for environmental authentication if auth_env_method is set to auth_env_email. Leave it blank " _

  Same there.

>+  auth_env_method => "Defines which environment variable will be used as primary login lookup method.",

  And there.