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.

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

Jump to bottom

Add the root domain check method as a util method added for use mod… #6124

Merged
merged 6 commits into from
Jan 13, 2021
Merged

Add the root domain check method as a util method added for use mod… #6124

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bebc77d
Add the root domain check method as a global method added for use mod…
markaconrad Dec 11, 2020
f32b599
Add a date suffix to keep the test cookie random
markaconrad Dec 11, 2020
0672f2b
Per review, moved the findRootDomain method to utils out of the userI…
markaconrad Dec 16, 2020
18ebc36
Merge branch 'master' into add_rootDomainCheck
markaconrad Jan 7, 2021
47ec3b2
More the domain method back the user module but attach it to the subm…
markaconrad Jan 7, 2021
b0204b7
Remove the storageManager as it is no longer used in utils
markaconrad Jan 8, 2021
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
55 changes: 55 additions & 0 deletions modules/userId/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,60 @@ function hasGDPRConsent(consentData) {
return true;
}

/**
* Find the root domain
* @param {string|undefined} fullDomain
* @return {string}
*/
function findRootDomain(fullDomain = document.domain) {
FilipStamenkovic marked this conversation as resolved.
Show resolved Hide resolved
if (!coreStorage.cookiesAreEnabled()) {
return fullDomain;
}

const domainParts = fullDomain.split('.');
if (domainParts.length == 2) {
return fullDomain;
}
let rootDomain;
let continueSearching;
let startIndex = -2;
const TEST_COOKIE_NAME = `_rdc${Date.now()}`;
const TEST_COOKIE_VALUE = 'writeable';
do {
rootDomain = domainParts.slice(startIndex).join('.');
let expirationDate = new Date(utils.timestamp() + 10 * 1000).toUTCString();

// Write a test cookie
coreStorage.setCookie(
TEST_COOKIE_NAME,
TEST_COOKIE_VALUE,
expirationDate,
'Lax',
rootDomain,
undefined
);

// See if the write was successful
const value = coreStorage.getCookie(TEST_COOKIE_NAME, undefined);
if (value === TEST_COOKIE_VALUE) {
continueSearching = false;
// Delete our test cookie
coreStorage.setCookie(
TEST_COOKIE_NAME,
'',
'Thu, 01 Jan 1970 00:00:01 GMT',
undefined,
rootDomain,
undefined
);
} else {
startIndex += -1;
continueSearching = Math.abs(startIndex) <= domainParts.length;
}
} while (continueSearching);
return rootDomain;
}

/**
* @param {SubmoduleContainer[]} submodules
* @param {function} cb - callback for after processing is done.
Expand Down Expand Up @@ -727,6 +781,7 @@ export function init(config) {
(getGlobal()).getUserIds = getUserIds;
(getGlobal()).getUserIdsAsEids = getUserIdsAsEids;
(getGlobal()).refreshUserIds = refreshUserIds;
(getGlobal()).findRootDomain = findRootDomain;
Copy link
Contributor

Choose a reason for hiding this comment

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

The plan for this function is to be used within pbjs? So, it won't be executed outside pbjs?
If that is the case then maybe we should not make this function accessible on pbjs namespace, but rather just exporting the function and then just using it inside some module?

If you decide to go that way (exporting instead of marking it available on pbjs) I suggest you move it in utils package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I have done so

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for going back and forth, I didn't think this through, my bad. Now I see that utils may not be the best place to put this function.
Now utils has dependency to storageManager.js, which should not happen. utils should not depend on storageManager.
Can you move this function to userId/index.js (again) and just leave it as exported instead of attaching it on pbjs namespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, yeah, I was on the fence about that so that's why I put it in the user module initially but now I'm not sure how my module will get access to it. The linter forbids me from importing directly from userId/index.js. Do you have any thoughts on that? What do you think about adding the root domain to the config object that gets passed into getId

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or maybe in userSync.js?

Copy link
Contributor

@FilipStamenkovic FilipStamenkovic Dec 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I would choose userSync.js.
@smenzer do you have any opinion on this one?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we should add it to the config that gets passed to getId, because that is specifically the config for that module. we could add it as a new parameter, though. would need to get passed to extendId as well.

the only issue i have with moving it to userSync.js is that we aren't even using that module today within userId module, so seems odd to put it there just because it's somewhat related.

since we can't use utils, which is where I would have expected to put this (but I understand the reason not to), and since putting it in userId/index.js won't work for using it within submodules...i think we're really left with two options:

  1. put it back on the global pbjs namespace
  2. pass it in the function calls to the modules

if we pass the result of the function to all modules, then it means we're executing that code EVERY time, which isn't necessary since most modules aren't using it today. we could pass it as a function to be used if necessary, but not sure if that's a good idea or not. which leads us back to putting it in the pbjs namespace even though it may not be used outside of pbjs ...

not a real answer i know, but that's how i'm thinking about it. maybe we can get some other opinions in the slack group, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checking in to see if there is consensus on an approach that I should take, thanks!

}

// init config update listener to start the application
Expand Down
45 changes: 45 additions & 0 deletions test/spec/modules/userId_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,6 +410,51 @@ describe('User ID', function () {
});
});

describe('findRootDomain', function () {
let sandbox;

beforeEach(function () {
setSubmoduleRegistry([pubCommonIdSubmodule]);
init(config);
config.setConfig({
userSync: {
syncDelay: 0,
userIds: [
{
name: 'pubCommonId',
value: { pubcid: '11111' },
},
],
},
});
sandbox = sinon.createSandbox()
sandbox
.stub(coreStorage, 'getCookie')
.onFirstCall()
.returns(null) // .co.uk
.onSecondCall()
.returns('writeable'); // realdomain.co.uk;
});

afterEach(function() {
sandbox.restore();
})

it('should be a global function', function() {
expect(typeof (getGlobal()).findRootDomain).to.equal('function');
})

it('should just find the root domain', function () {
var domain = getGlobal().findRootDomain('sub.realdomain.co.uk');
expect(domain).to.be.eq('realdomain.co.uk');
});

it('should find the full domain when no subdomain is present', function () {
var domain = getGlobal().findRootDomain('realdomain.co.uk');
expect(domain).to.be.eq('realdomain.co.uk');
});
});

describe('Opt out', function () {
before(function () {
coreStorage.setCookie(PBJS_USER_ID_OPTOUT_NAME, '1', (new Date(Date.now() + 5000).toUTCString()));
Expand Down