Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(profile-server): ensure profileChangedAt is included in assertion JWT for cache invalidation #2482

Merged
merged 1 commit into from
Sep 12, 2019
Merged

fix(profile-server): ensure profileChangedAt is included in assertion JWT for cache invalidation #2482

merged 1 commit into from
Sep 12, 2019

Conversation

lmorchard
Copy link
Contributor

issue #2206

@rfk
Copy link
Contributor

rfk commented Sep 12, 2019

Nice catch! I expect you will need to update the tests in test/local/oauthdb.js for completeness.

@lmorchard
Copy link
Contributor Author

Okay, so I'm digging around in bits that I don't quite fully understand here.

I noticed that profile-server is looking for creds.profile_changed_at to decide whether to invalidate cached profile data. However, when I threw in some log statements, it appeared that profile_changed_at is always missing from creds, so it seems like this cache invalidation never happens.

Tracing clumsily back through the code with log statements, it looks like fxa-profileChangedAt isn't added in makeAssertionJWT. I added it, and profile_changed_at now appears in creds in profile-server, and the cache invalidation actually happens now.

This seems to simple of a fix for issue #2206, but maybe this is it? Could use some sanity checks as to what I just did here :)

@lmorchard
Copy link
Contributor Author

Nice catch! I expect you will need to update the tests in test/local/oauthdb.js for completeness.

Oh, yeah, will need to update the tests... I wanted to get this out in PR form if only to see if I'm completely barking up the wrong tree and mucking with bits beyond my ken :)

@rfk
Copy link
Contributor

rfk commented Sep 12, 2019

I think this probably explains it TBH; the kind of "backdoor" assertion that's being used here to talk directly to oauth-server has diverged from the schema of assertions generated by ./lib/signer.js. The fix here is 100% correct and required.

@rfk
Copy link
Contributor

rfk commented Sep 12, 2019

(A broader follow-up might look into refactoring to avoid code duplicating between the code here and the code in ./lib/signer.js)

@lmorchard
Copy link
Contributor Author

Quick update to add the profileChangedAt claim in oauthdb test

@lmorchard lmorchard requested a review from a team September 12, 2019 15:16
@rlr
Copy link
Contributor

rlr commented Sep 12, 2019

amazing \o/

Copy link
Contributor

@rfk rfk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An r+ for the PR, and an A+ for the detective work; great find!

@lmorchard lmorchard merged commit c8e3599 into mozilla:master Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants