Skip to content

ETT-310, ETT-309: Role switcher for resource sharing; fixes for cookie handling #119

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 5, 2025
Merged

ETT-310, ETT-309: Role switcher for resource sharing; fixes for cookie handling #119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2e4ce6e
ETT-310: Resource sharing role switch
aelkiss Jun 5, 2025
312c29b
ETT-309: Cookies fixes for local dev/testing
aelkiss Jun 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/js/components/AcceptableUseBanner/index.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,8 @@
// let isVisible = loggedIn && xtracking[key] != true;

function confirm() {
let expires = new Date();
expires.setDate(expires.getDate() + 90);
xtracking[key] = true;
cookieJar.setItem('HT.x', JSON.stringify(xtracking), expires, '/', HT.cookies_domain, true);
cookieJar.setItem('HT.x', JSON.stringify(xtracking), 90);
isVisible = false;
}

Expand Down
4 changes: 1 addition & 3 deletions src/js/components/AlertBanner/BannerMessage.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,7 @@
export function closeAlert() {
//if user has functional/preference cookies enabled, set a 14-day cookie to remember dismissed preference
if ($preferencesConsent === 'true') {
let expires = new Date();
expires.setDate(expires.getDate() + 14);
cookieJar.setItem(`HT-alert-${id}`, 'dismissed', expires, '/', HT.cookies_domain, true);
cookieJar.setItem(`HT-alert-${id}`, 'dismissed', 14);
isVisible = false;
}
//reset focus to the main element once the banner is removed from the DOM
Expand Down
3 changes: 2 additions & 1 deletion src/js/components/Navbar/index.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,11 @@
export let compact = false;
export let userNavigation = true;

const switchableRoles = ['enhancedTextProxy', 'totalAccess'];
const switchableRoles = ['enhancedTextProxy', 'totalAccess','resourceSharing'];
const switchableRolesLabels = {};
switchableRolesLabels['enhancedTextProxy'] = 'ATRS';
switchableRolesLabels['totalAccess'] = 'CAA';
switchableRolesLabels['resourceSharing'] = 'Resource Sharing';

function toggleSearch() {
searchOpen = !searchOpen;
Expand Down
51 changes: 19 additions & 32 deletions src/js/lib/cookies.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,43 +31,34 @@ export const docCookies = {
) || null
);
},
setItem: function (sKey, sValue, vEnd, sPath, sDomain, bSecure) {
setItem: function (sKey, sValue, duration = 365) {
Copy link
Member Author

@aelkiss aelkiss Jun 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

if (!sKey || /^(?:expires|max\-age|path|domain|secure)$/i.test(sKey)) {
return false;
}
var sExpires = '';
if (vEnd) {
switch (vEnd.constructor) {
case Number:
sExpires = vEnd === Infinity ? '; expires=Fri, 31 Dec 9999 23:59:59 GMT' : '; max-age=' + vEnd;
break;
case String:
sExpires = '; expires=' + vEnd;
break;
case Date:
sExpires = '; expires=' + vEnd.toUTCString();
break;
}
}
var expires = new Date();
expires.setMonth(expires.getDate() + duration);

var sExpires = '; expires=' + expires.toUTCString();
document.cookie =
encodeURIComponent(sKey) +
'=' +
encodeURIComponent(sValue) +
sExpires +
(sDomain ? '; domain=' + sDomain : '') +
(sPath ? '; path=' + sPath : '') +
(bSecure ? '; secure' : '');
(HT.cookies_domain ? '; domain=' + HT.cookies_domain : '') +
Copy link
Member Author

Choose a reason for hiding this comment

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

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)

('; path=/') +
(HT.secure_cookies ? '; secure' : '') +
( '; SameSite=Lax')
Copy link
Member Author

@aelkiss aelkiss Jun 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link

@moseshll moseshll Jun 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

return true;
},
removeItem: function (sKey, sPath, sDomain) {
removeItem: function (sKey) {
if (!this.hasItem(sKey)) {
return false;
}
document.cookie =
encodeURIComponent(sKey) +
'=; expires=Thu, 01 Jan 1970 00:00:00 GMT' +
(sDomain ? '; domain=' + sDomain : '') +
(sPath ? '; path=' + sPath : '');
(HT.cookies_domain ? '; domain=' + HT.cookies_domain : '') +
('; path=/')
return true;
},
hasItem: function (sKey) {
Expand All @@ -89,41 +80,37 @@ export const docCookies = {
},
};

let expires = new Date();
expires.setMonth(expires.getMonth() + 12);

function setCookieConsentSeen() {
docCookies.setItem('HT-cookie-banner-seen', 'true', expires, '/', HT.cookies_domain, true);
docCookies.setItem('HT-cookie-banner-seen', 'true');
cookieConsentSeen.set('true');
}
function setTrackingAllowedCookie() {
docCookies.setItem('HT-tracking-cookie-consent', 'true', expires, '/', HT.cookies_domain, true);
docCookies.setItem('HT-tracking-cookie-consent', 'true');
trackingConsent.set('true');
return true;
}

function setTrackingDisallowedCookie() {
docCookies.setItem('HT-tracking-cookie-consent', 'false', expires, '/', HT.cookies_domain, true);
docCookies.setItem('HT-tracking-cookie-consent', 'false');
trackingConsent.set('false');
}

function setMarketingAllowedCookie() {
docCookies.setItem('HT-marketing-cookie-consent', 'true', expires, '/', HT.cookies_domain, true);
docCookies.setItem('HT-marketing-cookie-consent', 'true');
marketingConsent.set('true');
}

function setMarketingDisallowedCookie() {
docCookies.setItem('HT-marketing-cookie-consent', 'false', expires, '/', HT.cookies_domain, true);
docCookies.setItem('HT-marketing-cookie-consent', 'false');
marketingConsent.set('false');
}

function setPreferencesAllowedCookie() {
docCookies.setItem('HT-preferences-cookie-consent', 'true', expires, '/', HT.cookies_domain, true);
docCookies.setItem('HT-preferences-cookie-consent', 'true');
preferencesConsent.set('true');
}

function setPreferencesDisallowedCookie() {
docCookies.setItem('HT-preferences-cookie-consent', 'false', expires, '/', HT.cookies_domain, true);
docCookies.setItem('HT-preferences-cookie-consent', 'false');
preferencesConsent.set('false');
}

Expand Down
18 changes: 8 additions & 10 deletions src/js/lib/cookies.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { afterEach, afterAll, describe, it, expect, test, vi } from 'vitest'
import { beforeEach, afterAll, describe, it, expect, test, vi } from 'vitest'
import { docCookies, setSelectedConsent} from './cookies'
import { allowTracking, allowMarketing } from './store'
import { get } from 'svelte/store';
Expand All @@ -15,7 +15,6 @@ HT.cookies_domain = '.hathitrust.org'
window.location.host = "www.hathitrust.org"
window.location.protocol = "https:"


const getItemSpy = vi.spyOn(docCookies, 'getItem')
const setItemSpy = vi.spyOn(docCookies, 'setItem')
const remoteItemSpy = vi.spyOn(docCookies, 'removeItem')
Expand All @@ -26,14 +25,13 @@ it('should not be undefined', () => {
})
})
describe('docCookies', () => {
afterEach(() => {
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(() => {
Copy link
Member Author

Choose a reason for hiding this comment

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

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)

document.cookie = "COOKIE=; expires=Thu, 01 Jan 1970 00:00:00 GMT; path=/"
})
describe('getItem', () => {
document.cookie="COOKIE=true"

test('gets value of cookie from document.cookie', () => {
document.cookie="COOKIE=true; path=/"
expect(docCookies.getItem('COOKIE')).toStrictEqual('true');
expect(getItemSpy).toHaveBeenCalled()
})
Expand All @@ -44,7 +42,7 @@ describe('docCookies', () => {
})
describe('setItem', () => {
test('sets cookie with key and value', () => {
expect(docCookies.setItem('COOKIE', 'cookie', '', '', '')).toBe(true)
expect(docCookies.setItem('COOKIE', 'cookie')).toBe(true)
expect(document.cookie).toBeTruthy()
expect(document.cookie).toContain("COOKIE=cookie")
expect(setItemSpy).toHaveBeenCalled()
Expand All @@ -54,7 +52,7 @@ describe('docCookies', () => {
test('removes cookie with given name/key', () => {
//set up a cookie
expect(document.cookie).toEqual('')
document.cookie = "COOKIE=cookie"
document.cookie = "COOKIE=cookie; path=/"
expect(document.cookie).toContain("COOKIE=cookie")

//now test removeItem
Expand Down Expand Up @@ -84,4 +82,4 @@ describe('setSelectedConsent', () => {
expect(document.cookie).toContain('HT-marketing-cookie-consent=false')
})

})
})
8 changes: 6 additions & 2 deletions src/js/lib/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,20 +28,24 @@ function setDomains() {
HT.catalog_domain = 'catalog.hathitrust.org';
HT.www_domain = 'www.hathitrust.org';
HT.cookies_domain = '.hathitrust.org';
HT.secure_cookies = true;

var hostname = location.hostname;
HT.is_dev =
hostname != 'www.hathitrust.org' && hostname != 'catalog.hathitrust.org' && hostname != 'babel.hathitrust.org';
if (HT.is_dev) {
var prefix = hostname.split('.')[0];
console.log('-- main setting hostname', prefix, hostname);
if (prefix == 'localhost' || prefix == 'apache-test') {
HT.secure_cookies = (location.protocol == 'https');
// no dots in hostname -- e.g. localhost or docker container
if (hostname == prefix) {
if (location.port) {
hostname += ':' + location.port;
}
HT.service_domain = hostname;
HT.catalog_domain = hostname;
HT.www_domain = hostname;
HT.cookies_domain = 'localhost';
HT.cookies_domain = null;
} else if (hostname.indexOf('phiredevelopment') > -1) {
// shameless green
HT.www_domain = hostname;
Expand Down
Loading