Bug 33039 – [bzt] Bugzilla-tool dies with an error if it cannot find a keychain entry for bugs.webkit.org

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Daniel Bates]

Under Mac OS X, bugzilla-tool tries to query the keychain and Security framework (via /usr/sbin/security) for an internet-password entry for bugs.webkit.org so that it can use it to login to bugs.webkit.org. However, if no such entry exists then bugzilla-tool dies with an error:

Failed to run "['/usr/bin/security', 'find-internet-password', '-g', '-s', 'bugs.webkit.org']" exit_code: 44
security: SecKeychainSearchCopyNext: The specified item could not be found in the keychain.

Failed to run "['/usr/bin/security', 'find-internet-password', '-g', '-s', 'bugs.webkit.org']" exit_code: 44

[Daniel Bates]

Created attachment 45635 [details]
Patch with unit test

[Adam Barth]

Comment on attachment 45635 [details]
Patch with unit test

Thanks Dan.  It seems kind of silly to have that text twice, but I guess that makes sense.

[Eric Seidel (no email)]

Comment on attachment 45635 [details]
Patch with unit test

The unit test shoudl just use Credentials.keychain_entry_not_found instead of re-defining it, no?

In reality, what we should do is that we should prompt the user for username and password (which I think might be broken?) and then store those in the keychain for them.  See 32728.

[Daniel Bates]

(In reply to comment #3)
> (From update of attachment 45635 [details])
> The unit test shoudl just use Credentials.keychain_entry_not_found instead of
> re-defining it, no?
Yes, will use Credentials.keychain_entry_not_found instead of redefining. Will fix before I land.

[Eric Seidel (no email)]

Attachment 45635 [details] was posted by a committer and has review+, assigning to Daniel Bates for commit.

[Daniel Bates]

Committed in <http://trac.webkit.org/changeset/52643>.

[Daniel Bates]

The patch <https://bugs.webkit.org/attachment.cgi?id=45635> did not fix this issue.

The following is the terminal output when I commited bug #33041:

noric:WebKit dbates$ WebKitTools/Scripts/bugzilla-tool land-diff 33041 --no-build
Fetching: https://bugs.webkit.org/show_bug.cgi?id=33041&ctype=xml
Guessing "Eric Seidel" as reviewer from attachment 45639 [details] on bug 33041.
Parsing ChangeLog: WebKitTools/ChangeLog
Updating bug 33041
Reading Keychain for bugs.webkit.org account and password.  Click "Allow" to continue...
Failed to run "['/usr/bin/security', 'find-internet-password', '-g', '-s', 'bugs.webkit.org']" exit_code: 44
security: SecKeychainSearchCopyNext: The specified item could not be found in the keychain.

Failed to run "['/usr/bin/security', 'find-internet-password', '-g', '-s', 'bugs.webkit.org']" exit_code: 44

[Daniel Bates]

Comment on attachment 45635 [details]
Patch with unit test

Clearing review flag.

[Daniel Bates]

Created attachment 45644 [details]
Patch with unit test

Second attempt to fix this.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45644 [details] without any errors.

[Daniel Bates]

Created attachment 45645 [details]
Patch with unit test

Minor correction. Removed ';' from method CredentialsTest.test_security_output_parse_entry_not_found

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45645 [details] without any errors.

[Eric Seidel (no email)]

Comment on attachment 45645 [details]
Patch with unit test

I think that run_command actually throws an OSError if it can't find the command.

So you're only going to get a script error, with exit code 44 when it can't find the keychain item.  You only get ScriptErrors when the command runs but exits non-zero AFAIK.

Why does your test work?  And does it work on non mac?

[Daniel Bates]

(In reply to comment #13)
> (From update of attachment 45645 [details])
> I think that run_command actually throws an OSError if it can't find the
> command.

No, it longer does because of the patch for bug #32896. We now catch OSErrors, by line 126 <http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/executive.py?rev=52587#L126>. 

> So you're only going to get a script error, with exit code 44 when it can't
> find the keychain item.  You only get ScriptErrors when the command runs but
> exits non-zero AFAIK.

Should we then test by output like I originally did? I changed it because the output message could change but I think it is less likely for the error code to change.

> Why does your test work?  And does it work on non mac?

What do you mean? No, I haven't tested this under Windows, yet (will do).

[Eric Seidel (no email)]

Comment on attachment 45645 [details]
Patch with unit test

OK.  So I now understand that the OSError will be caught.  We should capture and ASSERT output in the unit test.  Seems silly to capture output if we're not going to look at it.  We can point the tool to /does/not/exist and try that, and also have a mac-only test which searches for a non-existant keychain entry?

[Daniel Bates]

Comment on attachment 45645 [details]
Patch with unit test

Clearing review patch while I work on this.

[Daniel Bates]

(In reply to comment #15)
> (From update of attachment 45645 [details])
> OK.  So I now understand that the OSError will be caught.  We should capture
> and ASSERT output in the unit test.  Seems silly to capture output if we're not
> going to look at it. 

This is redundant since we already capture and assert the output in the test method _assert_security_call, <http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/credentials_unittest.py?rev=52644#L85>.

>We can point the tool to /does/not/exist and try that,

I don't follow.  Are you implying to test for when the security tool does not exist? If so, we already have a generic test when the run_command is passed a command that does not exist, see methods est_run_command_with_bad_command_check_return_code and test_run_command_with_bad_command_check_calls_error_handler in file executive_unittest.py <http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/executive_unittest.py?rev=52587.

> and also have a mac-only test which searches for a non-existant keychain entry?

I can add a check to the test to only run if the platform is Mac. Notice, if we are not on a Mac then we never call the method Credentials._run_security_tool since the calling method Credentials._credentials_from_keychain returns [username, None] by line 81 <http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/credentials.py?rev=52667#L81>

[Daniel Bates]

Created attachment 45675 [details]
Patch with unit tests

Modified CredentialTest.test_security_output_parse_entry_not_found to return (without running the security tool) if we are on a non-Mac, since the security tool is Mac-specific.

[Eric Seidel (no email)]

Comment on attachment 45675 [details]
Patch with unit tests

Hum.  Seems Credentials already has an is_mac() test, no?  It's probably fine to duplicated the code in the unit test too, it is only one-line. :)

Remind me why we're discarding the outputCapture output again?  Seems we should have a comment there to explain why.

[Daniel Bates]

(In reply to comment #19)
> (From update of attachment 45675 [details])
> Hum.  Seems Credentials already has an is_mac() test, no?  It's probably fine
> to duplicated the code in the unit test too, it is only one-line. :)

I'll change it to call Credentials._is_mac_os_x().

I didn't think it was good practice for a unit test to depend on the code it is testing.

> Remind me why we're discarding the outputCapture output again?  Seems we should
> have a comment there to explain why.

I can add a comment. We should not need to assert the output captured because we already did that in the method _assert_security_call, <http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/credentials_unittest.py?rev=52644#L80>. See line 85 for the assertion.

I could assert the output, but then if we change it in the future, a person must now update two unit tests: Credentials._assert_security_call() and Credentials.test_security_output_parse_entry_not_found().

[Daniel Bates]

Created attachment 45752 [details]
Patch with unit test

Modified the unit test method CredentialsTest.test_security_output_parse_entry_not_found() to call Credentials._is_mac_os_x() instead of querying for the platform info. directly. Also, added a comment to explain why we are ignoring the captured output.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45752 [details] without any errors.

[Eric Seidel (no email)]

Is this the same as bug 32892?

[Eric Seidel (no email)]

Comment on attachment 45752 [details]
Patch with unit test

LGTM.

[Daniel Bates]

*** Bug 32892 has been marked as a duplicate of this bug. ***

[Daniel Bates]

Committed patch <https://bugs.webkit.org/attachment.cgi?id=45752> in <http://trac.webkit.org/changeset/52710>.

Committed unreviewed fix in <http://trac.webkit.org/changeset/52712>.

[Daniel Bates]

Confirmed that the unreviewed fix works when I committed bug #33097.