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

Prevent api responses that depend on current_user from being cached #43737

Merged
merged 2 commits into from Nov 22, 2021

Conversation

jamescodeorg
Copy link
Contributor

@jamescodeorg jamescodeorg commented Nov 19, 2021

During the caching-focused HoC bug bash, there were several reports of “stale” state being shown in the browser (e.g. page displaying as signed-in when the user had signed out). During the investigation, we discovered a general problem where the responses to asynchronous requests from the client, typically to /api endpoints were being cached by the browser and then being reused when the back button was hit (and possibly other scenarios).

I'm looking for feedback on the tradeoff between completeness of fix and risk. The most complete fix would be to prevent caching in the base classes and affect all APIs. The most targeted fix would be to change only the APIs where we know there to be an issue. This PR currently represents something in the middle -- I've audited our APIs to determine which ones are likely to return incorrect data because the response changes depending on current_user. (Note that it does not include APIs where current_user is only used to determine authorization.)

Links

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

Copy link
Member

@davidsbailey davidsbailey left a comment

Choose a reason for hiding this comment

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

Looks good to me. this seems like a good balance. On the one hand this seems safe because you are correcting bad behavior on these APIs. on the other hand, there's nothing to say that a change to the already-broken scenario of signing out and then pressing back could somehow get subtly worse with this change, e.g. progress now mismatched with sign in state leads to worse progress loss (purely hypothetical). Given the timeline I think shipping this makes sense, and then we can revert if needed.

for future reference, it is possible to configure an adhoc to use cloudfront simply by doing rake adhoc:cdn:start, and that could be used to verify this kind of a fix in the future. however this gets much less usage than normal adhoc and so there could be problems with that command which have cropped up since last time it was run successfully. so, I wouldn't hold up this PR on account of that.

@davidsbailey
Copy link
Member

Also, HUGE kudos to you both @jamescodeorg and @maureensturgeon for tracking this down! this seems really big, and could explain many previously unsolved mysteries.

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.

None yet

3 participants