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

get/set local/session storage/cookies through cmp #866

Draft
wants to merge 21 commits into
base: main
Choose a base branch
from
Draft

Conversation

vlkerag
Copy link
Contributor

@vlkerag vlkerag commented Dec 15, 2023

What does this change?

Adds a wrapper around get/set local/session strorage/cookies. Provides as list of available consent use cases (e.g. 'Targeted Advertising' or 'Essential') that users can specify and ensure that the right consent for the use-case has been granted by our reader.

Why?

To ensure compliance with consent and avoid many different locations in our code that check for consent for their use-cases which makes it hard to ensure compliance for example in case of a framework change.
Need to check consent on 'get' as the same cookie/storage item could be used for several different use-cases and require different consent. Obviously need to check on 'set'.
Privacy by Design: https://ico.org.uk/for-organisations/uk-gdpr-guidance-and-resources/accountability-and-governance/guide-to-accountability-and-governance/accountability-and-governance/data-protection-by-design-and-default/
https://ico.org.uk/for-organisations/uk-gdpr-guidance-and-resources/designing-products-that-protect-privacy/privacy-in-the-product-design-lifecycle/

Requirements and possible options

Consent requirements:

  • Consent needs to be checked on each pageview as the reader could have changed their settings from one pageview to the next. Hence consent needs to be established for each pageview.
  • Consent is use-case based: the reader might have given consent for the same data point for one use-case but not another.

Use-cases for storage/cookies:

  • Reader-facing: always requires consent. "Essential" use cases also have to check consent through the cmp and not make assumptions. -> All reader-facing function have to become asynchronous as consent is inherently asynchronous. There is no other way.
  • Internal tools: may not require any consent

How is consent obtained/checked:

  • The cmp provides an onConsent function that obtains the consentState
    ? What for server-side applications? --> Perhaps out of scope for now and next steps.
    ? Is there a use-case where consent is obtained not through the cmp? Could not currently identify such a use-case.

Architectural considerations

  1. Asynchronous: Consent is inherently asynchronous but currently interactions with cookies/storage is synchronous. Hence introducing consent requires all users of cookie/storage to become asynchronous:
    • Use-case that doe not require consent (such as internal tools) might not want to become asynchronous
    • The change from synchronous to asynchronous might be a large change for some existing use-cases and hence make it harder to adopt consent in this way.
    • Some apps might not like the asynchronous side-effects and prefer to obtain the consent state separately and then pass the consent state into the storage/cookie functions.
  2. Memory: importing the cmp adds additional size.
  3. Run-time: checking for consent on each read/write might add run-time
  4. If possible we'd like to avoid several functions/libraries providing interaction with cookies/storage so that users cannot inadvertently use the wrong one.
  5. T&C team doe not really have an interest in the actual cookie/storage interaction, just that consent is given for that interaction and use-case.

Possible architectural options:

  1. Provide one function interacting with the cookies/storage and taking care of consent. That function gets the consent and then accesses and returns the data (or not, depending on consent).
  • Ensures consent is checked timely and access is only granted with the right consent
  • Only one interface to cookies/local storage
  • Always requires the cmp even if consent is not required (internal apps)
  • Asynchronous for all even if consent is not required (internal apps), requires all to move to async functions.
    ? Where could this be located? Perhaps in the @guardian/libs: Would required libs to import cmp. cmp depends on libs (circular dependency?) Could it be in a separate package, something like @guardian/storage?

2a) "Self-certification" of consent and one function to interact with cookies/storage: Users would first check if there is consent for their use case and then certify that they have done so by passing in a boolean.

  • Only one interface to cookies/local storage
  • Cookie/Storage interaction can be synchronous
  • cmp not required if use-case does not require consent (internal tools)
  • Separation of functionality: consent check is provided by cmp and is separate from getting the data
  • It is harder to ensure that consent is really checked on each pageview.
    Reminder: all reader-facing functions have to be asynchronous as consent is required.
    ? Where could this be located? The consent check could live in cmp and the cookie/storage interaction in
    @guardian/journalism-tools-stream

2b) We also discussed that instead of passing a self-certify boolean to pass a function that would do the consent check and return a boolean.

  • Only one interface to cookies/local storage
  • Cookie/Storage interaction can be synchronous
  • cmp not required if use-case does not require consent (internal tools)
    Reminder: all reader-facing functions have to be asynchronous as consent is required.
    ? Is that really different from passing a boolean? Perhaps it makes it easier to use the cmp in a timely way while still allowing use-cases that do not require cmp to pass in 'true'.
    ? Where could this be located? Perhaps in the @guardian/libs: Would required libs to import cmp. cmp depends on libs (risk of circular dependency would be broken through dependency injection)
  1. Provide a mix of both options above:
    -> One async functions that does all in one: get consent and the data.
    -> One sync function that requires self-certification.
  • Most flexible
  • More than on interface to cookies/local storage
    ? Where could this be located? Perhaps in the @guardian/libs: Would required libs to import cmp. cmp depends on libs (circular dependency?) Could it be in a separate package, something like @guardian/storage?
  1. cmp does the consentState magic and users call a one-liner from cmp as an async call: await storage.local.get(useCase, key)

  2. users get the consentState from cmp (async) and pass it into a storage/cookie interaction function in cmp so that @guardian/libs do not depend on cmp: const consentState = await onConsent(); storage.local.get(useCase, consentState, key)

How do we get there?

The path will depend on the architectural choice. Using the linter to flag to users the correct usage is probably going to be helpful. Give a grace period to migrate with linter warnings, then upgrade to linter errors.

  1. Ensure that everyone migrates to the @guardian/libs version of storage and cookies by making any storage/cookie interaction not through the @guardian/libs a lint error in @guardian/lint: consent csnx#1041
  2. One difference between @guardian/libs and this PR is the fact that consent is inherently async. Hence users of @guardian/libs have to make their functions async when switching to using this PR. Initially make using @guardian/libs a warning to use this version instead.
  3. After some time (perhaps 4 months) make using @guardian/libs a lint error to point to using this version.

Copy link

changeset-bot bot commented Dec 15, 2023

🦋 Changeset detected

Latest commit: 0f8b4bf

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@guardian/consent-management-platform Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vlkerag vlkerag changed the title cmpGetLocalStorageItem for first discussion cmpGetLocalStorageItem POC Dec 15, 2023
@vlkerag vlkerag changed the title cmpGetLocalStorageItem POC get/set local/session storage/cookies through cmp Dec 19, 2023
console.log(`consentState.canTarget: ${consentState.canTarget}`);
*/

switch(useCase) {
Copy link
Member

Choose a reason for hiding this comment

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

I think there might be one other use-case for permissions 1 and 2 and maybe 7 - "Limited data advertising" - I'm going to ask Data Privacy if Article Count can switch to use [1, 2 and 7] rather than [1, 3 and 7].

@vlkerag
Copy link
Contributor Author

vlkerag commented Dec 20, 2023

apparently getraw and setraw are also used and hence will also have to wrap those: https://github.com/search?q=org%3Aguardian%20storage.local.getraw&type=code

@vlkerag
Copy link
Contributor Author

vlkerag commented Jan 18, 2024

[beta] @guardian/consent-management-platform

Copy link
Contributor

github-actions bot commented Jan 18, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
79.04% (-13.42% 🔻)
279/353
🟡 Branches
66.45% (-16.73% 🔻)
103/155
🟡 Functions
77.17% (-12.83% 🔻)
71/92
🟡 Lines
78.49% (-13.7% 🔻)
270/344
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🔴 cmpCookies.ts 24% 0% 0% 24%
🔴 hasConsentForUseCase.ts 33.33% 14.29% 50% 33.33%
🔴 cmpStorage.ts 47.92% 31.82% 50% 47.92%

Test suite run success

339 tests passing in 18 suites.

Report generated by 🧪jest coverage report action from 0f8b4bf

@vlkerag vlkerag added the [beta] @guardian/consent-management-platform This should debloy to beta label Jan 18, 2024
Copy link
Contributor

🚀 0.0.0-beta-20240118125826 published to npm as a beta release

@sndrs
Copy link
Member

sndrs commented Jan 22, 2024

After some further thought following our conversation with @akinsola-guardian, @sookburt and @mxdvl on friday, I think this is how i see things...

Whatever we do, these need to be true:

  1. We need consent checks for anything that writes data to a user's computer
  2. Checks need to be up-to-date each time they return
  3. We trust ourselves to try to be compliant. The intention is to simplify compliance and enable teams to focus on their domain problems:
    1. the consent team should be able to focus on consent-oriented development
    2. consumers should be able to focus on their immediate task (product delivery)
    3. csti/devex/anyone-who's-interested should be able to focus on writing useful helpers to common problems (handling local storage quirks etc)

Current proposal

The current proposal provides extended versions the existing @guardian/libs helpers by wrapping them in consent checks inside the CMP.

This achieves 1, but I think breaks 3:

  • it either creates multiple instances of near-identical functionality across different libraries:
    • without consent checks in @guardian/libs
    • with consent checks in @guardian/consent-management-platform
  • or, if we move the helper to the CMP, passes responsibility for maintaining the helpers to the consent team

Other options

Keep the helpers in libs, import the CMP

As you mention that creates a cyclical peer-dependency.

This cannot work, because it creates an eternally recurring peerDep bumping cycle, among other good other reasons not to.

Keep libs and CMP separate, offer self-certification in helper

e.g.

import { checkConsent, onConsentChange } from '@guardian/consent-management-platform'

onConsentChange(consentState => {
	storage.local.set('a', 'b', { 
		consent: checkConsent(consentState) // `true` or `false`
	})
})

I think this bends 3.i and breaks 3.ii – it passes much of the responsibility onto consumers, and any changes to the CMP implementation will need updates to consumer code.

I think the fact you could pass a simple true to override it is a sign that this is too brittle.

Dependency injection

e.g.

import { checkConsent, onConsentChange } from '@guardian/consent-management-platform'

onConsentChange(consentState => {
	storage.local.set('a', 'b', { checkConsent,	consentState })
})

This is safer, and while you could pass a function of the same signature as checkConsent that just returned true, that not something you're likely to do by accident.

But it does still pass a lot of work onto consumers, requiring them to compose consent checking and storage access with little benefit to them.

I'd say it bends 3.i and 3.ii?

Ideal API?

My feeling is we want an API that's something like this:

import { cookie, storage } from '@guardian/libs'

await cookie.set('a', 'b', { useCase: 'good' })
await storage.local.set('a', 'b', { useCase: 'evil' })

It abstracts all consent logic away to the CMP (and so the consent team), and allows consumers to focus on their job at hand (saving something to storage) while making it easy for them provide the reason.

It does mean the function becomes async, but we can keep, deprecate and lint against the current sync signature, while adding the consent options object to make async:

storage.local.set('a', 'b') // current, no consent
await storage.local.set('a', 'b', { useCase: 'evil' }) // new, with consent

How do we get there?

By moving the CMP modules to @guardian/libs. All of the above problems go away.

The only reason they are different codebases is because that's how we're organised internally. Until now, it was annoying (the waterfall of package bumps between projects) but it didn't impede functionality.

But given all the above, I think it's the best eventual outcome.

What about now?

It obviously not the quickest solution. In the meantime, if we agree that would be a good eventuality, we could still inject an instance/methods of the CMP into @guardian/libs from consumer code, something like:

import { checkConsent } from '@guardian/consent-management-platform'
import { provideConsentCheck, storage } from '@guardian/libs'

provideConsentCheck(checkConsent)

cmp.init({})

// `storage.local.set` uses an async `checkConsent` internally
await storage.local.set('a', 'b', { useCase })

Once the CMP code has been integrated into libs, the final API would be something like:

import { cmp, storage } from '@guardian/libs'

cmp.init({})

await storage.local.set('a', 'b', { useCase })

@vlkerag
Copy link
Contributor Author

vlkerag commented Jan 22, 2024

[beta] @guardian/consent-management-platform

@vlkerag vlkerag added [beta] @guardian/consent-management-platform This should debloy to beta and removed [beta] @guardian/consent-management-platform This should debloy to beta labels Jan 22, 2024
Copy link
Contributor

🚀 0.0.0-beta-20240122213831 published to npm as a beta release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants