390802 - In the "Name and Password" panel of userprefs.cgi, leaving the "Password" field empty throws an error

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect)

Description

[Frédéric Buclin]

Attachment: patch, v1

The old password doesn't need to be given to change the real name only (as it's a safe change which can easily be reverted if desired), but Bugzilla->login complains if this field is left empty, because Bugzilla::Auth::Login::CGI thinks the password is "", which is obviously not the correct one. So we have to delete Bugzilla_password completely (i.e. undef) so that Bugzilla->login looks at the login cookie instead and throws an error only if credentials checks really fail.

Comment 1

[Marc Schumann [:Wurblzap]]

I kinda thought this was expected behaviour. After all, the page says "Please enter your existing password to confirm account changes".

Comment 2

[Frédéric Buclin]

No, the code updating the real name is intentionally left outside any check. Only changing your password or your email address requires explicitly checks.

Comment 3

[Max Kanat-Alexander]

Comment on attachment 275118 [details] [diff] [review]
patch, v1

Before I give this r+, I think we should audit userprefs.cgi to make sure that this won't introduce any security holes.

Comment 4

[Frédéric Buclin]

Comment on attachment 275118 [details] [diff] [review]
patch, v1

Talking about this with mkanat on IRC, we decided it was better to request the password for *all* changes, including the real name. This way, no risk to bypass checks. I will update my patch accordingly.

Comment 5

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.

Comment 6

[Sunil Joshi [:sjoshi]]

Attachment: Patch-v1

Implemented as per comment#4

Comment 7

[Tiago Mello [:timello]]

Comment on attachment 690949 [details] [diff] [review]
Patch-v1

The change looks trivial. It applies cleanly and it passes in all tests.

Comment 8

[Frédéric Buclin]

(In reply to Tiago Mello [:timello] from comment #7)
> The change looks trivial.

I don't think changes are trivial. Did you test with login methods which do not use passwords?

Comment 9

[Tiago Mello [:timello]]

Comment on attachment 690949 [details] [diff] [review]
Patch-v1

The patch does not work when the password is not needed. For instance when user_info_class is set to 'Env'. It should also check if the login method requires a password (requires_verification constant, possibly).

Comment 10

[Frédéric Buclin]

Fixed by bug 553693. No other action required.