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




User Accounts
9 years ago
8 years ago


(Reporter: Michael Thomas (Mockodin), Assigned: Michael Thomas (Mockodin))




(1 attachment, 2 obsolete attachments)

4.95 KB, patch
Max Kanat-Alexander
: review-
Details | Diff | Splinter Review


9 years ago
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

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

9 years ago
Created attachment 387715 [details] [diff] [review]
Attachment #387715 - Flags: review?(mkanat)


9 years ago
Duplicate of this bug: 441027


9 years ago
Assignee: user-accounts → mockodin

Comment 3

9 years ago
Comment on attachment 387715 [details] [diff] [review]

The extern_id-editing parts of this should go into bug 423612.
Attachment #387715 - Flags: review?(mkanat) → review-


9 years ago
Depends on: 423612

Comment 4

9 years ago
Created attachment 405101 [details] [diff] [review]

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
Attachment #387715 - Attachment is obsolete: true
Attachment #405101 - Flags: review?(mkanat)

Comment 5

9 years ago
Created attachment 410447 [details] [diff] [review]

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..
Attachment #405101 - Attachment is obsolete: true
Attachment #410447 - Flags: review?(mkanat)
Attachment #405101 - Flags: review?(mkanat)

Comment 6

8 years ago
Comment on attachment 410447 [details] [diff] [review]

>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.
Attachment #410447 - Flags: review?(mkanat) → review-
You need to log in before you can comment on or make changes to this bug.