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.
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.
Created attachment 256794 [details] Patch
Created attachment 256795 [details] Patch
Both attachments are the same, one was just testing my fix.
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.
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.
Created attachment 256846 [details] Patch
Comment on attachment 256846 [details] Patch Can we write a test for this change?
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 ':'.
Created attachment 256877 [details] Patch
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.
Created attachment 256905 [details] Patch
Comment on attachment 256905 [details] Patch Clearing flags on attachment: 256905 Committed r186927: <http://trac.webkit.org/changeset/186927>
All reviewed patches have been landed. Closing bug.