Bug 146923 – webkit-patch uses incorrect credentials from keychain to login until account lockout

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Dean Johnson]

If you have incorrect credentials in a keychain for logging onto bugs.webkit.org and you attempt to use webkit-upload -g you will be put into a loop of login attempts until you are locked out. 

Fix: If credentials are incorrect, ask the user to enter them manually instead of trying continuously.

[Dean Johnson]

After a quick look the issue seems to lie somewhere in OpenSource/Tools/Scripts/webkitpy/tool/steps/commit.py.

There looks like there should be a 3 try attempt, but maybe the interaction with the keychain is causing issues.

[Dean Johnson]

Created attachment 256794 [details]
Patch

[Dean Johnson]

Created attachment 256795 [details]
Patch

[Dean Johnson]

Both attachments are the same, one was just testing my fix.

[Daniel Bates]

Comment on attachment 256795 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256795&action=review

> Tools/Scripts/webkitpy/common/net/credentials.py:140
>          username, password = self._credentials_from_environment()

R-, the proposed patch does not handle credentials that are retrieved from the environment. There are also other minors issues with this patch. I'll elaborate further tomorrow.

[Daniel Bates]

Comment on attachment 256795 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256795&action=review

> Tools/ChangeLog:11
> +        you in until lockout of bugzilla.

This description is disingenuous. The webkitpy logic to read credentials will make up to five attempts to authenticate with Bugzilla. And it sounds like five attempts is more than Bugzilla allows before locking out an account.

> Tools/ChangeLog:14
> +        (Bugzilla.authenticate):Added argument to read_credentials() that

Nit: Missing space character after ':'.

> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:532
> +            # Make sure we don't loop over invalid credentials:
> +            # https://bugs.webkit.org/show_bug.cgi?id=146923
> +            username, password = credentials.read_credentials(previous_fail=attempts > 1)

I suggest that we come up with more descriptive name for the argument previous_fail. As we discussed in person today, maybe name it use_stored_credentials and write line 532 as:

username, password = credentials.read_credentials(use_stored_credentials=attempts == 1)

Then we can remove the need for the comments on lines 530 and 531 to explain the purpose of argument previous_fail.

> Tools/Scripts/webkitpy/common/net/credentials.py:139
> +    def read_credentials(self, user=User, previous_fail=False):

As we discussed in person today, maybe a more descriptive name for this argument is use_stored_credentials.

> Tools/Scripts/webkitpy/common/net/credentials.py:143
> +        # Don't auto-fill incorrect passwords if they've been filled in incorrectly once.

Please remove this comment as it is not applicable to the purpose of this function, which is to return credentials regardless of the validity of such credentials. Although this comment would be better suited in Bugzilla.authenticate(), I feel it is unnecessary if we pick a more descriptive name for the argument previous_fail.

[Dean Johnson]

Created attachment 256846 [details]
Patch

[Daniel Bates]

Comment on attachment 256846 [details]
Patch

Can we write a test for this change?

[Daniel Bates]

Comment on attachment 256846 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256846&action=review

> Tools/ChangeLog:15
> +        (Bugzilla.authenticate):Added argument to read_credentials() that

Nit: Missing space character after ':'.

[Dean Johnson]

Created attachment 256877 [details]
Patch

[Daniel Bates]

Comment on attachment 256877 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256877&action=review

> Tools/ChangeLog:24
> +        use_stored_password argument.

use_stored_password => use_stored_credentials

> Tools/ChangeLog:25
> +        (test_keyring_without_git_repo_nor_keychain): This wasn't actually changed...

This is a bug in prepare-ChangeLog. Please file a bug for this issue and remove this line from the ChangeLog.

> Tools/ChangeLog:28
> +        keyring) are never called. It also tests that we get back our mocked username and password.

Nit: git => Git

> Tools/ChangeLog:29
> +        (test_do_not_use_stored_credentials.MockKeyring): Mock keyring

Either elaborate further or remove this remark.

> Tools/ChangeLog:31
> +        (test_do_not_use_stored_credentials.FakeCredentials): Mock credentials

Ditto.

> Tools/ChangeLog:36
> +        (test_do_not_use_stored_credentials.FakeCredentials
> +        ._offer_to_store_credentials_in_keyring): Ditto

Please do not wrap line 35 in the middle of the name of the method.

> Tools/ChangeLog:37
> +        (test_do_not_use_stored_credentials.FakeUser): Mock the User class

This patch looks good. I noticed some very minor nits.

> Tools/Scripts/webkitpy/common/net/credentials_unittest.py:224
> +                return

This is OK as-is. It seems that we tend to use "pass" instead of an explicit return for functions that have an empty body.

[Dean Johnson]

Created attachment 256905 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 256905 [details]
Patch

Clearing flags on attachment: 256905

Committed r186927: <http://trac.webkit.org/changeset/186927>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.