Merge lp:~bac/launchpadlib/keyring into lp:launchpadlib

Proposed by Brad Crittenden
Status: Merged
Approved by: Gary Poster
Approved revision: 129
Merged at revision: 127
Proposed branch: lp:~bac/launchpadlib/keyring
Merge into: lp:launchpadlib
Diff against target: 0 lines
To merge this branch: bzr merge lp:~bac/launchpadlib/keyring
Reviewer Review Type Date Requested Status
Gary Poster Approve
Review via email: mp+84495@code.launchpad.net

Commit message

Move the base64 encoding of keyring passwords to the KeyringCredentialStore where it belongs. Set a marker so we definitely know it is encoded.

Description of the change

Move the base64 encoding out of the Credential serialization and into the KeyringCredentialStore where it really belongs.

Be more defensive when converting stored credentials and require user to re-authorize rather than giving them an error.

To post a comment you must log in.
Revision history for this message
Gary Poster (gary) wrote :

Nice, Brad. Thank you.

We discussed this on a phone call and IRC. The discussion had two points.

1) Why return None if we can't decode, forcing the user to get a new authorization? Why not try the undecoded string, in case this is some kind of legacy issue? Answer: deleting or changing broken passwords is annoying. It is better to simply make the user follow a known workflow to get a new authorization than to make them try to figure out how to delete broken passwords in the key. There's a nice comment for that now.

2) We talked about the marker, and whether we needed any further checks. I was convinced that this is fine. In particular, the "consumer_key" string was at the beginning, so checking for <B64> at the beginning effectively subsumes that check. We also talked about whether it was important that the marker string not be base 64 itself, and agreed that it was not important.

A small additional point: if you are going to mess with whitespace, in line 80 of the diff, don't we want a space after the colon?

  headers = {'Referer': web_root}

Do what you will. :-)

Gary

review: Approve
lp:~bac/launchpadlib/keyring updated
130. By Brad Crittenden

Fixed lint

Preview Diff

Empty

Subscribers

People subscribed via source and target branches