Bug 33039 - [bzt] Bugzilla-tool dies with an error if it cannot find a keychain entry for bugs.webkit.org
Summary: [bzt] Bugzilla-tool dies with an error if it cannot find a keychain entry for...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.6
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: PlatformOnly
: 32892 (view as bug list)
Depends on:
Blocks: 32728
  Show dependency treegraph
 
Reported: 2009-12-29 20:38 PST by Daniel Bates
Modified: 2010-01-04 13:27 PST (History)
3 users (show)

See Also:


Attachments
Patch with unit test (3.38 KB, patch)
2009-12-29 20:39 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch with unit test (3.39 KB, patch)
2009-12-29 23:13 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch with unit test (3.39 KB, patch)
2009-12-29 23:18 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch with unit tests (3.61 KB, patch)
2009-12-30 11:26 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch with unit test (3.64 KB, patch)
2010-01-02 14:32 PST, Daniel Bates
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2009-12-29 20:38:34 PST
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
Comment 1 Daniel Bates 2009-12-29 20:39:53 PST
Created attachment 45635 [details]
Patch with unit test
Comment 2 Adam Barth 2009-12-29 20:42:49 PST
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.
Comment 3 Eric Seidel (no email) 2009-12-29 20:44:46 PST
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.
Comment 4 Daniel Bates 2009-12-29 20:49:50 PST
(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.
Comment 5 Eric Seidel (no email) 2009-12-29 20:49:57 PST
Attachment 45635 [details] was posted by a committer and has review+, assigning to Daniel Bates for commit.
Comment 6 Daniel Bates 2009-12-29 22:05:27 PST
Committed in <http://trac.webkit.org/changeset/52643>.
Comment 7 Daniel Bates 2009-12-29 22:33:45 PST
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
Comment 8 Daniel Bates 2009-12-29 22:37:38 PST
Comment on attachment 45635 [details]
Patch with unit test

Clearing review flag.
Comment 9 Daniel Bates 2009-12-29 23:13:28 PST
Created attachment 45644 [details]
Patch with unit test

Second attempt to fix this.
Comment 10 WebKit Review Bot 2009-12-29 23:16:30 PST
style-queue ran check-webkit-style on attachment 45644 [details] without any errors.
Comment 11 Daniel Bates 2009-12-29 23:18:32 PST
Created attachment 45645 [details]
Patch with unit test

Minor correction. Removed ';' from method CredentialsTest.test_security_output_parse_entry_not_found
Comment 12 WebKit Review Bot 2009-12-29 23:21:54 PST
style-queue ran check-webkit-style on attachment 45645 [details] without any errors.
Comment 13 Eric Seidel (no email) 2009-12-29 23:25:49 PST
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?
Comment 14 Daniel Bates 2009-12-29 23:32:18 PST
(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).
Comment 15 Eric Seidel (no email) 2009-12-29 23:42:49 PST
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?
Comment 16 Daniel Bates 2009-12-30 00:00:54 PST
Comment on attachment 45645 [details]
Patch with unit test

Clearing review patch while I work on this.
Comment 17 Daniel Bates 2009-12-30 11:09:09 PST
(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>
Comment 18 Daniel Bates 2009-12-30 11:26:48 PST
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.
Comment 19 Eric Seidel (no email) 2010-01-02 12:17:47 PST
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.
Comment 20 Daniel Bates 2010-01-02 13:24:55 PST
(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().
Comment 21 Daniel Bates 2010-01-02 14:32:37 PST
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.
Comment 22 WebKit Review Bot 2010-01-02 14:33:37 PST
style-queue ran check-webkit-style on attachment 45752 [details] without any errors.
Comment 23 Eric Seidel (no email) 2010-01-03 19:08:03 PST
Is this the same as bug 32892?
Comment 24 Eric Seidel (no email) 2010-01-03 19:08:40 PST
Comment on attachment 45752 [details]
Patch with unit test

LGTM.
Comment 25 Daniel Bates 2010-01-04 00:45:32 PST
*** Bug 32892 has been marked as a duplicate of this bug. ***
Comment 26 Daniel Bates 2010-01-04 00:47:35 PST
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>.
Comment 27 Daniel Bates 2010-01-04 00:48:20 PST
Confirmed that the unreviewed fix works when I committed bug #33097.