Merge lp://staging/~bac/launchpadlib/keyring into lp://staging/launchpadlib
Proposed by
Brad Crittenden
| Status: | Merged | ||||
|---|---|---|---|---|---|
| Approved by: | Gary Poster | ||||
| Approved revision: | 129 | ||||
| Merged at revision: | 127 | ||||
| Proposed branch: | lp://staging/~bac/launchpadlib/keyring | ||||
| Merge into: | lp://staging/launchpadlib | ||||
| Diff against target: | 0 lines | ||||
| To merge this branch: | bzr merge lp://staging/~bac/launchpadlib/keyring | ||||
| Related bugs: |
|
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Gary Poster | Approve | ||
|
Review via email:
|
|||
Commit message
Move the base64 encoding of keyring passwords to the KeyringCredenti
Description of the change
Move the base64 encoding out of the Credential serialization and into the KeyringCredenti
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.
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