136770 - unreachable code in DBNameToIdAndCheck()

Status: RESOLVED WORKSFORME

(Bugzilla :: Bugzilla-General, defect, P2)

Description

[John Vandenberg]

DBNameToIdAndCheck can be forced to create a new user if one does not exist,
however this functionality is no longer used.

Comment 1

[Dave Miller [:justdave]]

Isn't that used by the LDAP code to create the user if they exist in LDAP but
aren't in Bugzilla yet?

Comment 2

[John Vandenberg]

confirm_login calls PasswordForLogin($enteredlogin).  If no password is
returned, InsertNewUser is called.  As a result, this now bypasses the
ValidateNewUser in DBNameToIdAndCheck, which could result in addresses in the
process of an email change being hijacked when using LDAP.  

Due to the way LDAP users are added now, ValidateNewUser should be called from
within confirm_login before InsertNewUser.

Comment 3

[John Vandenberg]

Attachment: removes unreachable code; adds consistency check

The consistency check for adding new users under LDAP (the block in CGI.pl) has
not been tested.

Comment 4

[Bradley Baetz (:bbaetz)]

Comment on attachment 79229 [details] [diff] [review]
removes unreachable code; adds consistency check

Change the first line of DBNameToIDAndCheck to not refer to/declare $forceok.

Comment 5

[John Vandenberg]

Attachment: v.2

Comment 6

[Bradley Baetz (:bbaetz)]

Comment on attachment 79231 [details] [diff] [review]
v.2

r=bbaetz

I'll claim ignorance on the ldap stuff, though, although it wasn't using a
second param to start with, so this won't affect that, at least.

(BTW, should email change stuff be disabled for ldap?)

Comment 7

[John Vandenberg]

I dont know and could not find a LDAP expert on
http://www.bugzilla.org/who_we_are.html.  Would be good to fix this, as I am
sure an administrator of an LDAP installation would prefer to be asked now
rather than have us blitz their setup.

I can see why 'email change' in specific should be disabled, as there is no
other means to sync. with the LDAP directory.  I can also see the benefit of
adding a param to lock all account details down to their LDAP values, but that
is another issue.

Comment 8

[Dave Miller [:justdave]]

CCing the guy who wrote the original LDAP stuff...

Comment 9

[John Vandenberg]

Comment on attachment 79231 [details] [diff] [review]
v.2

The majority of this issue has been resolved in bug 138456.  All that remains
is the LDAP issues.

Comment 10

[Joel Peshkin]

These unloved bugs have been sitting untouched since June 2002 or longer.  If
nobody does anything else to them, they certainly won't make 2.18
Retargetting to 2.20.  If you really plan to push them right now, you might pull
them back in.

Comment 11

[Max Kanat-Alexander]

We totally re-wrote LDAP, so um, this bug should probably be closed. FIXED, or
WORKSFORME, or...?

Comment 12

[Max Kanat-Alexander]

Yeah, this is no longer an issue with the modern LDAP code.

Comment 13

[Dave Miller [:justdave]]

clearing target of DUPLICATE/WONTFIX/INVALID/WORKSFORME so they'll show up as
untriaged if they get reopened.