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

fix: session when impersonated #25897

Merged
merged 1 commit into from
Oct 30, 2024
Merged

Conversation

pauldambra
Copy link
Member

session replay takes any distinct id from a session

we've noticed a couple of times that when posthog staff impersonate a user sometimes that session ends up having the impersonated user's distinct id in the replay events table

and if you then search for posthog staff we'll display the, legitimately by a posthog-person, session as if its user was the impersonated user

the persons in posthog aren't being merged, we're just managing to write a "bad" distinct id into replay events

i don't think i can do impersonation locally, and it's not happening every time, so it's difficult to reproduce but...

let's reset the session id when we impersonate a user to try and avoid this (I guess) little race

}

if (window.IMPERSONATED_SESSION) {
posthog.opt_out_capturing()
loadedInstance.sessionManager?.resetSessionId()
Copy link
Member Author

@pauldambra pauldambra Oct 30, 2024

Choose a reason for hiding this comment

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

added this to (hopefully) ensure impersonation is always a new session

i'm slightly worried still about a race between start-up continuing a recording and the loaded callback, but this is the lightest touch I can see to try and fix

Comment on lines +44 to +63
const Cypress = (window as any).Cypress

if (Cypress) {
Object.entries(Cypress.env()).forEach(([key, value]) => {
if (key.startsWith('POSTHOG_PROPERTY_')) {
loadedInstance.register_for_session({
[key.replace('POSTHOG_PROPERTY_', 'E2E_TESTING_').toLowerCase()]: value,
})
}
})
}

// This is a helpful flag to set to automatically reset the recording session on load for testing multiple recordings
const shouldResetSessionOnLoad = loadedInstance.getFeatureFlag(FEATURE_FLAGS.SESSION_RESET_ON_LOAD)
if (shouldResetSessionOnLoad) {
loadedInstance.sessionManager?.resetSessionId()
}

// Make sure we have access to the object in window for debugging
window.posthog = loadedInstance
Copy link
Member Author

Choose a reason for hiding this comment

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

moved these into the loaded callback to make it a little clearer what we think we're operating on

@pauldambra pauldambra requested review from oliverb123 and a team October 30, 2024 11:08
Copy link
Contributor

Size Change: 0 B

Total Size: 1.15 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 1.15 MB

compressed-size-action

Copy link
Contributor

@oliverb123 oliverb123 left a comment

Choose a reason for hiding this comment

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

Thanks for taking a swing at this!

@pauldambra pauldambra merged commit a9a77ac into master Oct 30, 2024
95 checks passed
@pauldambra pauldambra deleted the fix/session-when-impersonated branch October 30, 2024 11:39
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.

2 participants