From d40457bc33593babc3b51495e52da606785b88ae Mon Sep 17 00:00:00 2001 From: Paul Fitzpatrick Date: Wed, 8 May 2024 21:55:39 -0400 Subject: [PATCH] add more tests --- app/client/boot.ts | 7 +++++-- app/client/models/AdminChecks.ts | 35 +++++++++++++------------------- app/client/ui/AdminPanel.ts | 27 ++++++++++++++++++++++-- app/common/BaseAPI.ts | 15 ++++++++++---- app/common/InstallAPI.ts | 11 ++++++++++ app/server/lib/Authorizer.ts | 2 ++ app/server/lib/BootProbes.ts | 6 +++--- app/server/lib/FlexServer.ts | 16 +++++++++++---- test/nbrowser/AdminPanel.ts | 31 +++++++++++++++++++++------- 9 files changed, 107 insertions(+), 43 deletions(-) diff --git a/app/client/boot.ts b/app/client/boot.ts index 4da8a650..760ed750 100644 --- a/app/client/boot.ts +++ b/app/client/boot.ts @@ -1,10 +1,11 @@ -import { AppModel } from 'app/client/models/AppModel'; +import { AppModel, getHomeUrl } from 'app/client/models/AppModel'; import { AdminChecks } from 'app/client/models/AdminChecks'; import { AdminPanel } from 'app/client/ui/AdminPanel'; import { createAppPage } from 'app/client/ui/createAppPage'; import { pagePanels } from 'app/client/ui/PagePanels'; import { getGristConfig } from 'app/common/urlUtils'; import { Disposable, dom, Observable, styled, UseCBOwner } from 'grainjs'; +import { InstallAPI, InstallAPIImpl } from 'app/common/InstallAPI'; const cssBody = styled('div', ` padding: 20px; @@ -38,12 +39,14 @@ export class Boot extends Disposable { private _checks: AdminChecks; + private readonly _installAPI: InstallAPI = new InstallAPIImpl(getHomeUrl()); + constructor(private _appModel: AppModel) { super(); // Setting title in constructor seems to be how we are doing this, // based on other similar pages. document.title = 'Booting Grist'; - this._checks = new AdminChecks(this); + this._checks = new AdminChecks(this, this._installAPI); } /** diff --git a/app/client/models/AdminChecks.ts b/app/client/models/AdminChecks.ts index da720306..40baf812 100644 --- a/app/client/models/AdminChecks.ts +++ b/app/client/models/AdminChecks.ts @@ -1,5 +1,5 @@ import { BootProbeIds, BootProbeInfo, BootProbeResult } from 'app/common/BootProbe'; -import { removeTrailingSlash } from 'app/common/gutil'; +import { InstallAPI } from 'app/common/InstallAPI'; import { getGristConfig } from 'app/common/urlUtils'; import { Disposable, Observable, UseCBOwner } from 'grainjs'; @@ -19,7 +19,7 @@ export class AdminChecks { // Keep track of probe results we have received, by probe ID. private _results: Map>; - constructor(private _parent: Disposable) { + constructor(private _parent: Disposable, private _installAPI: InstallAPI) { this.probes = Observable.create(_parent, []); this._results = new Map(); this._requests = new Map(); @@ -32,20 +32,14 @@ export class AdminChecks { const config = getGristConfig(); const errMessage = config.errMessage; if (!errMessage) { - // Probe tool URLs are relative to the current URL. Don't trust configuration, - // because it may be buggy if the user is here looking at the boot page - // to figure out some problem. - // - // We have been careful to make URLs available with appropriate - // middleware relative to both of the admin panel and the boot page. - const url = new URL(removeTrailingSlash(document.location.href)); - url.pathname += '/probe'; - const resp = await fetch(url.href); - const _probes = await resp.json(); + const _probes = await this._installAPI.getChecks().catch(() => undefined); if (!this._parent.isDisposed()) { - this.probes.set(_probes.probes); + // Currently, no probes are allowed if not admin. + // May want to relax this to allow some probes that help + // diagnose some initial auth problems. + this.probes.set(_probes ? _probes.probes : []); } - return _probes.probes; + return _probes; } return []; } @@ -63,7 +57,7 @@ export class AdminChecks { } let request = this._requests.get(id); if (!request) { - request = new AdminCheckRunner(id, this._results, this._parent); + request = new AdminCheckRunner(this._installAPI, id, this._results, this._parent); this._requests.set(id, request); } request.start(); @@ -97,16 +91,15 @@ export interface AdminCheckRequest { * Manage a single check. */ export class AdminCheckRunner { - constructor(public id: string, public results: Map>, + constructor(private _installAPI: InstallAPI, + public id: string, + public results: Map>, public parent: Disposable) { - const url = new URL(removeTrailingSlash(document.location.href)); - url.pathname = url.pathname + '/probe/' + id; - fetch(url.href).then(async resp => { - const _probes: BootProbeResult = await resp.json(); + this._installAPI.runCheck(id).then(async result => { if (parent.isDisposed()) { return; } const ob = results.get(id); if (ob) { - ob.set(_probes); + ob.set(result); } }).catch(e => console.error(e)); } diff --git a/app/client/ui/AdminPanel.ts b/app/client/ui/AdminPanel.ts index 464eefdb..8b5df5a6 100644 --- a/app/client/ui/AdminPanel.ts +++ b/app/client/ui/AdminPanel.ts @@ -42,7 +42,7 @@ export class AdminPanel extends Disposable { constructor(private _appModel: AppModel, private _fullScreen: boolean = false) { super(); document.title = getAdminPanelName() + getPageTitleSuffix(getGristConfig()); - this._checks = new AdminChecks(this); + this._checks = new AdminChecks(this, this._installAPI); } public buildDom() { @@ -50,7 +50,7 @@ export class AdminPanel extends Disposable { reportError(err); }); if (this._fullScreen) { - return dom.create(this._buildMainContent.bind(this)); + return dom.create(this._buildMainContentForAdmin.bind(this)); } const panelOpen = Observable.create(this, false); return pagePanels({ @@ -82,6 +82,29 @@ export class AdminPanel extends Disposable { } private _buildMainContent(owner: MultiHolder) { + return dom.maybe(this._checks.probes, probes => { + return probes.length > 0 + ? this._buildMainContentForAdmin(owner) + : this._buildMainContentForOthers(owner); + }); + } + + private _buildMainContentForOthers(owner: MultiHolder) { + return cssPageContainer( + dom.cls('clipboard'), + {tabIndex: "-1"}, + cssSection( + cssSectionTitle(t('Admin Page Unavailable')), + ` +You are not logged in as an administrator. +If logging in is broken, you can set GRIST_BOOT_KEY=secret in +the environment and visit /admin?key=secret.` + ), + testId('admin-panel'), + ); + } + + private _buildMainContentForAdmin(owner: MultiHolder) { return cssPageContainer( dom.cls('clipboard'), {tabIndex: "-1"}, diff --git a/app/common/BaseAPI.ts b/app/common/BaseAPI.ts index 17d9ff69..8c17def0 100644 --- a/app/common/BaseAPI.ts +++ b/app/common/BaseAPI.ts @@ -61,10 +61,17 @@ export class BaseAPI { 'X-Requested-With': 'XMLHttpRequest', ...options.headers }; - if (typeof window !== 'undefined' && (window as any)?.isGristBootPage) { - const parts = (new URL(window.location.href).pathname).split('/'); - if (parts[0] === '' && parts[1] === 'boot' && parts[2] !== undefined) { - this._headers['X-Boot-Key'] = parts[2]; + if (typeof window !== 'undefined') { + const url = new URL(window.location.href); + if ((window as any)?.isGristBootPage) { + const parts = (url.pathname).split('/'); + if (parts[0] === '' && parts[1] === 'boot' && parts[2] !== undefined) { + this._headers['X-Boot-Key'] = parts[2]; + } + } + const bootKey = url.searchParams.get('boot'); + if (bootKey) { + this._headers['X-Boot-Key'] = bootKey; } } this._extraParameters = options.extraParameters; diff --git a/app/common/InstallAPI.ts b/app/common/InstallAPI.ts index 88cc35a5..b71b1389 100644 --- a/app/common/InstallAPI.ts +++ b/app/common/InstallAPI.ts @@ -1,4 +1,5 @@ import {BaseAPI, IOptions} from 'app/common/BaseAPI'; +import {BootProbeInfo, BootProbeResult} from 'app/common/BootProbe'; import {InstallPrefs} from 'app/common/Install'; import {TelemetryLevel} from 'app/common/Telemetry'; import {addCurrentOrgToPath} from 'app/common/urlUtils'; @@ -56,6 +57,8 @@ export interface InstallAPI { * Returns information about latest version of Grist */ checkUpdates(): Promise; + getChecks(): Promise<{probes: BootProbeInfo[]}>; + runCheck(id: string): Promise; } export class InstallAPIImpl extends BaseAPI implements InstallAPI { @@ -78,6 +81,14 @@ export class InstallAPIImpl extends BaseAPI implements InstallAPI { return this.requestJson(`${this._url}/api/install/updates`, {method: 'GET'}); } + getChecks(): Promise<{probes: BootProbeInfo[]}> { + return this.requestJson(`${this._url}/api/probes`, {method: 'GET'}); + } + + runCheck(id: string): Promise { + return this.requestJson(`${this._url}/api/probes/${id}`, {method: 'GET'}); + } + private get _url(): string { return addCurrentOrgToPath(this._homeUrl); } diff --git a/app/server/lib/Authorizer.ts b/app/server/lib/Authorizer.ts index 296ed0a7..c32bf8de 100644 --- a/app/server/lib/Authorizer.ts +++ b/app/server/lib/Authorizer.ts @@ -203,7 +203,9 @@ export async function addRequestUser( const user = await dbManager.getUser(userId); mreq.user = user; mreq.userId = userId; + mreq.users = [dbManager.makeFullUser(user!)]; mreq.userIsAuthorized = true; + authDone = true; } // Special permission header for internal housekeeping tasks diff --git a/app/server/lib/BootProbes.ts b/app/server/lib/BootProbes.ts index 31a72766..6c21c8a0 100644 --- a/app/server/lib/BootProbes.ts +++ b/app/server/lib/BootProbes.ts @@ -25,7 +25,7 @@ export class BootProbes { public addEndpoints() { // Return a list of available probes. - this._app.use(`${this._base}/probe$`, + this._app.use(`${this._base}/probes$`, ...this._middleware, expressWrap(async (_, res) => { res.json({ @@ -36,7 +36,7 @@ export class BootProbes { })); // Return result of running an individual probe. - this._app.use(`${this._base}/probe/:probeId`, + this._app.use(`${this._base}/probes/:probeId`, ...this._middleware, expressWrap(async (req, res) => { const probe = this._probeById.get(req.params.probeId); @@ -48,7 +48,7 @@ export class BootProbes { })); // Fall-back for errors. - this._app.use(`${this._base}/probe`, jsonErrorHandler); + this._app.use(`${this._base}/probes`, jsonErrorHandler); } private _addProbes() { diff --git a/app/server/lib/FlexServer.ts b/app/server/lib/FlexServer.ts index 733c146e..ab0b326e 100644 --- a/app/server/lib/FlexServer.ts +++ b/app/server/lib/FlexServer.ts @@ -1859,19 +1859,27 @@ export class FlexServer implements GristServer { const requireInstallAdmin = this.getInstallAdmin().getMiddlewareRequireAdmin(); - const adminPageMiddleware = [ - this._redirectToHostMiddleware, + const adminPageFullMiddleware = [ this._userIdMiddleware, - this._redirectToLoginWithoutExceptionsMiddleware, // In principle, it may be safe to show the Admin Panel to non-admins but let's protect it // since it's intended for admins, and it's easier not to have to worry how it should behave // for others. requireInstallAdmin, ]; + + const adminPageMiddleware = [ + // this._redirectToHostMiddleware, + this._userIdMiddleware, + // this._redirectToLoginWithoutExceptionsMiddleware, + // In principle, it may be safe to show the Admin Panel to non-admins but let's protect it + // since it's intended for admins, and it's easier not to have to worry how it should behave + // for others. + // requireInstallAdmin, + ]; this.app.get('/admin', ...adminPageMiddleware, expressWrap(async (req, resp) => { return this.sendAppPage(req, resp, {path: 'app.html', status: 200, config: {}}); })); - const probes = new BootProbes(this.app, this, '/admin', adminPageMiddleware); + const probes = new BootProbes(this.app, this, '/api', adminPageFullMiddleware); probes.addEndpoints(); // Restrict this endpoint to install admins too, for the same reason as the /admin page. diff --git a/test/nbrowser/AdminPanel.ts b/test/nbrowser/AdminPanel.ts index 7cfcd78a..c16715f8 100644 --- a/test/nbrowser/AdminPanel.ts +++ b/test/nbrowser/AdminPanel.ts @@ -31,7 +31,7 @@ describe('AdminPanel', function() { await server.restart(true); }); - it('should not be shown to non-managers', async function() { + it('should show an explanation to non-managers', async function() { session = await gu.session().user('user2').personalSite.login(); await session.loadDocMenu('/'); @@ -42,8 +42,9 @@ describe('AdminPanel', function() { // Try loading the URL directly. await driver.get(`${server.getHost()}/admin`); - assert.match(await driver.findWait('.test-error-header', 2000).getText(), /Access denied/); - assert.equal(await driver.find('.test-admin-panel').isPresent(), false); + await waitForAdminPanel(); + assert.equal(await driver.find('.test-admin-panel').isDisplayed(), true); + assert.match(await driver.find('.test-admin-panel').getText(), /not logged in/); }); it('should be shown to managers', async function() { @@ -330,14 +331,30 @@ describe('AdminPanel', function() { }); it('should survive APP_HOME_URL misconfiguration', async function() { - // TODO: this works in theory, but admin page is in practice hard - // to access unless other pages work (e.g. to log in). So falling - // back on boot page for now. process.env.APP_HOME_URL = 'http://misconfigured.invalid'; process.env.GRIST_BOOT_KEY = 'zig'; await server.restart(true); - await driver.get(`${server.getHost()}/boot/zig`); + await driver.get(`${server.getHost()}/admin`); + await waitForAdminPanel(); + }); + + it('should honor GRIST_BOOT_KEY fallback', async function() { + await gu.removeLogin(); + await driver.get(`${server.getHost()}/admin`); + await waitForAdminPanel(); + assert.equal(await driver.find('.test-admin-panel').isDisplayed(), true); + assert.match(await driver.find('.test-admin-panel').getText(), /not logged in/); + + process.env.GRIST_BOOT_KEY = 'zig'; + await server.restart(true); + await driver.get(`${server.getHost()}/admin?boot=zig`); + await waitForAdminPanel(); + assert.equal(await driver.find('.test-admin-panel').isDisplayed(), true); + assert.notMatch(await driver.find('.test-admin-panel').getText(), /not logged in/); + await driver.get(`${server.getHost()}/admin?boot=zig-wrong`); await waitForAdminPanel(); + assert.equal(await driver.find('.test-admin-panel').isDisplayed(), true); + assert.match(await driver.find('.test-admin-panel').getText(), /not logged in/); }); });