From 810ac042bacc992cc1cb3ae324b0d61b0615b697 Mon Sep 17 00:00:00 2001 From: "mkanat%bugzilla.org" <> Date: Sat, 21 Oct 2006 03:47:15 +0000 Subject: Bug 352243: Make editusers.cgi use Bugzilla::User for basic user updates Patch By Max Kanat-Alexander r=LpSolit, a=justdave --- Bugzilla/Object.pm | 43 ++++++++++++++++++++---- Bugzilla/User.pm | 97 +++++++++++++++++++++++++++++++++++++++++++++--------- editusers.cgi | 78 ++++++------------------------------------- 3 files changed, 127 insertions(+), 91 deletions(-) diff --git a/Bugzilla/Object.pm b/Bugzilla/Object.pm index 5d80a9d0f..a2ca8ff94 100644 --- a/Bugzilla/Object.pm +++ b/Bugzilla/Object.pm @@ -149,17 +149,28 @@ sub update { my $dbh = Bugzilla->dbh; my $table = $self->DB_TABLE; my $id_field = $self->ID_FIELD; + + my $old_self = $self->new($self->id); - my $columns = join(', ', map {"$_ = ?"} $self->UPDATE_COLUMNS); - my @values; + my (@update_columns, @values, %changes); foreach my $column ($self->UPDATE_COLUMNS) { - my $value = $self->{$column}; - trick_taint($value) if defined $value; - push(@values, $value); + if ($old_self->{$column} ne $self->{$column}) { + my $value = $self->{$column}; + trick_taint($value) if defined $value; + push(@values, $value); + push(@update_columns, $column); + # We don't use $value because we don't want to detaint this for + # the caller. + $changes{$column} = [$old_self->{$column}, $self->{$column}]; + } } + my $columns = join(', ', map {"$_ = ?"} @update_columns); + $dbh->do("UPDATE $table SET $columns WHERE $id_field = ?", undef, - @values, $self->id); + @values, $self->id) if @values; + + return \%changes; } ############################### @@ -452,9 +463,27 @@ data into the database. Returns a newly created object. =item C +=over + +=item B + Saves the values currently in this object to the database. Only the fields specified in L will be -updated. Returns nothing and takes no parameters. +updated, and they will only be updated if their values have changed. + +=item B (none) + +=item B + +A hashref showing what changed during the update. The keys are the column +names from L. If a field was not changed, it will not be +in the hash at all. If the field was changed, the key will point to an arrayref. +The first item of the arrayref will be the old value, and the second item +will be the new value. + +If there were no changes, we return a reference to an empty hash. + +=back =back diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm index 33c8535f5..ce778c728 100644 --- a/Bugzilla/User.pm +++ b/Bugzilla/User.pm @@ -67,8 +67,8 @@ use constant MATCH_SKIP_CONFIRM => 1; use constant DEFAULT_USER => { 'id' => 0, - 'name' => '', - 'login' => '', + 'realname' => '', + 'login_name' => '', 'showmybugslink' => 0, 'disabledtext' => '', 'disable_mail' => 0, @@ -82,8 +82,8 @@ use constant DB_TABLE => 'profiles'; # fixed one day. use constant DB_COLUMNS => ( 'profiles.userid AS id', - 'profiles.login_name AS login', - 'profiles.realname AS name', + 'profiles.login_name', + 'profiles.realname', 'profiles.mybugslink AS showmybugslink', 'profiles.disabledtext', 'profiles.disable_mail', @@ -101,6 +101,18 @@ use constant VALIDATORS => { realname => \&_check_realname, }; +sub UPDATE_COLUMNS { + my $self = shift; + my @cols = qw( + disable_mail + disabledtext + login_name + realname + ); + push(@cols, 'cryptpassword') if exists $self->{cryptpassword}; + return @cols; +}; + ################################################################################ # Functions ################################################################################ @@ -117,6 +129,29 @@ sub new { return $class->SUPER::new(@_); } +sub update { + my $self = shift; + my $changes = $self->SUPER::update(@_); + my $dbh = Bugzilla->dbh; + + if (exists $changes->{login_name}) { + # If we changed the login, silently delete any tokens. + $dbh->do('DELETE FROM tokens WHERE userid = ?', undef, $self->id); + # And rederive regex groups + $self->derive_regexp_groups(); + } + + # Logout the user if necessary. + Bugzilla->logout_user($self) + if (exists $changes->{login_name} || exists $changes->{disabledtext} + || exists $changes->{cryptpassword}); + + # XXX Can update profiles_activity here as soon as it understands + # field names like login_name. + + return $changes; +} + ################################################################################ # Validators ################################################################################ @@ -127,13 +162,18 @@ sub _check_disabledtext { return trim($_[1]) || ''; } # This is public since createaccount.cgi needs to use it before issuing # a token for account creation. sub check_login_name_for_creation { - my ($self, $name) = @_; + my ($invocant, $name) = @_; $name = trim($name); $name || ThrowUserError('user_login_required'); validate_email_syntax($name) || ThrowUserError('illegal_email_address', { addr => $name }); - is_available_username($name) - || ThrowUserError('account_exists', { email => $name }); + + # Check the name if it's a new user, or if we're changing the name. + if (!ref($invocant) || $invocant->login ne $name) { + is_available_username($name) + || ThrowUserError('account_exists', { email => $name }); + } + return $name; } @@ -152,13 +192,37 @@ sub _check_password { sub _check_realname { return trim($_[1]) || ''; } +################################################################################ +# Mutators +################################################################################ + +sub set_disabledtext { $_[0]->set('disabledtext', $_[1]); } +sub set_disable_mail { $_[0]->set('disable_mail', $_[1]); } + +sub set_login { + my ($self, $login) = @_; + $self->set('login_name', $login); + delete $self->{identity}; + delete $self->{nick}; +} + +sub set_name { + my ($self, $name) = @_; + $self->set('realname', $name); + delete $self->{identity}; +} + +sub set_password { $_[0]->set('cryptpassword', $_[1]); } + + ################################################################################ # Methods ################################################################################ # Accessors for user attributes -sub login { $_[0]->{login}; } -sub email { $_[0]->{login} . Bugzilla->params->{'emailsuffix'}; } +sub name { $_[0]->{realname}; } +sub login { $_[0]->{login_name}; } +sub email { $_[0]->login . Bugzilla->params->{'emailsuffix'}; } sub disabledtext { $_[0]->{'disabledtext'}; } sub is_disabled { $_[0]->disabledtext ? 1 : 0; } sub showmybugslink { $_[0]->{showmybugslink}; } @@ -187,7 +251,7 @@ sub identity { if (!defined $self->{identity}) { $self->{identity} = - $self->{name} ? "$self->{name} <$self->{login}>" : $self->{login}; + $self->name ? $self->name . " <" . $self->login. ">" : $self->login; } return $self->{identity}; @@ -199,7 +263,7 @@ sub nick { return "" unless $self->id; if (!defined $self->{nick}) { - $self->{nick} = (split(/@/, $self->{login}, 2))[0]; + $self->{nick} = (split(/@/, $self->login, 2))[0]; } return $self->{nick}; @@ -767,7 +831,7 @@ sub derive_regexp_groups { AND isbless = 0 AND grant_type = ?}); while (my ($group, $regexp, $present) = $sth->fetchrow_array()) { - if (($regexp ne '') && ($self->{login} =~ m/$regexp/i)) { + if (($regexp ne '') && ($self->login =~ m/$regexp/i)) { $group_insert->execute($id, $group, GRANT_REGEXP) unless $present; } else { $group_delete->execute($id, $group, GRANT_REGEXP) if $present; @@ -1101,10 +1165,11 @@ sub match_field { # skip confirmation for exact matches if ((scalar(@{$users}) == 1) - && (lc(@{$users}[0]->{'login'}) eq lc($query))) + && (lc(@{$users}[0]->login) eq lc($query))) + { $cgi->append(-name=>$field, - -values=>[@{$users}[0]->{'login'}]); + -values=>[@{$users}[0]->login]); next; } @@ -1117,7 +1182,7 @@ sub match_field { if (scalar(@{$users}) == 1) { # exactly one match $cgi->append(-name=>$field, - -values=>[@{$users}[0]->{'login'}]); + -values=>[@{$users}[0]->login]); $need_confirm = 1 if $params->{'confirmuniqueusermatch'}; @@ -1282,7 +1347,7 @@ sub wants_bug_mail { # # We do them separately because if _any_ of them are set, we don't want # the mail. - if ($wants_mail && $changer && ($self->{'login'} eq $changer)) { + if ($wants_mail && $changer && ($self->login eq $changer)) { $wants_mail &= $self->wants_mail([EVT_CHANGED_BY_ME], $relationship); } diff --git a/editusers.cgi b/editusers.cgi index 5f356fb40..a1c82db9f 100755 --- a/editusers.cgi +++ b/editusers.cgi @@ -226,9 +226,6 @@ if ($action eq 'search') { my $otherUser = check_user($otherUserID, $otherUserLogin); $otherUserID = $otherUser->id; - my $logoutNeeded = 0; - my @changedFields; - # Lock tables during the check+update session. $dbh->bz_lock_tables('profiles WRITE', 'profiles_activity WRITE', @@ -245,73 +242,19 @@ if ($action eq 'search') { action => "modify", object => "user"}); - my $login = $cgi->param('login'); - my $password = $cgi->param('password'); - my $realname = trim($cgi->param('name') || ''); - my $disabledtext = trim($cgi->param('disabledtext') || ''); - my $disable_mail = $cgi->param('disable_mail') ? 1 : 0; + $vars->{'loginold'} = $otherUser->login; # Update profiles table entry; silently skip doing this if the user # is not authorized. + my %changes; if ($editusers) { - my @values; - - if ($login ne $otherUser->login) { - # Validating untaints for us. - $login || ThrowUserError('user_login_required'); - validate_email_syntax($login) - || ThrowUserError('illegal_email_address', {addr => $login}); - is_available_username($login) - || ThrowUserError('account_exists', {email => $login}); - - push(@changedFields, 'login_name'); - push(@values, $login); - $logoutNeeded = 1; - - # Since we change the login, silently delete any tokens. - $dbh->do('DELETE FROM tokens WHERE userid = ?', {}, $otherUserID); - } - if ($realname ne $otherUser->name) { - # The real name may be anything; we use a placeholder for our - # INSERT, and we rely on displaying code to FILTER html. - trick_taint($realname); - push(@changedFields, 'realname'); - push(@values, $realname); - } - if ($password) { - # Validating untaints for us. - validate_password($password) if $password; - push(@changedFields, 'cryptpassword'); - push(@values, bz_crypt($password)); - $logoutNeeded = 1; - } - if ($disabledtext ne $otherUser->disabledtext) { - # The disable text may be anything; we use a placeholder for our - # INSERT, and we rely on displaying code to FILTER html. - trick_taint($disabledtext); - push(@changedFields, 'disabledtext'); - push(@values, $disabledtext); - $logoutNeeded = 1; - } - if ($disable_mail != $otherUser->email_disabled) { - push(@changedFields, 'disable_mail'); - push(@values, $disable_mail); - } - if (@changedFields) { - push (@values, $otherUserID); - $logoutNeeded && Bugzilla->logout_user($otherUser); - $dbh->do('UPDATE profiles SET ' . - join(' = ?,', @changedFields).' = ? ' . - 'WHERE userid = ?', - undef, @values); - # XXX: should create profiles_activity entries. - # - # We create a new user object here because it needs to - # read information that may have changed since this - # script started. - my $newprofile = new Bugzilla::User($otherUserID); - $newprofile->derive_regexp_groups(); - } + $otherUser->set_login($cgi->param('login')); + $otherUser->set_name($cgi->param('name')); + $otherUser->set_password($cgi->param('password')) + if $cgi->param('password'); + $otherUser->set_disabledtext($cgi->param('disabledtext')); + $otherUser->set_disable_mail($cgi->param('disable_mail')); + %changes = %{$otherUser->update()}; } # Update group settings. @@ -399,8 +342,7 @@ if ($action eq 'search') { delete_token($token); $vars->{'message'} = 'account_updated'; - $vars->{'loginold'} = $otherUser->login; - $vars->{'changed_fields'} = \@changedFields; + $vars->{'changed_fields'} = [keys %changes]; $vars->{'groups_added_to'} = \@groupsAddedTo; $vars->{'groups_removed_from'} = \@groupsRemovedFrom; $vars->{'groups_granted_rights_to_bless'} = \@groupsGrantedRightsToBless; -- cgit v1.2.3-24-g4f1b