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

feat: Improve impersonation logic #26891

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
34 changes: 27 additions & 7 deletions frontend/src/layout/navigation/ProjectNotice.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { IconGear, IconPlus } from '@posthog/icons'
import { Spinner } from '@posthog/lemon-ui'
import { useActions, useValues } from 'kea'
import { dayjs } from 'lib/dayjs'
import { LemonBanner } from 'lib/lemon-ui/LemonBanner'
Expand All @@ -22,26 +23,38 @@ interface ProjectNoticeBlueprint {
closeable?: boolean
}

function CountDown({ datetime }: { datetime: dayjs.Dayjs }): JSX.Element {
function CountDown({ datetime, callback }: { datetime: dayjs.Dayjs; callback?: () => void }): JSX.Element {
const [now, setNow] = useState(dayjs())

// Format the time difference as 00:00:00
const duration = dayjs.duration(datetime.diff(now))
const pastCountdown = duration.seconds() < 0

const countdown = pastCountdown
? 'Expired'
: duration.hours() > 0
? duration.format('HH:mm:ss')
: duration.format('mm:ss')

useEffect(() => {
const interval = setInterval(() => setNow(dayjs()), 1000)
return () => clearInterval(interval)
}, [])

// Format the time difference as 00:00:00
const duration = dayjs.duration(datetime.diff(now))
const countdown = duration.hours() > 0 ? duration.format('HH:mm:ss') : duration.format('mm:ss')
useEffect(() => {
if (pastCountdown) {
callback?.()
}
}, [pastCountdown])

return <>{countdown}</>
}

export function ProjectNotice(): JSX.Element | null {
const { projectNoticeVariant } = useValues(navigationLogic)
const { currentOrganization } = useValues(organizationLogic)
const { logout } = useActions(userLogic)
const { user } = useValues(userLogic)
const { logout, loadUser } = useActions(userLogic)
const { user, userLoading } = useValues(userLogic)
const { closeProjectNotice } = useActions(navigationLogic)
const { showInviteModal } = useActions(inviteLogic)
const { requestVerificationLink } = useActions(verifyEmailLogic)
Expand Down Expand Up @@ -124,7 +137,14 @@ export function ProjectNotice(): JSX.Element | null {
You are currently logged in as a customer.{' '}
{user?.is_impersonated_until && (
<>
Expires in <CountDown datetime={dayjs(user.is_impersonated_until)} />
Expires in <CountDown datetime={dayjs(user.is_impersonated_until)} callback={loadUser} />
{userLoading ? (
<Spinner />
) : (
<Link className="ml-2" onClick={() => loadUser()}>
Refresh
</Link>
)}
</>
)}
</>
Expand Down
14 changes: 13 additions & 1 deletion posthog/middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -677,7 +677,19 @@ def get_impersonated_session_expires_at(request: HttpRequest) -> Optional[dateti

init_time = get_or_set_session_cookie_created_at(request=request)

return datetime.fromtimestamp(init_time) + timedelta(seconds=settings.IMPERSONATION_TIMEOUT_SECONDS)
last_activity_time = request.session.get(settings.IMPERSONATION_COOKIE_LAST_ACTIVITY_KEY, init_time)
Copy link
Member

Choose a reason for hiding this comment

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

thinking out loud

a mini google suggests the default session engine signs the cookie content so a naughty person can't send their own init time here

(and anyway it would only be useful to an attacker if they had control of an impersonated session which would mean even if you could edit this then it wouldn't matter since they have impersonation)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you're confusing session and cookies. The cookie just has a sessionid. The session is loaded in django from the DB. So here it is modifying the session DB entry, nothing from the user.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome, bad googling from me 👍


# If the last activity time is less than the idle timeout, we extend the session
if time.time() - last_activity_time < settings.IMPERSONATION_IDLE_TIMEOUT_SECONDS:
last_activity_time = request.session[settings.IMPERSONATION_COOKIE_LAST_ACTIVITY_KEY] = time.time()
request.session.modified = True

idle_expiry_time = datetime.fromtimestamp(last_activity_time) + timedelta(
seconds=settings.IMPERSONATION_IDLE_TIMEOUT_SECONDS
)
total_expiry_time = datetime.fromtimestamp(init_time) + timedelta(seconds=settings.IMPERSONATION_TIMEOUT_SECONDS)

return min(idle_expiry_time, total_expiry_time)


class AutoLogoutImpersonateMiddleware:
Expand Down
10 changes: 9 additions & 1 deletion posthog/settings/web.py
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,15 @@
# Used only to display in the UI to inform users of allowlist options
PUBLIC_EGRESS_IP_ADDRESSES = get_list(os.getenv("PUBLIC_EGRESS_IP_ADDRESSES", ""))

IMPERSONATION_TIMEOUT_SECONDS = get_from_env("IMPERSONATION_TIMEOUT_SECONDS", 15 * 60, type_cast=int)
# The total time allowed for an impersonated session
IMPERSONATION_TIMEOUT_SECONDS = get_from_env("IMPERSONATION_TIMEOUT_SECONDS", 60 * 60 * 2, type_cast=int)
# The time allowed for an impersonated session to be idle before it expires
IMPERSONATION_IDLE_TIMEOUT_SECONDS = get_from_env("IMPERSONATION_IDLE_TIMEOUT_SECONDS", 30 * 60, type_cast=int)
# Impersonation cookie last activity key
IMPERSONATION_COOKIE_LAST_ACTIVITY_KEY = get_from_env(
"IMPERSONATION_COOKIE_LAST_ACTIVITY_KEY", "impersonation_last_activity"
)

SESSION_COOKIE_CREATED_AT_KEY = get_from_env("SESSION_COOKIE_CREATED_AT_KEY", "session_created_at")

PROJECT_SWITCHING_TOKEN_ALLOWLIST = get_list(os.getenv("PROJECT_SWITCHING_TOKEN_ALLOWLIST", "sTMFPsFhdP1Ssg"))
Expand Down
55 changes: 50 additions & 5 deletions posthog/test/test_middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,8 @@ def test_logout(self):
self.assertNotIn("ph_current_project_name", response.cookies)


@override_settings(IMPERSONATION_TIMEOUT_SECONDS=30)
@override_settings(IMPERSONATION_TIMEOUT_SECONDS=100)
@override_settings(IMPERSONATION_IDLE_TIMEOUT_SECONDS=20)
class TestAutoLogoutImpersonateMiddleware(APIBaseTest):
other_user: User

Expand Down Expand Up @@ -538,21 +539,65 @@ def test_not_staff_user_cannot_login(self):
assert response.status_code == 200
assert self.client.get("/api/users/@me").json()["email"] == self.user.email

def test_after_timeout_api_requests_401(self):
now = datetime.now()
def test_after_idle_timeout_api_requests_401(self):
now = datetime(2024, 1, 1, 12, 0, 0)
with freeze_time(now):
self.login_as_other_user()
res = self.client.get("/api/users/@me")
assert res.status_code == 200
assert res.json()["email"] == "[email protected]"
assert res.json()["is_impersonated_until"] == "2024-01-01T12:00:20+00:00"
assert self.client.session.get("session_created_at") == now.timestamp()

with freeze_time(now + timedelta(seconds=10)):
# Move forward by 19
now = now + timedelta(seconds=19)
with freeze_time(now):
res = self.client.get("/api/users/@me")
assert res.status_code == 200
assert res.json()["email"] == "[email protected]"
assert res.json()["is_impersonated_until"] == "2024-01-01T12:00:39+00:00"

with freeze_time(now + timedelta(seconds=35)):
# Past idle timeout
now = now + timedelta(seconds=21)

with freeze_time(now):
res = self.client.get("/api/users/@me")
assert res.status_code == 401

def test_after_total_timeout_api_requests_401(self):
now = datetime(2024, 1, 1, 12, 0, 0)
with freeze_time(now):
self.login_as_other_user()
res = self.client.get("/api/users/@me")
assert res.status_code == 200
assert res.json()["email"] == "[email protected]"
assert res.json()["is_impersonated_until"] == "2024-01-01T12:00:20+00:00"
assert self.client.session.get("session_created_at") == now.timestamp()

for _ in range(4):
# Move forward by 19 seconds 4 times for a total of 76 seconds
now = now + timedelta(seconds=19)
with freeze_time(now):
res = self.client.get("/api/users/@me")
assert res.status_code == 200
assert res.json()["email"] == "[email protected]"
# Format exactly like the date above
assert res.json()["is_impersonated_until"] == (now + timedelta(seconds=20)).strftime(
"%Y-%m-%dT%H:%M:%S+00:00"
)

now = now + timedelta(seconds=19)
with freeze_time(now):
res = self.client.get("/api/users/@me")
assert res.status_code == 200
assert res.json()["email"] == "[email protected]"
# Even though below the idle timeout, we now see the total timeout as that is earlier
assert res.json()["is_impersonated_until"] == "2024-01-01T12:01:40+00:00"

# Now even less than the idle time will take us past the total timeout
now = now + timedelta(seconds=10)

with freeze_time(now):
res = self.client.get("/api/users/@me")
assert res.status_code == 401

Expand Down
Loading