Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 7 additions & 10 deletions frontend/src/app/components/setup/setup.component.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { BrowserAnimationsModule } from '@angular/platform-browser/animations';
import { provideRouter, Router } from '@angular/router';
import { IPasswordStrengthMeterService } from 'angular-password-strength-meter';
import { Angulartics2Module } from 'angulartics2';
import { of } from 'rxjs';
import { SelfhostedService } from 'src/app/services/selfhosted.service';
import { SetupComponent } from './setup.component';

Expand Down Expand Up @@ -53,39 +52,37 @@ describe('SetupComponent', () => {
expect(testable.password()).toBe('SecurePass123');
});

it('should not submit if email is empty', () => {
it('should not submit if email is empty', async () => {
const testable = component as SetupComponentTestable;
const fakeCreateInitialUser = vi.spyOn(selfhostedService, 'createInitialUser');

testable.email.set('');
testable.password.set('SecurePass123');

component.createAdminAccount();
await component.createAdminAccount();
expect(fakeCreateInitialUser).not.toHaveBeenCalled();
});

it('should not submit if password is empty', () => {
it('should not submit if password is empty', async () => {
const testable = component as SetupComponentTestable;
const fakeCreateInitialUser = vi.spyOn(selfhostedService, 'createInitialUser');

testable.email.set('test@example.com');
testable.password.set('');

component.createAdminAccount();
await component.createAdminAccount();
expect(fakeCreateInitialUser).not.toHaveBeenCalled();
});

it('should create admin account and navigate to login on success', () => {
it('should create admin account and navigate to login on success', async () => {
const testable = component as SetupComponentTestable;
const fakeCreateInitialUser = vi
.spyOn(selfhostedService, 'createInitialUser')
.mockReturnValue(of({ success: true }));
const fakeCreateInitialUser = vi.spyOn(selfhostedService, 'createInitialUser').mockResolvedValue({ success: true });
const fakeNavigate = vi.spyOn(router, 'navigate').mockResolvedValue(true);

testable.email.set('admin@example.com');
testable.password.set('SecurePass123');

component.createAdminAccount();
await component.createAdminAccount();

expect(fakeCreateInitialUser).toHaveBeenCalledWith({
email: 'admin@example.com',
Expand Down
24 changes: 9 additions & 15 deletions frontend/src/app/components/setup/setup.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,29 +41,23 @@ export class SetupComponent {
this.password.set(value);
}

createAdminAccount(): void {
async createAdminAccount(): Promise<void> {

Copilot AI Feb 3, 2026

Copy link

Choose a reason for hiding this comment

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

The password field validation is delegated entirely to the UserPasswordComponent, but there's no explicit validation in the setup component itself. While the submit button checks for empty values (!this.email() || !this.password()), it doesn't verify password strength requirements. Consider whether password strength validation should be enforced at the component level in addition to any validation performed by UserPasswordComponent, or add a comment clarifying that password validation is handled by the child component.

Suggested change
async createAdminAccount(): Promise<void> {
async createAdminAccount(): Promise<void> {
// Password strength and detailed validation are handled by UserPasswordComponent
// and on the server side; here we only ensure that values are present.

Copilot uses AI. Check for mistakes.
if (!this.email() || !this.password()) {
return;
}

this.submitting.set(true);

this._selfhostedService
.createInitialUser({
try {
await this._selfhostedService.createInitialUser({
email: this.email(),
password: this.password(),
})
.subscribe({
next: () => {
this.submitting.set(false);
this._router.navigate(['/login']);
},
error: () => {
this.submitting.set(false);
},
complete: () => {
this.submitting.set(false);
},
});
this._router.navigate(['/login']);
} catch {
// Error handling is done in the service
} finally {
this.submitting.set(false);
}
}
}
24 changes: 10 additions & 14 deletions frontend/src/app/guards/configuration.guard.ts
Original file line number Diff line number Diff line change
@@ -1,40 +1,36 @@
import { inject } from '@angular/core';
import { CanActivateFn, Router } from '@angular/router';
import { map, of } from 'rxjs';
import { SelfhostedService } from '../services/selfhosted.service';

/**
* Guard that protects the /login route.
* In self-hosted mode, redirects to /setup if the app is not configured.
* In SaaS mode, allows access immediately.
*/
export const configurationGuard: CanActivateFn = () => {
export const configurationGuard: CanActivateFn = async () => {
const selfhostedService = inject(SelfhostedService);
const router = inject(Router);

// In SaaS mode, always allow access to login
if (!selfhostedService.isSelfHosted()) {
return of(true);
return true;
}

// If we already know the configuration state, use it
const currentState = selfhostedService.isConfigured();
if (currentState !== null) {
if (currentState) {
return of(true);
return true;
} else {
return of(router.createUrlTree(['/setup']));
return router.createUrlTree(['/setup']);
}
}

// Check configuration from the server
return selfhostedService.checkConfiguration().pipe(
map((response) => {
if (response.isConfigured) {
return true;
} else {
return router.createUrlTree(['/setup']);
}
}),
);
const response = await selfhostedService.checkConfiguration();
Comment on lines 19 to +30

Copilot AI Feb 3, 2026

Copy link

Choose a reason for hiding this comment

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

There's a potential race condition if a user opens multiple tabs or if the guard is triggered multiple times simultaneously. Both guards cache the configuration state and make API calls if the state is null. However, there's no mechanism to prevent multiple concurrent API calls to checkConfiguration() when the state is null. Consider implementing request deduplication or a loading flag to ensure only one API call is made at a time.

Copilot uses AI. Check for mistakes.
if (response.isConfigured) {
return true;
} else {
return router.createUrlTree(['/setup']);
}
};
Comment on lines 1 to 36

Copilot AI Feb 3, 2026

Copy link

Choose a reason for hiding this comment

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

The guards setup.guard.ts and configuration.guard.ts lack test coverage. The codebase has a test file for auth.guard.ts (auth.guard.spec.ts), indicating that guards should have test coverage. These guards should be tested to ensure proper routing behavior in different scenarios (SaaS mode, self-hosted configured, self-hosted unconfigured).

Copilot uses AI. Check for mistakes.
28 changes: 12 additions & 16 deletions frontend/src/app/guards/setup.guard.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { inject } from '@angular/core';
import { CanActivateFn, Router } from '@angular/router';
import { map, of } from 'rxjs';
import { SelfhostedService } from '../services/selfhosted.service';

/**
Expand All @@ -9,37 +8,34 @@ import { SelfhostedService } from '../services/selfhosted.service';
* - In self-hosted mode, redirects to /login if already configured
* - Allows access to /setup only if self-hosted and not configured
*/
export const setupGuard: CanActivateFn = () => {
export const setupGuard: CanActivateFn = async () => {
const selfhostedService = inject(SelfhostedService);
const router = inject(Router);

// In SaaS mode, redirect to login (setup is only for self-hosted)
if (!selfhostedService.isSelfHosted()) {
return of(router.createUrlTree(['/login']));
return router.createUrlTree(['/login']);
}

// If we already know the configuration state, use it
const currentState = selfhostedService.isConfigured();
if (currentState !== null) {
if (currentState) {
// Already configured, redirect to login
return of(router.createUrlTree(['/login']));
return router.createUrlTree(['/login']);
} else {
// Not configured, allow access to setup
return of(true);
return true;
}
}

// Check configuration from the server
return selfhostedService.checkConfiguration().pipe(
map((response) => {
if (response.isConfigured) {
// Already configured, redirect to login
return router.createUrlTree(['/login']);
} else {
// Not configured, allow access to setup
return true;
}
}),
);
const response = await selfhostedService.checkConfiguration();
Comment on lines 20 to +33

Copilot AI Feb 3, 2026

Copy link

Choose a reason for hiding this comment

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

There's a potential race condition if a user opens multiple tabs or if the guard is triggered multiple times simultaneously. Both guards cache the configuration state and make API calls if the state is null. However, there's no mechanism to prevent multiple concurrent API calls to checkConfiguration() when the state is null. Consider implementing request deduplication or a loading flag to ensure only one API call is made at a time.

Copilot uses AI. Check for mistakes.
if (response.isConfigured) {
// Already configured, redirect to login
return router.createUrlTree(['/login']);
} else {
// Not configured, allow access to setup
return true;
}
};
91 changes: 51 additions & 40 deletions frontend/src/app/services/selfhosted.service.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
import { HttpClient } from '@angular/common/http';
import { computed, Injectable, inject, signal } from '@angular/core';
import { catchError, EMPTY, map, Observable, tap } from 'rxjs';
import { environment } from 'src/environments/environment';
import { AlertActionType, AlertType } from '../models/alert';
import { NotificationsService } from './notifications.service';
Expand All @@ -22,7 +20,6 @@ export interface CreateInitialUserResponse {
providedIn: 'root',
})
export class SelfhostedService {
private _http = inject(HttpClient);
private _notifications = inject(NotificationsService);

private _isConfigured = signal<boolean | null>(null);
Expand All @@ -32,46 +29,60 @@ export class SelfhostedService {
public readonly isCheckingConfiguration = this._isCheckingConfiguration.asReadonly();
public readonly isSelfHosted = computed(() => !(environment as any).saas);

checkConfiguration(): Observable<IsConfiguredResponse> {
async checkConfiguration(): Promise<IsConfiguredResponse> {
this._isCheckingConfiguration.set(true);
return this._http.get<IsConfiguredResponse>('/selfhosted/is-configured').pipe(
tap((response) => {
this._isConfigured.set(response.isConfigured);
this._isCheckingConfiguration.set(false);
}),
catchError((err) => {
console.error('Failed to check configuration:', err);
this._isCheckingConfiguration.set(false);
// If the endpoint fails, assume configured to avoid blocking login
this._isConfigured.set(true);
return EMPTY;
}),
);
try {
const response = await fetch('/api/selfhosted/is-configured');
if (!response.ok) {
throw new Error(`HTTP error: ${response.status}`);
}
const data: IsConfiguredResponse = await response.json();
this._isConfigured.set(data.isConfigured);
this._isCheckingConfiguration.set(false);
return data;
} catch (err) {
console.error('Failed to check configuration:', err);
this._isCheckingConfiguration.set(false);
// If the endpoint fails, assume configured to avoid blocking login
this._isConfigured.set(true);
return { isConfigured: true };
}
Comment on lines +43 to +49

Copilot AI Feb 3, 2026

Copy link

Choose a reason for hiding this comment

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

In the error catch block, the service assumes configured state (line 47) when the configuration check fails. This is a fail-open approach that could allow access to the login page even when the backend is unreachable or has an issue. The comment on line 46 says this is to "avoid blocking login", but in a self-hosted setup scenario, this could lead to a confusing user experience where the login page is accessible but the actual login might fail. Consider whether this fallback behavior is appropriate, or if it should fail more gracefully (e.g., show a specific error page or retry logic).

Copilot uses AI. Check for mistakes.
}

createInitialUser(userData: CreateInitialUserRequest): Observable<CreateInitialUserResponse> {
return this._http.post<CreateInitialUserResponse>('/selfhosted/initial-user', userData).pipe(
map((res) => {
this._notifications.showSuccessSnackbar('Admin account created successfully.');
this._isConfigured.set(true);
return res;
}),
catchError((err) => {
console.error('Failed to create initial user:', err);
this._notifications.showAlert(
AlertType.Error,
{ abstract: err.error?.message || err.message, details: err.error?.originalMessage },
[
{
type: AlertActionType.Button,
caption: 'Dismiss',
action: () => this._notifications.dismissAlert(),
},
],
);
return EMPTY;
}),
);
async createInitialUser(userData: CreateInitialUserRequest): Promise<CreateInitialUserResponse> {
try {
const response = await fetch('/api/selfhosted/initial-user', {
method: 'POST',
headers: {
'Content-Type': 'application/json',
},
body: JSON.stringify(userData),
});
Comment on lines +35 to +60

Copilot AI Feb 3, 2026

Copy link

Choose a reason for hiding this comment

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

The selfhosted.service uses the native fetch API instead of Angular's HttpClient, which is inconsistent with the rest of the codebase. All other services in this application (auth.service.ts, company.service.ts, connections.service.ts, etc.) use HttpClient for HTTP requests. Using HttpClient provides benefits such as:

  • Automatic integration with Angular's HttpInterceptor (including token.interceptor.ts)
  • Better testability with HttpClientTestingModule
  • RxJS observable integration
  • Consistent error handling patterns

Consider refactoring to use HttpClient to maintain consistency with the established patterns in the codebase.

Copilot uses AI. Check for mistakes.

if (!response.ok) {
const errorData = await response.json().catch(() => ({}));
throw { error: errorData, message: `HTTP error: ${response.status}` };
}

const data: CreateInitialUserResponse = await response.json();
this._notifications.showSuccessSnackbar('Admin account created successfully.');
this._isConfigured.set(true);
return data;
} catch (err: any) {
console.error('Failed to create initial user:', err);
this._notifications.showAlert(
AlertType.Error,
{ abstract: err.error?.message || err.message, details: err.error?.originalMessage },
[
{
type: AlertActionType.Button,
caption: 'Dismiss',
action: () => this._notifications.dismissAlert(),
},
],
);
throw err;
}
}

resetConfigurationState(): void {
Expand Down
Loading