241903 - Environment Variable Authentication

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, enhancement, P2)

Description

[Erik Stambaugh]

This would really be the simplest way of entering user information.  Login name
and so on are made available through environment variables by an external
source.  It is assumed that the presence of these variables means the user has
sucessfully passed through the external authentication system.

A large number of (regularly?) requested external authentication systems will be
able to work with this.

I have a patch that works with Bugzilla CVS, but I'd like to make it work with
the one I posted for bug 241900, which would require some changes.  I will post
that patch as soon as the necessary changes have been made.

Comment 1

[Erik Stambaugh]

Attachment: Bugzilla::Auth::Login::WWW::Env

This is probably not ready to be checked in, due to the issue in bug 253636. 
I'm willing to accept arguments to the contrary, though.

Env.pm optionally delegates user authentication to system environment variables
($REMOTE_USER, for example).  It requires the login_name to be present in one
variable.  The other variables are optional.

This patch adds a new column to the profiles table: 'extern_id'.  This is a
varchar that contains some kind of unique user information from the external
authentication source.	That allows a user's email address and realname to be
changed arbitrarily by the external source, and still keep the same profile.

I also added a 'flags' method to user objects, which makes certain information
about the logged in user's current session available.  The need for it here was
to allow the templates to know if the current user is allowed to log out, which
is not allowed by this module.

One thing this does not have that I would like is the ability to authenticate
using only an extern_id.  The problem is if a new ID is sent and Bugzilla does
not know the user's email address, a profile cannot be created.  A possible
workaround would be an intermediate screen that asks the user for their address
and probably verifies it before creating their profile.  I'd really like to see
that taken care of that in a later patch.

Comment 2

[Joel Peshkin]

Comment on attachment 155186 [details] [diff] [review]
Bugzilla::Auth::Login::WWW::Env



+if (!GetFieldDef("profiles", "extern_id")) {
+    AddField("profiles", "extern_id", "VARCHAR(64)");

don't start using all caps now.

Also, AddField checks before it adds, you don't need the IF unless you are
triggering some other data migration.

Add POD docs to User.pm for new methods.


+	     # Not having the email address defined but having an ID isn't
+	     # allowed.
+	     return undef unless $env_email;
+

It seems reasonable that someone might load user profiles into the database and
then just pass the userid, relying on the database to have a login_name.  


I need to create  an environment in which to test this.

Comment 3

[Erik Stambaugh]

(In reply to comment #2)

> It seems reasonable that someone might load user profiles into the database and
> then just pass the userid, relying on the database to have a login_name.  

It seems more likely than not.  I opened bug 254366 to add that enhancement
after this one lands.

Comment 4

[Erik Stambaugh]

Attachment: 241903-v2

Fixes the above and an oversight in one SQL statement (missing whitespace in a
concatenation)

Comment 5

[Christian Reis]

Why do we want an external id *and* an email? The way I see it there should be
only one identifier -- and it can be an id -- and the rest of the information
kept in the Bugzilla database.

What's the rationale for can_logout? Isn't this another one of our has_db-style
hacks?

Comment 6

[Christian Reis]

I think flags() should become a pair of accessors instead of getting and setting
in the same function. And I think that the Params need some sort of heavy
documentation I'd appreciate seeing in this patch (if we expect someone else to
be able to use this). 

It would get a lot of extra points we me if there was an example of how to rig
up Apache HTTP AUTH to this, in fact.

I register my consent with can_logout but it makes me highly unhappy to have to
add this sort of special-casing everywhere and I feel somehow I'm missing the
right solution to this.

Comment 7

[Joel Peshkin]

After applying this patch, attempts to logout result in....

Can't call method "login_class" on an undefined value at
Bugzilla/Auth/Login/WWW.pm line 42.


FWIW:  The way to make this work with apache is to add to the httpd.conf file...
    AllowOverride Limit AuthConfig

Then, in .htaccess ....

FilesMatch ^(.*\.pl|.*localconfig.*|runtests.sh)$>
  deny from all
</FilesMatch>
<FilesMatch ^(localconfig.js|localconfig.rdf)$>
  AuthType Basic
  AuthName "Bugzilla"
  AuthUserFile /home/bugzilla/passwd
  Require valid-user
  allow from all
</FilesMatch>
  AuthType Basic
  AuthName "Bugzilla"
  AuthUserFile /home/bugzilla/passwd
  Require valid-user
  allow from all
~
~

and provide a passwd file readable by the webserver at that location.  If the
users in the passwd file are full email addresses, that is fine. If not, regexp
must change and you must append a suffix to email addresses.

Then, set the user info method to ENV and set the env variable for email to
REMOTE_USER

I'll finish the review later.

Comment 8

[Joel Peshkin]

Also, we should still show people who they are in the footer even if we dont
offer to log them out.

Comment 9

[Joel Peshkin]

Comment on attachment 155296 [details] [diff] [review]
241903-v2

+# can_logout determines if a user may log out
+sub can_logout {
+    my $class = shift;
+    return 1 if ($class->login_class && $class->login_class->can_logout);
+    return 0;
+}
+

I suspect this is the culprit.

Humor kiko on the mechanisms for acessing flags.

Keep the login_name in the footer even if the user cannot logout. It is
confusing otherwise.

Do we want to make it possible for a user who authenticates via ENV bit does
not get a realname passed to edit realname?

Comment 10

[Erik Stambaugh]

Attachment: 241903-v3

This should address the above issues.

Comment 11

[Joel Peshkin]

All the critical stuff works well.  However, editing user prefs pretends to
allow me to edit my realname, but does not seem to do so.  It also looks like it
lets me change my password, but I would need to knwo my old password for that.

Comment 12

[Erik Stambaugh]

(In reply to comment #11)
> All the critical stuff works well.  However, editing user prefs pretends to
> allow me to edit my realname, but does not seem to do so.

It actually lets you change your realname in the profiles table.  The problem is
that next time Auth runs (on next page load), it sees that they differ and
treats it like an update, putting the old values back into the database.

This is probably close to the desired behavior, except for the part where it
gives the false impression that your data can be altered.  I'll make a new patch
where it keeps track of which fields may be edited.

> It also looks like it lets me change my password, but I would need to knwo
> my old password for that.

Password field is a special case.  It populated cryptpasswd with an impossible
string ("*", I think).  If you're using Env alongside CGI, you'll want some
records to be able to use the password field.

This is a sticky problem.

First, users should not be led into thinking they can change their password if
they're only using Env, and the password will not affect anything.

Second, if you're using Env and CGI, a user will have to be able to maintain
their password, so blocking the password dialog if they're logged in using Env
doesn't work.

Third, users who have "*" for a password either need to have the password dialog
turned off altogether or have some way to change their password from within Env.

I think for the time being it will suffice to simply suppress any password
dialogs if Env is being used, but in the future it'd be nice to be able to
maintain your CGI password from within Env.  Maybe only suppress the password
dialog if your cryptpassword is set to the default "*"?  I may have to think
about that one.

Comment 13

[Joel Peshkin]

That would be great if the login method was passing realname.  However, since
there is no realname variable defined, we probably don't want to ask for
realname in userprefs and then immediately wipe it out all the time.

So, if we ask for realname, we should respect it and not trounce it.  If we
offer to let someone change a password, it should work.

Comment 14

[Erik Stambaugh]

(In reply to comment #13)
> That would be great if the login method was passing realname.  However, since
> there is no realname variable defined, we probably don't want to ask for
> realname in userprefs and then immediately wipe it out all the time.
> 
> So, if we ask for realname, we should respect it and not trounce it.  If we
> offer to let someone change a password, it should work.

Oh... Oh!

I misread, or mis-thought, or something.  It's not supposed to replace realname
if it's not being passed.  I'll check that out in a couple of hours.

Comment 15

[Erik Stambaugh]

Attachment: 241903-v4

This should fix the problem where systems not providing a realname were
overwriting the user-supplied ones with "".

Comment 16

[Dave Miller [:justdave]]

We're adding an external_id field to the profiles table, but not providing any
way to edit it?  I realize you guys are creating users from an external source,
but what about everyone else?

Comment 17

[Joel Peshkin]

That is a link to the external system's identifier so that it can track changes
to login (email) for the same unique individual.  If the external system does
not pass that, it is not used.   It is, effectively, a foreign key.

Comment 18

[Dave Miller [:justdave]]

I'm still thinking there's stuff to be done here, but we can always add onto it
later as other folks actually start to use it.

Comment 19

[Joel Peshkin]

checked in for erik