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

Make cookie domain configurable for container #94

Open
kingo55 opened this issue Apr 22, 2021 · 5 comments
Open

Make cookie domain configurable for container #94

kingo55 opened this issue Apr 22, 2021 · 5 comments
Labels
enhancement New feature or request

Comments

@kingo55
Copy link
Member

kingo55 commented Apr 22, 2021

For cookies like the Mojito User ID and test assignment, we should allow users to specify the domain and/or behaviour of the cookies (e.g. properties).

It might even be worth updating the cookie library we use too.

Screen Shot 2021-04-22 at 17 05 19

@kingo55
Copy link
Member Author

kingo55 commented Jun 30, 2021

@dapperdrop and I just caught up to review the cookie enhancements properly. Would you guys have any thoughts to share @awoehrl and @allmywant ?

Here's a brief rundown of what we're thinking:

Current state

We set two cookies from the library for user salt and recording assignment:

Cookie name Purpose
_mjo User salt (optional)
mojito{{id}}{{state}} Assignment record

We have some undocumented settings for the name prefix, domain and expiry - but we're missing some important new cookies for cross-site/secure usage:

Settings Supported Features
Cookie name Yes undocumented, via this.options.options.cookiePrefix
Domain Yes undocumented, via this.options.options.cookieDomain
Expiry Yes undocumented, via this.options.options.cookieDuration
Path No
Secure No
Same site No

The original undocumented API was setup a long time ago when there were fewer options we had to support:

Existing undocumented API Purpose
Mojito.options.cookiePrefix Sets the name of assignment cookies
Mojito.options.cookieDomain Sets the domain of all cookies
Mojito.options.cookieDuration Sets the duration of assignment cookies

Proposed state, requirements & key questions

  1. Need to consider backwards compatibility with the existing undocumented API
  2. Do we need to upgrade libraries? Or review new options to migrate to?: https://github.com/js-cookie/js-cookie
  3. How do we reduce or maintain the size of the library?
  4. Can we ensure the new solution is more robust?
  5. How can we implement tests to ensure the library performs the right cookie actions?
  6. How do we specify different options between a) User salt and b) Assignment cookies?

Example proposed API

We could simply accept an object with settings specified.

Mojito.options.cookie = {
    domain: '.foo.bar',
    path: '/',
    secure: true,
    sameSite: 'Lax'
};

Maybe we need to have settings specified for the user salt cookie (because that may have different expiry/naming conventions).

@allmywant
Copy link
Member

@kingo55 @dapperdrop
The proposed API looking good but I think we can expose all cookie options through the API, we can add follow options:

Mojito.options.cookie = {
    prefix: 'foo',
    expiry: 90, // days
    domain: '.foo.bar',
    path: '/',
    secure: true,
    sameSite: 'Lax'
};

Same API can be used for the user salt cookie.

Need to consider backwards compatibility with the existing undocumented API
We will need a default cook option object and merge it with the Mojito.options.cookie (higher priority) and undocumented APIs and use the merged options for all cookie actions.

Do we need to upgrade libraries? Or review new options to migrate to?: https://github.com/js-cookie/js-cookie

js-cookie looks great, we can consider to migrate to it but it will be a big change to the library if we use js-cookie directly without modifying its APIs.

How can we implement tests to ensure the library performs the right cookie actions?

I think we can write a set of tests write cookies with the proposed option(s) then read and check it.

@kingo55
Copy link
Member Author

kingo55 commented Jun 30, 2021

we can add follow options:

Yes, that's a good idea... I like that. Is it better to represent the cookie expiry in days or seconds?

js-cookie looks great, we can consider to migrate to it but it will be a big change to the library if we use js-cookie directly without modifying its APIs.

Agreed. We haven't had any issues with the current library, so this isn't a big priority.

How can we implement tests to ensure the library performs the right cookie actions?

I think we can write a set of tests write cookies with the proposed option(s) then read and check it.

I wonder whether we can test the cookieDomain/secure in a way that works inside the environments we run tests within (GitHub Actions / Mocha). Most of the other cookie settings should be pretty easy to test for though (path/same site etc).

@allmywant
Copy link
Member

Is it better to represent the cookie expiry in days or seconds?

I think days is better.

I wonder whether we can test the cookieDomain/secure in a way that works inside the environments we run tests within (GitHub Actions / Mocha).

Ahh this is the problem, it seems we have to tests online, e.g. mintmetrics.io

@kingo55
Copy link
Member Author

kingo55 commented Jun 30, 2021

Cool - yeah, days is much more practical.

I thought the testing environment would be an issue here. Maybe we'd only have to build out the unit tests that we can and manually test anything we can't. Looks like js-cookie doesn't test those cases either.

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

No branches or pull requests

2 participants