feat(admin): Admin settings should only apply if adminControlsApplicable = true and fetch errors should be fatal (#19453)

This commit is contained in:
Shreya Keshive
2026-02-18 17:54:07 -05:00
committed by GitHub
parent 012392ad0a
commit 261788cf91
5 changed files with 79 additions and 72 deletions
+1
View File
@@ -699,6 +699,7 @@ export const AppContainer = (props: AppContainerProps) => {
settings.setValue(scope, 'security.auth.selectedType', authType); settings.setValue(scope, 'security.auth.selectedType', authType);
try { try {
config.setRemoteAdminSettings(undefined);
await config.refreshAuth(authType); await config.refreshAuth(authType);
setAuthState(AuthState.Authenticated); setAuthState(AuthState.Authenticated);
} catch (e) { } catch (e) {
@@ -345,6 +345,7 @@ describe('Admin Controls', () => {
// Should still start polling // Should still start polling
(mockServer.fetchAdminControls as Mock).mockResolvedValue({ (mockServer.fetchAdminControls as Mock).mockResolvedValue({
strictModeDisabled: true, strictModeDisabled: true,
adminControlsApplicable: true,
}); });
await vi.advanceTimersByTimeAsync(5 * 60 * 1000); await vi.advanceTimersByTimeAsync(5 * 60 * 1000);
@@ -363,7 +364,10 @@ describe('Admin Controls', () => {
}); });
it('should fetch from server if no cachedSettings provided', async () => { it('should fetch from server if no cachedSettings provided', async () => {
const serverResponse = { strictModeDisabled: false }; const serverResponse = {
strictModeDisabled: false,
adminControlsApplicable: true,
};
(mockServer.fetchAdminControls as Mock).mockResolvedValue(serverResponse); (mockServer.fetchAdminControls as Mock).mockResolvedValue(serverResponse);
const result = await fetchAdminControls( const result = await fetchAdminControls(
@@ -386,31 +390,24 @@ describe('Admin Controls', () => {
expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(1); expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(1);
}); });
it('should return empty object on fetch error and still start polling', async () => { it('should throw error on fetch error and NOT start polling', async () => {
(mockServer.fetchAdminControls as Mock).mockRejectedValue( const error = new Error('Network error');
new Error('Network error'), (mockServer.fetchAdminControls as Mock).mockRejectedValue(error);
);
const result = await fetchAdminControls(
mockServer,
undefined,
true,
mockOnSettingsChanged,
);
expect(result).toEqual({}); await expect(
fetchAdminControls(mockServer, undefined, true, mockOnSettingsChanged),
).rejects.toThrow(error);
// Polling should have been started and should retry // Polling should NOT have been started
(mockServer.fetchAdminControls as Mock).mockResolvedValue({ // Advance timers just to be absolutely sure
strictModeDisabled: false,
});
await vi.advanceTimersByTimeAsync(5 * 60 * 1000); await vi.advanceTimersByTimeAsync(5 * 60 * 1000);
expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(2); // Initial + poll expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(1); // Only initial fetch
}); });
it('should return empty object on 403 fetch error and STOP polling', async () => { it('should return empty object on adminControlsApplicable false and STOP polling', async () => {
const error403 = new Error('Forbidden'); (mockServer.fetchAdminControls as Mock).mockResolvedValue({
Object.assign(error403, { status: 403 }); adminControlsApplicable: false,
(mockServer.fetchAdminControls as Mock).mockRejectedValue(error403); });
const result = await fetchAdminControls( const result = await fetchAdminControls(
mockServer, mockServer,
@@ -421,7 +418,7 @@ describe('Admin Controls', () => {
expect(result).toEqual({}); expect(result).toEqual({});
// Advance time - should NOT poll because of 403 // Advance time - should NOT poll because of adminControlsApplicable: false
await vi.advanceTimersByTimeAsync(5 * 60 * 1000); await vi.advanceTimersByTimeAsync(5 * 60 * 1000);
expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(1); // Only the initial call expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(1); // Only the initial call
}); });
@@ -430,6 +427,7 @@ describe('Admin Controls', () => {
(mockServer.fetchAdminControls as Mock).mockResolvedValue({ (mockServer.fetchAdminControls as Mock).mockResolvedValue({
strictModeDisabled: false, strictModeDisabled: false,
unknownField: 'bad', unknownField: 'bad',
adminControlsApplicable: true,
}); });
const result = await fetchAdminControls( const result = await fetchAdminControls(
@@ -455,7 +453,9 @@ describe('Admin Controls', () => {
}); });
it('should reset polling interval if called again', async () => { it('should reset polling interval if called again', async () => {
(mockServer.fetchAdminControls as Mock).mockResolvedValue({}); (mockServer.fetchAdminControls as Mock).mockResolvedValue({
adminControlsApplicable: true,
});
// First call // First call
await fetchAdminControls( await fetchAdminControls(
@@ -514,6 +514,7 @@ describe('Admin Controls', () => {
const serverResponse = { const serverResponse = {
strictModeDisabled: true, strictModeDisabled: true,
unknownField: 'should be removed', unknownField: 'should be removed',
adminControlsApplicable: true,
}; };
(mockServer.fetchAdminControls as Mock).mockResolvedValue(serverResponse); (mockServer.fetchAdminControls as Mock).mockResolvedValue(serverResponse);
@@ -532,22 +533,22 @@ describe('Admin Controls', () => {
expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(1); expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(1);
}); });
it('should return empty object on 403 fetch error', async () => { it('should return empty object on adminControlsApplicable false', async () => {
const error403 = new Error('Forbidden'); (mockServer.fetchAdminControls as Mock).mockResolvedValue({
Object.assign(error403, { status: 403 }); adminControlsApplicable: false,
(mockServer.fetchAdminControls as Mock).mockRejectedValue(error403); });
const result = await fetchAdminControlsOnce(mockServer, true); const result = await fetchAdminControlsOnce(mockServer, true);
expect(result).toEqual({}); expect(result).toEqual({});
expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(1); expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(1);
}); });
it('should return empty object on any other fetch error', async () => { it('should throw error on any other fetch error', async () => {
(mockServer.fetchAdminControls as Mock).mockRejectedValue( const error = new Error('Network error');
new Error('Network error'), (mockServer.fetchAdminControls as Mock).mockRejectedValue(error);
await expect(fetchAdminControlsOnce(mockServer, true)).rejects.toThrow(
error,
); );
const result = await fetchAdminControlsOnce(mockServer, true);
expect(result).toEqual({});
expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(1); expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(1);
}); });
@@ -555,7 +556,9 @@ describe('Admin Controls', () => {
const setIntervalSpy = vi.spyOn(global, 'setInterval'); const setIntervalSpy = vi.spyOn(global, 'setInterval');
const clearIntervalSpy = vi.spyOn(global, 'clearInterval'); const clearIntervalSpy = vi.spyOn(global, 'clearInterval');
(mockServer.fetchAdminControls as Mock).mockResolvedValue({}); (mockServer.fetchAdminControls as Mock).mockResolvedValue({
adminControlsApplicable: true,
});
await fetchAdminControlsOnce(mockServer, true); await fetchAdminControlsOnce(mockServer, true);
expect(setIntervalSpy).not.toHaveBeenCalled(); expect(setIntervalSpy).not.toHaveBeenCalled();
@@ -568,6 +571,7 @@ describe('Admin Controls', () => {
// Initial fetch // Initial fetch
(mockServer.fetchAdminControls as Mock).mockResolvedValue({ (mockServer.fetchAdminControls as Mock).mockResolvedValue({
strictModeDisabled: true, strictModeDisabled: true,
adminControlsApplicable: true,
}); });
await fetchAdminControls( await fetchAdminControls(
mockServer, mockServer,
@@ -579,6 +583,7 @@ describe('Admin Controls', () => {
// Update for next poll // Update for next poll
(mockServer.fetchAdminControls as Mock).mockResolvedValue({ (mockServer.fetchAdminControls as Mock).mockResolvedValue({
strictModeDisabled: false, strictModeDisabled: false,
adminControlsApplicable: true,
}); });
// Fast forward // Fast forward
@@ -598,7 +603,10 @@ describe('Admin Controls', () => {
}); });
it('should NOT emit if settings are deeply equal but not the same instance', async () => { it('should NOT emit if settings are deeply equal but not the same instance', async () => {
const settings = { strictModeDisabled: false }; const settings = {
strictModeDisabled: false,
adminControlsApplicable: true,
};
(mockServer.fetchAdminControls as Mock).mockResolvedValue(settings); (mockServer.fetchAdminControls as Mock).mockResolvedValue(settings);
await fetchAdminControls( await fetchAdminControls(
@@ -613,6 +621,7 @@ describe('Admin Controls', () => {
// Next poll returns a different object with the same values // Next poll returns a different object with the same values
(mockServer.fetchAdminControls as Mock).mockResolvedValue({ (mockServer.fetchAdminControls as Mock).mockResolvedValue({
strictModeDisabled: false, strictModeDisabled: false,
adminControlsApplicable: true,
}); });
await vi.advanceTimersByTimeAsync(5 * 60 * 1000); await vi.advanceTimersByTimeAsync(5 * 60 * 1000);
@@ -623,6 +632,7 @@ describe('Admin Controls', () => {
// Initial fetch is successful // Initial fetch is successful
(mockServer.fetchAdminControls as Mock).mockResolvedValue({ (mockServer.fetchAdminControls as Mock).mockResolvedValue({
strictModeDisabled: true, strictModeDisabled: true,
adminControlsApplicable: true,
}); });
await fetchAdminControls( await fetchAdminControls(
mockServer, mockServer,
@@ -643,6 +653,7 @@ describe('Admin Controls', () => {
// Subsequent poll succeeds with new data // Subsequent poll succeeds with new data
(mockServer.fetchAdminControls as Mock).mockResolvedValue({ (mockServer.fetchAdminControls as Mock).mockResolvedValue({
strictModeDisabled: false, strictModeDisabled: false,
adminControlsApplicable: true,
}); });
await vi.advanceTimersByTimeAsync(5 * 60 * 1000); await vi.advanceTimersByTimeAsync(5 * 60 * 1000);
expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(3); expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(3);
@@ -659,10 +670,11 @@ describe('Admin Controls', () => {
}); });
}); });
it('should STOP polling if server returns 403', async () => { it('should STOP polling if server returns adminControlsApplicable false', async () => {
// Initial fetch is successful // Initial fetch is successful
(mockServer.fetchAdminControls as Mock).mockResolvedValue({ (mockServer.fetchAdminControls as Mock).mockResolvedValue({
strictModeDisabled: true, strictModeDisabled: true,
adminControlsApplicable: true,
}); });
await fetchAdminControls( await fetchAdminControls(
mockServer, mockServer,
@@ -672,10 +684,10 @@ describe('Admin Controls', () => {
); );
expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(1); expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(1);
// Next poll returns 403 // Next poll returns adminControlsApplicable: false
const error403 = new Error('Forbidden'); (mockServer.fetchAdminControls as Mock).mockResolvedValue({
Object.assign(error403, { status: 403 }); adminControlsApplicable: false,
(mockServer.fetchAdminControls as Mock).mockRejectedValue(error403); });
await vi.advanceTimersByTimeAsync(5 * 60 * 1000); await vi.advanceTimersByTimeAsync(5 * 60 * 1000);
expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(2); expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(2);
@@ -688,7 +700,9 @@ describe('Admin Controls', () => {
describe('stopAdminControlsPolling', () => { describe('stopAdminControlsPolling', () => {
it('should stop polling after it has started', async () => { it('should stop polling after it has started', async () => {
(mockServer.fetchAdminControls as Mock).mockResolvedValue({}); (mockServer.fetchAdminControls as Mock).mockResolvedValue({
adminControlsApplicable: true,
});
// Start polling // Start polling
await fetchAdminControls( await fetchAdminControls(
@@ -80,15 +80,6 @@ export function sanitizeAdminSettings(
}; };
} }
function isGaxiosError(error: unknown): error is { status: number } {
return (
typeof error === 'object' &&
error !== null &&
'status' in error &&
typeof (error as { status: unknown }).status === 'number'
);
}
/** /**
* Fetches the admin controls from the server if enabled by experiment flag. * Fetches the admin controls from the server if enabled by experiment flag.
* Safely handles polling start/stop based on the flag and server availability. * Safely handles polling start/stop based on the flag and server availability.
@@ -113,7 +104,7 @@ export async function fetchAdminControls(
// If we already have settings (e.g. from IPC during relaunch), use them // If we already have settings (e.g. from IPC during relaunch), use them
// to avoid blocking startup with another fetch. We'll still start polling. // to avoid blocking startup with another fetch. We'll still start polling.
if (cachedSettings) { if (cachedSettings && Object.keys(cachedSettings).length !== 0) {
currentSettings = cachedSettings; currentSettings = cachedSettings;
startAdminControlsPolling(server, server.projectId, onSettingsChanged); startAdminControlsPolling(server, server.projectId, onSettingsChanged);
return cachedSettings; return cachedSettings;
@@ -123,22 +114,20 @@ export async function fetchAdminControls(
const rawSettings = await server.fetchAdminControls({ const rawSettings = await server.fetchAdminControls({
project: server.projectId, project: server.projectId,
}); });
if (rawSettings.adminControlsApplicable !== true) {
stopAdminControlsPolling();
currentSettings = undefined;
return {};
}
const sanitizedSettings = sanitizeAdminSettings(rawSettings); const sanitizedSettings = sanitizeAdminSettings(rawSettings);
currentSettings = sanitizedSettings; currentSettings = sanitizedSettings;
startAdminControlsPolling(server, server.projectId, onSettingsChanged); startAdminControlsPolling(server, server.projectId, onSettingsChanged);
return sanitizedSettings; return sanitizedSettings;
} catch (e) { } catch (e) {
// Non-enterprise users don't have access to fetch settings.
if (isGaxiosError(e) && e.status === 403) {
stopAdminControlsPolling();
currentSettings = undefined;
return {};
}
debugLogger.error('Failed to fetch admin controls: ', e); debugLogger.error('Failed to fetch admin controls: ', e);
// If initial fetch fails, start polling to retry. throw e;
currentSettings = {};
startAdminControlsPolling(server, server.projectId, onSettingsChanged);
return {};
} }
} }
@@ -162,17 +151,18 @@ export async function fetchAdminControlsOnce(
const rawSettings = await server.fetchAdminControls({ const rawSettings = await server.fetchAdminControls({
project: server.projectId, project: server.projectId,
}); });
return sanitizeAdminSettings(rawSettings);
} catch (e) { if (rawSettings.adminControlsApplicable !== true) {
// Non-enterprise users don't have access to fetch settings.
if (isGaxiosError(e) && e.status === 403) {
return {}; return {};
} }
return sanitizeAdminSettings(rawSettings);
} catch (e) {
debugLogger.error( debugLogger.error(
'Failed to fetch admin controls: ', 'Failed to fetch admin controls: ',
e instanceof Error ? e.message : e, e instanceof Error ? e.message : e,
); );
return {}; throw e;
} }
} }
@@ -192,6 +182,13 @@ function startAdminControlsPolling(
const rawSettings = await server.fetchAdminControls({ const rawSettings = await server.fetchAdminControls({
project, project,
}); });
if (rawSettings.adminControlsApplicable !== true) {
stopAdminControlsPolling();
currentSettings = undefined;
return;
}
const newSettings = sanitizeAdminSettings(rawSettings); const newSettings = sanitizeAdminSettings(rawSettings);
if (!isDeepStrictEqual(newSettings, currentSettings)) { if (!isDeepStrictEqual(newSettings, currentSettings)) {
@@ -199,12 +196,6 @@ function startAdminControlsPolling(
onSettingsChanged(newSettings); onSettingsChanged(newSettings);
} }
} catch (e) { } catch (e) {
// Non-enterprise users don't have access to fetch settings.
if (isGaxiosError(e) && e.status === 403) {
stopAdminControlsPolling();
currentSettings = undefined;
return;
}
debugLogger.error('Failed to poll admin controls: ', e); debugLogger.error('Failed to poll admin controls: ', e);
} }
}, },
+1
View File
@@ -355,4 +355,5 @@ export const FetchAdminControlsResponseSchema = z.object({
strictModeDisabled: z.boolean().optional(), strictModeDisabled: z.boolean().optional(),
mcpSetting: McpSettingSchema.optional(), mcpSetting: McpSettingSchema.optional(),
cliFeatureSetting: CliFeatureSettingSchema.optional(), cliFeatureSetting: CliFeatureSettingSchema.optional(),
adminControlsApplicable: z.boolean().optional(),
}); });
+1 -1
View File
@@ -1171,7 +1171,7 @@ export class Config {
return this.remoteAdminSettings; return this.remoteAdminSettings;
} }
setRemoteAdminSettings(settings: AdminControlsSettings): void { setRemoteAdminSettings(settings: AdminControlsSettings | undefined): void {
this.remoteAdminSettings = settings; this.remoteAdminSettings = settings;
} }