Canonical SSO provider

Merge lp://staging/~facundo/canonical-identity-provider/refuse-refresh-password-changed into lp://staging/canonical-identity-provider/release

  1. refuse-refresh-password-changed
  2. Merge into trunk
Proposed by Facundo Batista
Status: Merged
Approved by: Facundo Batista
Approved revision: no longer in the source branch.
Merged at revision: 1418
Proposed branch: lp://staging/~facundo/canonical-identity-provider/refuse-refresh-password-changed
Merge into: lp://staging/canonical-identity-provider/release
Prerequisite: lp://staging/~facundo/canonical-identity-provider/add-password-changed-timestamp
Diff against target: 74 lines (+29/-3)
2 files modified
src/identityprovider/auth.py (+10/-3)
src/identityprovider/tests/test_auth.py (+19/-0)
To merge this branch: bzr merge lp://staging/~facundo/canonical-identity-provider/refuse-refresh-password-changed
Reviewer Review Type Date Requested Status
Matias Bordese (community) Approve
Daniel Manrique (community) Approve
Review via email: mp+289938@code.staging.launchpad.net

Commit message

Check if account's password changed before refreshing the macaroon.

Description of the change

Check if account's password changed before refreshing the macaroon.

To post a comment you must log in.
Revision history for this message
Daniel Manrique (roadmr) wrote :

The test, I think, is not really doing what we expect it to do. See longish comment below and I think we have some fixing to do :(

review: Needs Fixing
Revision history for this message
Matias Bordese (matiasb) :
Revision history for this message
Facundo Batista (facundo) wrote :

Answered a comment, pushed the fix for the other.

Revision history for this message
Daniel Manrique (roadmr) wrote :

Looks OK to me, Matías suggested another way to achieve the None value using update(), your choice on which one to use.

I'd still prefer a more explicit check than "just run this and ensure it didn't exception out" but am OK if you think this is reasonable.

I'll +1 in case you want to merge as is, or if you want to apply the update() change.

review: Approve
Revision history for this message
Daniel Manrique (roadmr) wrote :

Thanks for the assert.

review: Approve
Revision history for this message
Matias Bordese (matiasb) wrote :

LGTM

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.