Skip to content

feat(#78): allow cookieExpiresAfterDays as a function#79

Open
gtnsimon wants to merge 3 commits intoboscop-fr:mainfrom
jack-agency:feature-cookie-expire
Open

feat(#78): allow cookieExpiresAfterDays as a function#79
gtnsimon wants to merge 3 commits intoboscop-fr:mainfrom
jack-agency:feature-cookie-expire

Conversation

@gtnsimon
Copy link
Copy Markdown
Contributor

@gtnsimon gtnsimon commented Jan 12, 2022

This allow to conditionally define cookie duration based on user consent.

Here a snippet function to control cookie duration using a function:

// orejime.config.js
const config = {
  /* ... */
  cookieExpiresAfterDays(consents, config) {
		if (consents) {
			// ignore required apps (true is enforced)
			const consentableApps = config.apps
				.filter((app) => app.required !== true)
				.map((app) => app.name);
			// get selected user apps
			const userConsents = Object.values(
				Object.fromEntries(
					Object.entries(consents).filter(([name]) =>
						consentableApps.includes(name)
					)
				)
			);

			// cookie will expire after 90 days for users who accepted all apps
			if (userConsents.every((value) => value === true)) {
				return 90;
			}

			// otherwise (all apps refused or not all accepted) cookie will expire after 30 days
			return 30;
		}
	},
  /* ... */
}

Resolves: #78

This allow to conditionally define expiration days of cookie based on user consent.
@gtnsimon gtnsimon changed the title Allow cookieExpiresAfterDays to be a function feat(#78): allow cookieExpiresAfterDays as a function Jan 12, 2022
Comment thread src/consent-manager.js Outdated
? configCookieExpiresAfterDays(this.consents, this.config)
: configCookieExpiresAfterDays;

if (
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would maybe remove this test and use a default value in setCookie like before.
This way any falsy or 0 value would be set to the default.

setCookie(
	this.cookieName,
	value,
	cookieExpiresAfterDays || 120,
	this.config.cookieDomain
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

cookieExpiresAfterDays could then be a const var.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree with falsies values but 0 as an integer has a meaning for cookie expiration which ends when browser is closed. Using cookieExpiresAfterDays || 120 will replace 0 to 120 which may not be the expected behavior by the end developer.

To not break existing installations, the default could apply to non-functional cookieExpiresAfterDays (like before this feature). When used as a function it'll be up to the developer to explicitly return a value if he don't want a session cookie.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I didn't know about this behavior with 0, given the old implementation I thought it was kind of invalid.
I'm kind of torn here, it would be a breaking change to modify the current behavior, but it seems odd to have different behaviors for different methods...

Comment thread src/consent-manager.js
This will prevent replacing a `return 0` from developer code land with the default `120` from orejime.
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.

Custom cookie duration based on consents

2 participants