Force asset manager preflight login - #5789
Conversation
b867df7 to
40f6063
Compare
40f6063 to
1c58c29
Compare
1c58c29 to
36be2d2
Compare
36be2d2 to
fa5dd00
Compare
fa5dd00 to
7f545c3
Compare
7f545c3 to
2092801
Compare
2092801 to
9563417
Compare
9563417 to
e61e602
Compare
e61e602 to
0903f8d
Compare
eYinka
changed the title
0903f8d to
a8e7d09
Compare
a8e7d09 to
b64f549
Compare
b64f549 to
abe84f0
Compare
abe84f0 to
dabd80c
Compare
dabd80c to
a400b37
Compare
a400b37 to
28cb7c3
Compare
28cb7c3 to
f944a16
Compare
ChrisBAshton
left a comment
ChrisBAshton
left a comment
There was a problem hiding this comment.
LGTM 👍 Some small comments below, which I can help with later if you don't get a chance to in the meantime.
f944a16 to
8c0d9db
Compare
8c0d9db to
fd628c5
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
fd628c5 to
7e0bd86
Compare
7e0bd86 to
2961910
Compare
2961910 to
d4d1e4c
Compare
d4d1e4c to
e40b779
Compare
ChrisBAshton
left a comment
ChrisBAshton
left a comment
There was a problem hiding this comment.
Looks good to me - nice one for keeping it scoped only to the draft stack, to minimise blast radius, whilst still keeping it modular and testable 🎉
Will get a 2i from a frontend dev
| </div> | ||
| <%= javascript_include_tag "asset-manager-session.js", integrity: false %> | ||
| <%= javascript_tag nonce: true do %> | ||
| new GOVUK.Modules.AssetManagerSession(document.getElementById("asset-manager-session")).init() |
There was a problem hiding this comment.
I don't think you need to manually initialise this code here (unless I've misunderstood how it works). All of our Javascript modules are started automatically when an element is given a data-module attribute e.g. in this case data-module="asset-manager-session". This would mean you can also remove the id from the element.
There was a problem hiding this comment.
Well, that was the initial implementation but it didn't solve our race-condition problem. The auto-init is scanned on DOMContentLoaded, which meant the images already fired and triggered a csrf problem before the module kicks in. This was the only other way I could think of.
There was a problem hiding this comment.
In that case I wouldn't write it as a module. I'd write it as a self executing block of code and put it in app/javascripts/lib or something. See https://github.com/alphagov/govuk_publishing_components/blob/main/app/assets/javascripts/govuk_publishing_components/lib/cookie-functions.js for an example.
There was a problem hiding this comment.
Thanks for the tip. That's all done now!
There was a problem hiding this comment.
Can I check that this is best place to put this new code? I think only 11 content types use this partial. It might be better in the main layouts (although you would have to duplicate it, so it would need to go in its own partial).
There was a problem hiding this comment.
Ooh, good shout - worth us double-checking this.
There was a problem hiding this comment.
I've now extracted it to its own partial and used across the 5 layouts in app/views/layouts
| * then call `new GOVUK.Modules.AssetManagerSession(element).init()`. | ||
| */ | ||
|
|
||
| /* istanbul ignore next */ |
There was a problem hiding this comment.
Were these ignore lines necessary for the code coverage?
There was a problem hiding this comment.
I think so. I just copied over the same convention from https://github.com/alphagov/frontend/blob/main/app/assets/javascripts/modules/sticky-element-container.js
There was a problem hiding this comment.
Non-blocking, but I'm not sure they're necessary. Might be worth removing them and seeing if the coverage still passes.
There was a problem hiding this comment.
Just got tripped by its absence in ci. See coverage error
e40b779 to
816fc4b
Compare
816fc4b to
6be574d
Compare
Each draft image on a page independently triggers its own Signon/Asset Manager OAuth handshake when there's no existing session. With more than one draft image, these handshakes race and clobber each other's CSRF state, so all but (at best) one image fail to load. In this commit, we load a single placeholder image from the draft-assets host, and once its request has settled - whether it succeeds or fails, we only care that the handshake has finished; we then retry every draft image already on the page. Added an `asset-manager-session.js` script that is inlined directly into the layout files via a `<script>` tag, and only on draft-stack pages, rather than shipped as part of the site's main JavaScript bundle. This is so that live pages are completely unaffected and don't load or run any of this code.
6be574d to
cd8d341
Compare
ChrisBAshton
left a comment
ChrisBAshton
left a comment
There was a problem hiding this comment.
Thanks for working through the comments! ⭐
| <%= javascript_include_tag "lib/asset-manager-session.js", integrity: false %> | ||
| <%= javascript_tag nonce: true do %> | ||
| window.addEventListener("load", function () { | ||
| GOVUK.warmUpAssetManagerSession('<%= placeholder_asset_url %>') |
5 participants
What
Load a placeholder image from the draft-assets host on every draft page. Once that request has settled, retry every draft
<img>already on the page with a cache-busting query param, so they get a fresh attempt against what should now be a warm Asset Manager session cookie.Why
As of alphagov/whitehall#11667, images can now be uploaded to the draft stack, meaning Frontend needs to be able to render the draft images in the page when on draft preview.
Unlike live images, draft images require the user to be authenticated (via Signon) and for an Asset Manager session to exist. When rendering a draft asset, the request for the asset from Asset Manager is redirected through a chain of calls including Authenticating Proxy and Signon, and if 1) the user is logged into Signon, and 2) they have permission to view the asset, the asset is then downloaded. Subsequent attempts to render any asset avoids the OAuth round-trip, as the user already has a warm Asset Manager session cookie by then.
The problem arises when more than one draft asset is included in a page, and a user is coming to the page cold. When rendering the page, every draft asset begins its own authentication journey, and the various callbacks and tokens will often arrive out of order, meaning no session takes hold. The images fail to load, and subsequent refreshes of the page rarely help (unless the order of requests sent and received happens in a very particular order). Visiting a draft asset in isolation, e.g. on a new tab, forces the session cookie and then subsequent attempts to render a page full of draft assets will succeed.
In #5783 we explored adding a preflight mechanism to Frontend whereby 'cold' users are redirected to a page containing a single asset, and then redirected back to their original page. Unfortunately our nginx config would require that the single-asset-page be added to an allowlist of pages that are allowed to set cookies, and even then, subsequent pages would not be allowed to read the cookie, which is then clobbered to an empty string:
https://github.com/alphagov/govuk-helm-charts/blob/3f15020fa87e412e8e67ac9c1e1339030c4ef440/charts/app-config/templates/router-nginx-config.tpl#L116-L149
The blast radius is small - we only inject this request on the draft stack, so only publishers will see it, and the real world ramifications are perhaps a very slight delay in the rendering of the page.
The asset chosen is the default lead image placeholder referenced throughout Frontend. It is the same asset used in a similar solution in Whitehall: alphagov/whitehall#11775
Jira: https://gov-uk.atlassian.net/browse/WHIT-3983