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

[8.2] [FullStory] Improve UUID generation #131008

Merged
merged 4 commits into from
May 6, 2022
Merged

[8.2] [FullStory] Improve UUID generation #131008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4fa6973
[8.2] [FullStory] Improve UUID generation
afharo Apr 26, 2022
b277463
Validate the Authentication Realm type as well
afharo May 5, 2022
bebf248
Merge branch '8.2' into hotfix/8.2/fullstory-userId
kibanamachine May 5, 2022
1c10a13
More explicit tests about hashed plain username
afharo May 6, 2022
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
87 changes: 68 additions & 19 deletions x-pack/plugins/cloud/public/plugin.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* 2.0.
*/

import { sha256 } from 'js-sha256';
import { nextTick } from '@kbn/test-jest-helpers';
import { coreMock } from 'src/core/public/mocks';
import { homePluginMock } from 'src/plugins/home/public/mocks';
Expand All @@ -17,6 +18,9 @@ import { KibanaExecutionContext } from 'kibana/public';
describe('Cloud Plugin', () => {
describe('#setup', () => {
describe('setupFullstory', () => {
const username = '1234';
const expectedHashedPlainUsername = sha256(username);

beforeEach(() => {
jest.clearAllMocks();
});
Expand Down Expand Up @@ -74,9 +78,7 @@ describe('Cloud Plugin', () => {
it('calls initializeFullStory with correct args when enabled and org_id are set', async () => {
const { initContext } = await setupPlugin({
config: { full_story: { enabled: true, org_id: 'foo' } },
currentUserProps: {
username: '1234',
},
currentUserProps: { username },
});

expect(initializeFullStoryMock).toHaveBeenCalled();
Expand All @@ -89,9 +91,7 @@ describe('Cloud Plugin', () => {
it('calls FS.identify with hashed user ID when security is available', async () => {
await setupPlugin({
config: { full_story: { enabled: true, org_id: 'foo' } },
currentUserProps: {
username: '1234',
},
currentUserProps: { username },
});

expect(fullStoryApiMock.identify).toHaveBeenCalledWith(
Expand All @@ -106,35 +106,81 @@ describe('Cloud Plugin', () => {
);
});

it('user hash includes org id', async () => {
it('user hash includes the org id when not authenticated via Cloud SAML', async () => {
await setupPlugin({
config: { full_story: { enabled: true, org_id: 'foo' }, id: 'esOrg1' },
currentUserProps: {
username: '1234',
},
currentUserProps: { username },
});

expect(fullStoryApiMock.identify).toHaveBeenCalledTimes(1);
const hashId1 = fullStoryApiMock.identify.mock.calls[0][0];
expect(hashId1).not.toEqual(expectedHashedPlainUsername);

await setupPlugin({
config: { full_story: { enabled: true, org_id: 'foo' }, id: 'esOrg2' },
currentUserProps: {
username: '1234',
},
currentUserProps: { username },
});

expect(fullStoryApiMock.identify).toHaveBeenCalledTimes(2);
const hashId2 = fullStoryApiMock.identify.mock.calls[1][0];
expect(hashId2).not.toEqual(expectedHashedPlainUsername);

expect(hashId1).not.toEqual(hashId2);
});

it('user hash does not include the org id when there is none', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

To actually test this case, wouldn't you want to include an id in one of the calls to setupPlugin and not to the other, so that you can assert that the hashes do not match?

As it is written I'm not sure the test is really proving that an org id isn't included, since both setupPlugin calls are identical.

It might be simpler to create a hash of the expected ID directly, and then assert that the call to fullStoryApiMock.identify matches, e.g.

      it('user hash does not include the org id when there is none', async () => {
        const username = '1234';
        const expected = sha256(username);

        await setupPlugin({
          config: { full_story: { enabled: true, org_id: 'foo' }, id: undefined },
          currentUserProps: { username },
        });

        const hashId = fullStoryApiMock.identify.mock.calls[0][0];
        expect(expected).toEqual(hashId);
      });

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call! I pushed a change to make the tests more explicit about using the hashed plain username :)

await setupPlugin({
config: { full_story: { enabled: true, org_id: 'foo' }, id: undefined },
currentUserProps: { username },
});

expect(fullStoryApiMock.identify).toHaveBeenCalledWith(expectedHashedPlainUsername, {
version_str: 'version',
version_major_int: -1,
version_minor_int: -1,
version_patch_int: -1,
});
});

it('user hash does not include org id when authenticated via Cloud SAML', async () => {
await setupPlugin({
config: { full_story: { enabled: true, org_id: 'foo' }, id: 'esOrg1' },
currentUserProps: {
username,
authentication_realm: { type: 'saml', name: 'cloud-saml-kibana' },
},
});

expect(fullStoryApiMock.identify).toHaveBeenCalledWith(expectedHashedPlainUsername, {
version_str: 'version',
version_major_int: -1,
version_minor_int: -1,
version_patch_int: -1,
org_id_str: 'esOrg1',
});

await setupPlugin({
config: { full_story: { enabled: true, org_id: 'foo' }, id: 'esOrg2' },
currentUserProps: {
username,
authentication_realm: { type: 'saml', name: 'cloud-saml-kibana' },
},
});

expect(fullStoryApiMock.identify).toHaveBeenCalledWith(expectedHashedPlainUsername, {
version_str: 'version',
version_major_int: -1,
version_minor_int: -1,
version_patch_int: -1,
org_id_str: 'esOrg2',
});
});

it('calls FS.setVars everytime an app changes', async () => {
const currentContext$ = new Subject<KibanaExecutionContext>();
const { plugin } = await setupPlugin({
config: { full_story: { enabled: true, org_id: 'foo' } },
currentUserProps: {
username: '1234',
},
currentUserProps: { username },
currentContext$,
});

Expand Down Expand Up @@ -251,16 +297,19 @@ describe('Cloud Plugin', () => {
fullStoryApiMock.identify.mockImplementationOnce(() => {
throw new Error(`identify failed!`);
});
const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementationOnce(() => {});
const { initContext } = await setupPlugin({
config: { full_story: { enabled: true, org_id: 'foo' } },
currentUserProps: {
username: '1234',
},
currentUserProps: { username },
});

expect(fullStoryApiMock.event).toHaveBeenCalledWith('Loaded Kibana', {
kibana_version_str: initContext.env.packageInfo.version,
});
expect(consoleErrorSpy).toHaveBeenCalledWith(
'[cloud.full_story] Could not call FS.identify due to error: Error: identify failed!',
expect.any(Error)
);
});

it('does not call initializeFullStory when enabled=false', async () => {
Expand Down
24 changes: 20 additions & 4 deletions x-pack/plugins/cloud/public/plugin.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,10 @@ export class CloudPlugin implements Plugin<CloudSetup> {
// Keep this import async so that we do not load any FullStory code into the browser when it is disabled.
const fullStoryChunkPromise = import('./fullstory');
const userIdPromise: Promise<string | undefined> = security
? loadFullStoryUserId({ getCurrentUser: security.authc.getCurrentUser })
? loadFullStoryUserId({
cloudDeploymentId: this.config.id,
getCurrentUser: security.authc.getCurrentUser,
})
: Promise.resolve(undefined);

// We need to call FS.identify synchronously after FullStory is initialized, so we must load the user upfront
Expand All @@ -264,9 +267,8 @@ export class CloudPlugin implements Plugin<CloudSetup> {
// This needs to be called syncronously to be sure that we populate the user ID soon enough to make sessions merging
// across domains work
if (userId) {
// Join the cloud org id and the user to create a truly unique user id.
// The hashing here is to keep it at clear as possible in our source code that we do not send literal user IDs
const hashedId = sha256(esOrgId ? `${esOrgId}:${userId}` : `${userId}`);
const hashedId = sha256(`${userId}`);

executionContextPromise
?.then(async (executionContext) => {
Expand Down Expand Up @@ -377,8 +379,10 @@ export class CloudPlugin implements Plugin<CloudSetup> {

/** @internal exported for testing */
export const loadFullStoryUserId = async ({
cloudDeploymentId,
getCurrentUser,
}: {
cloudDeploymentId?: string;
getCurrentUser: () => Promise<AuthenticatedUser>;
}) => {
try {
Expand All @@ -395,9 +399,21 @@ export const loadFullStoryUserId = async ({
currentUser.metadata
)}`
);

return undefined;
}

if (
getIsCloudEnabled(cloudDeploymentId) &&
currentUser.authentication_realm?.type === 'saml' &&
currentUser.authentication_realm?.name === 'cloud-saml-kibana'
Copy link
Member

Choose a reason for hiding this comment

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

To be extra safe, I think we'll also want to check that currentUser.authentication_realm?.type === 'saml'.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call! Added!

) {
return currentUser.username;
}

return currentUser.username;
return cloudDeploymentId
? `${cloudDeploymentId}:${currentUser.username}`
: currentUser.username;
} catch (e) {
// eslint-disable-next-line no-console
console.error(`[cloud.full_story] Error loading the current user: ${e.toString()}`, e);
Expand Down