ETT-310, ETT-309: Role switcher for resource sharing; fixes for cookie handling #119
Conversation
aelkiss
force-pushed
the
ETT-310-rs-role-switch
branch
from
d8297d0 to
1de9c93
Compare
aelkiss
force-pushed
the
ETT-310-rs-role-switch
branch
2 times, most recently
from
fc17034 to
1649f9a
Compare
There was a problem hiding this comment.
@carylwyatt Are we still using this component at all (given that we now have the cookie consent banner) or could we remove it? I couldn't find any references to it but I might not be looking in the right way
There was a problem hiding this comment.
yup looks like you removed it in the svelte 5 branch anyway
| ); | ||
| }, | ||
| setItem: function (sKey, sValue, vEnd, sPath, sDomain, bSecure) { | ||
| setItem: function (sKey, sValue, duration = 365) { |
There was a problem hiding this comment.
My thought here is that a default duration of 365 days isn't exactly the same as 12 months (what we were doing before) but it's probably good enough and simplifies calling setItem
| (sDomain ? '; domain=' + sDomain : '') + | ||
| (sPath ? '; path=' + sPath : '') + | ||
| (bSecure ? '; secure' : ''); | ||
| (HT.cookies_domain ? '; domain=' + HT.cookies_domain : '') + |
There was a problem hiding this comment.
We don't have any use cases for setting any of these parameters differently by cookie right now, so I removed this as a parameter. If we need it in the future we could add it back (perhaps with a default value)
| (HT.cookies_domain ? '; domain=' + HT.cookies_domain : '') + | ||
| ('; path=/') + | ||
| (HT.secure_cookies ? '; secure' : '') + | ||
| ( '; SameSite=Lax') |
There was a problem hiding this comment.
Adding SameSite=Lax to all cookies mitigates warnings in Firefox that this will be the default in the future.
@moseshll The only case I can think of where we could potentially be relying on 3rd party cookies is for the embedding in CRMS, but that's still all from the same domain, so it should all work.... right?
Are there any other cases where we could potentially rely on third-party cookies working? I feel like in general these days many browsers/people block them anyway, so we probably would have found out about this?
There was a problem hiding this comment.
CRMS only uses cookies to remember a few pagination/view settings in its own pages. Shouldn't be any bleed between that and mdp apps.
| document.cookie = "COOKIE=true;expires=Thu, 01 Jan 1970 00:00:00 GMT" | ||
| document.cookie = "COOKIE=cookie;expires=Thu, 01 Jan 1970 00:00:00 GMT" | ||
| }) | ||
| beforeEach(() => { |
There was a problem hiding this comment.
Doing this in a before block to ensure the state before we run the test seems generally more idiomatic to me.
We also need to add the path since now we are setting that by default on all cookies (and things might not always work as expected for cookies without paths anyway)
aelkiss
force-pushed
the
ETT-310-rs-role-switch
branch
from
1649f9a to
b1c4f64
Compare
* If running locally, don't set the cookie domain at all (some browsers reject such cookies) * If running in dev, don't set "secure cookies" unless we're using https * Refactor cookie settings to eliminate duplicative parameters/unused logic
aelkiss
force-pushed
the
ETT-310-rs-role-switch
branch
from
b1c4f64 to
312c29b
Compare
3 participants
See comments on individual PRs. For testing with hathitrust/babel#113