Skip to content

Preserve Clerk session between login window opens #642

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

Merged
highperformancecoder merged 3 commits into from
Apr 9, 2026
Merged

Preserve Clerk session between login window opens #642

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
32fbb2d
Initial plan
Copilot Apr 9, 2026
8852185
Preserve Clerk session between login window opens
Copilot Apr 9, 2026
7d51337
Fix setSession to actually reinstate the Clerk session
Copilot Apr 9, 2026
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
4 changes: 3 additions & 1 deletion gui-js/apps/minsky-electron/src/app/managers/WindowManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,12 +389,14 @@ export class WindowManager {
WindowManager._resolveAuthToken = resolve;
});

const existingToken = StoreManager.store.get('authToken') || '';

const loginWindow = WindowManager.createPopupWindowWithRouting({
width: 420,
height: 500,
title: 'Login',
modal: false,
url: '#/headless/login',
url: `#/headless/login?authToken=${encodeURIComponent(existingToken)}`,
});
Comment on lines +392 to 400
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

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

Passing the (potentially long-lived) authToken in the login window URL query string risks accidental disclosure via logs, crash reports, screenshots, devtools, or navigation history. Prefer keeping tokens out of URLs: fetch the stored token from the main process via IPC after the window loads, or rely on Clerk’s own storage restoration without transmitting the token to the renderer.

Copilot uses AI. Check for mistakes.

// Resolve with null if the user closes the window before authenticating
Expand Down
14 changes: 14 additions & 0 deletions gui-js/libs/core/src/lib/services/clerk/clerk.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,18 @@ export class ClerkService {
const token = await this.getToken();
await this.electronService.invoke(events.SET_AUTH_TOKEN, token);
}

async setSession(token: string): Promise<void> {
if (!this.clerk) throw new Error('Clerk not initialized');
// clerk.load() in initialize() restores the session from browser storage if still valid.
// If no session is active but sessions exist on the client, activate the first available one.
// The token parameter is a hint that the user previously authenticated.
if (!token) return;
if (!this.clerk.session && this.clerk.client?.sessions?.length > 0) {
await this.clerk.setActive({ session: this.clerk.client.sessions[0].id });
Comment on lines +63 to +70
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

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

Activating this.clerk.client.sessions[0] is an arbitrary choice if multiple sessions exist; it may activate a different session than the one the user expects. If multiple sessions are possible here, prefer selecting the most recently active session or otherwise deterministically choosing the intended session (and document the selection criteria).

Suggested change
async setSession(token: string): Promise<void> {
if (!this.clerk) throw new Error('Clerk not initialized');
// clerk.load() in initialize() restores the session from browser storage if still valid.
// If no session is active but sessions exist on the client, activate the first available one.
// The token parameter is a hint that the user previously authenticated.
if (!token) return;
if (!this.clerk.session && this.clerk.client?.sessions?.length > 0) {
await this.clerk.setActive({ session: this.clerk.client.sessions[0].id });
private getSessionRecencyValue(session: any): number {
const recencyCandidate = session?.lastActiveAt ?? session?.updatedAt ?? session?.createdAt;
if (typeof recencyCandidate === 'number') {
return recencyCandidate;
}
if (recencyCandidate instanceof Date) {
return recencyCandidate.getTime();
}
if (typeof recencyCandidate === 'string') {
const parsedValue = Date.parse(recencyCandidate);
return Number.isNaN(parsedValue) ? Number.NEGATIVE_INFINITY : parsedValue;
}
return Number.NEGATIVE_INFINITY;
}
private selectSessionToActivate(): any | null {
const sessions = this.clerk?.client?.sessions;
if (!Array.isArray(sessions) || sessions.length === 0) return null;
return sessions.reduce((selectedSession: any, currentSession: any) => {
if (!selectedSession) return currentSession;
const selectedRecency = this.getSessionRecencyValue(selectedSession);
const currentRecency = this.getSessionRecencyValue(currentSession);
if (currentRecency > selectedRecency) return currentSession;
if (currentRecency < selectedRecency) return selectedSession;
const selectedId = String(selectedSession?.id ?? '');
const currentId = String(currentSession?.id ?? '');
return currentId.localeCompare(selectedId) < 0 ? currentSession : selectedSession;
}, null);
}
async setSession(token: string): Promise<void> {
if (!this.clerk) throw new Error('Clerk not initialized');
// clerk.load() in initialize() restores the session from browser storage if still valid.
// If no session is active but sessions exist on the client, activate one deterministically.
// Prefer the most recently active session (`lastActiveAt`), then `updatedAt`, then `createdAt`;
// if timestamps are unavailable, fall back to the lexicographically smallest session id.
// The token parameter is a hint that the user previously authenticated.
if (!token) return;
if (!this.clerk.session && this.clerk.client?.sessions?.length > 0) {
const sessionToActivate = this.selectSessionToActivate();
if (sessionToActivate?.id) {
await this.clerk.setActive({ session: sessionToActivate.id });
}

Copilot uses AI. Check for mistakes.
}
if (!this.clerk.session) {
throw new Error('Session expired or invalid');
}
Comment on lines +63 to +74
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

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

setSession(token) does not actually use the provided token to reinstate a session; it only checks truthiness and then activates the first session from clerk.client.sessions. This is misleading for callers and makes it hard to reason about correctness/security. Consider renaming to reflect what it does (e.g., restoreSessionFromStorage()), or using the token meaningfully (or dropping the parameter entirely).

Suggested change
async setSession(token: string): Promise<void> {
if (!this.clerk) throw new Error('Clerk not initialized');
// clerk.load() in initialize() restores the session from browser storage if still valid.
// If no session is active but sessions exist on the client, activate the first available one.
// The token parameter is a hint that the user previously authenticated.
if (!token) return;
if (!this.clerk.session && this.clerk.client?.sessions?.length > 0) {
await this.clerk.setActive({ session: this.clerk.client.sessions[0].id });
}
if (!this.clerk.session) {
throw new Error('Session expired or invalid');
}
private getSessionIdFromToken(token: string): string | null {
const parts = token.split('.');
if (parts.length < 2) return null;
try {
const payload = parts[1]
.replace(/-/g, '+')
.replace(/_/g, '/')
.padEnd(Math.ceil(parts[1].length / 4) * 4, '=');
const decoded = JSON.parse(atob(payload));
return typeof decoded?.sid === 'string' ? decoded.sid : null;
} catch {
return null;
}
}
async setSession(token: string): Promise<void> {
if (!this.clerk) throw new Error('Clerk not initialized');
if (!token) throw new Error('Session token is required');
const sessionId = this.getSessionIdFromToken(token);
if (!sessionId) {
throw new Error('Session token is invalid');
}
// clerk.load() in initialize() may already have restored the matching session from browser storage.
if (this.clerk.session?.id === sessionId) {
return;
}
const matchingSession = this.clerk.client?.sessions?.find((session: { id: string }) => session.id === sessionId);
if (!matchingSession) {
throw new Error('Session expired or invalid');
}
await this.clerk.setActive({ session: matchingSession.id });
if (this.clerk.session?.id !== sessionId) {
throw new Error('Session expired or invalid');
}

Copilot uses AI. Check for mistakes.
Comment on lines +63 to +74
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

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

Throwing Session expired or invalid when no session becomes active after attempting restore forces the caller to treat “no restorable session” as an exceptional condition. For this workflow it’s usually better to return a boolean (restored vs not) and let the UI fall back to the login form without showing an error unless the user action fails.

Suggested change
async setSession(token: string): Promise<void> {
if (!this.clerk) throw new Error('Clerk not initialized');
// clerk.load() in initialize() restores the session from browser storage if still valid.
// If no session is active but sessions exist on the client, activate the first available one.
// The token parameter is a hint that the user previously authenticated.
if (!token) return;
if (!this.clerk.session && this.clerk.client?.sessions?.length > 0) {
await this.clerk.setActive({ session: this.clerk.client.sessions[0].id });
}
if (!this.clerk.session) {
throw new Error('Session expired or invalid');
}
async setSession(token: string): Promise<boolean> {
if (!this.clerk) throw new Error('Clerk not initialized');
// clerk.load() in initialize() restores the session from browser storage if still valid.
// If no session is active but sessions exist on the client, activate the first available one.
// The token parameter is a hint that the user previously authenticated.
if (!token) return false;
if (!this.clerk.session && this.clerk.client?.sessions?.length > 0) {
await this.clerk.setActive({ session: this.clerk.client.sessions[0].id });
}
return !!this.clerk.session;

Copilot uses AI. Check for mistakes.
}
}
33 changes: 29 additions & 4 deletions gui-js/libs/ui-components/src/lib/login/login.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Component, OnInit } from '@angular/core';
import { Component, OnInit, OnDestroy } from '@angular/core';
import { CommonModule } from '@angular/common';
import { FormControl, FormGroup, Validators, ReactiveFormsModule } from '@angular/forms';
import { MatButtonModule } from '@angular/material/button';
Expand All @@ -7,6 +7,8 @@ import { MatFormFieldModule } from '@angular/material/form-field';
import { MatProgressSpinnerModule } from '@angular/material/progress-spinner';
import { ClerkService } from '@minsky/core';
import { ElectronService } from '@minsky/core';
import { ActivatedRoute } from '@angular/router';
import { Subject, take } from 'rxjs';

@Component({
selector: 'minsky-login',
Expand All @@ -22,7 +24,7 @@ import { ElectronService } from '@minsky/core';
MatProgressSpinnerModule,
],
})
export class LoginComponent implements OnInit {
export class LoginComponent implements OnInit, OnDestroy {
loginForm = new FormGroup({
email: new FormControl('', [Validators.required, Validators.email]),
password: new FormControl('', [Validators.required]),
Expand All @@ -32,17 +34,40 @@ export class LoginComponent implements OnInit {
errorMessage = '';
isAuthenticated = false;

constructor(private clerkService: ClerkService, private electronService: ElectronService) {}
private destroy$ = new Subject<void>();

constructor(
private clerkService: ClerkService,
private electronService: ElectronService,
private route: ActivatedRoute
) {}
Comment on lines +37 to +43
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

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

destroy$/OnDestroy are currently unused for subscription cleanup (the queryParams subscription uses take(1) and completes automatically). Either remove destroy$ + ngOnDestroy, or switch to takeUntil(this.destroy$) if you intend to keep a long-lived subscription.

Copilot uses AI. Check for mistakes.

async ngOnInit() {
this.route.queryParams.pipe(take(1)).subscribe((params) => {
this.initializeSession(params['authToken']);
});
}

private async initializeSession(authToken: string | undefined) {
try {
await this.clerkService.initialize();

if (authToken) {
await this.clerkService.setSession(authToken);
}

this.isAuthenticated = await this.clerkService.isSignedIn();
Comment on lines 45 to 59
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

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

initializeSession() is async but is invoked from the queryParams subscription without awaiting and without setting an initialization loading state. This allows the user to submit the form before clerkService.initialize() completes, which can cause signInWithEmailPassword() to fail with “Clerk is not initialized”. Consider awaiting initialization in ngOnInit (e.g., via firstValueFrom/await) and/or disabling the form until initialization finishes, and/or calling clerkService.initialize() inside onSubmit() as a guard.

Copilot uses AI. Check for mistakes.
} catch (err) {
this.errorMessage = 'Failed to initialize authentication.';
this.errorMessage = 'Session expired. Please sign in again.';
this.isAuthenticated = false;
}
Comment on lines 60 to 63
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

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

The catch block always shows “Session expired. Please sign in again.” even when the failure is unrelated to session expiry (e.g., Clerk initialization/config/network errors). Consider using a more general message when initialize() fails, and only showing the “session expired” message when a session restore attempt actually fails.

Copilot uses AI. Check for mistakes.
}

ngOnDestroy() {
this.destroy$.next();
this.destroy$.complete();
}

get email() {
return this.loginForm.get('email');
}
Expand Down