Use actions & write tests for GET /v1/apps/:appId

Signed-off-by: Christina Ying Wang <christina@balena.io>

src/device-api/actions.ts

@@ -6,6 +6,7 @@ import * as messages from './messages';
 import * as eventTracker from '../event-tracker';
 import * as deviceState from '../device-state';
 import * as logger from '../logger';
+import * as config from '../config';
 import { App } from '../compose/app';
 import * as applicationManager from '../compose/application-manager';
 import * as serviceManager from '../compose/service-manager';
@@ -14,11 +15,13 @@ import {
 	CompositionStepAction,
 	generateStep,
 } from '../compose/composition-steps';
+import * as commitStore from '../compose/commit';
 import { getApp } from '../device-state/db-format';
 import * as TargetState from '../device-state/target-state';
 import log from '../lib/supervisor-console';
 import blink = require('../lib/blink');
 import { lock } from '../lib/update-lock';
+import * as constants from '../lib/constants';
 import {
 	InternalInconsistencyError,
 	NotFoundError,
@@ -416,3 +419,37 @@ export const updateTarget = async (force: boolean = false) => {
 	);
 	return false;
 };
+
+/**
+ * Get application information for a single-container app, throwing if multicontainer
+ * Used by:
+ * - GET /v1/apps/:appId
+ */
+export const getSingleContainerApp = async (appId: number) => {
+	eventTracker.track('GET app (v1)', { appId });
+	const apps = await applicationManager.getCurrentApps();
+	const app = apps[appId];
+	const service = app?.services?.[0];
+	if (service == null) {
+		// This should return a 404 Not Found, but we can't change the interface now so keep it as a 400
+		throw new BadRequestError('App not found');
+	}
+	if (app.services.length > 1) {
+		throw new BadRequestError(
+			'Some v1 endpoints are only allowed on single-container apps',
+		);
+	}
+
+	// Because we only have a single app, we can fetch the commit for that
+	// app, and maintain backwards compatability
+	const commit = await commitStore.getCommitForApp(appId);
+
+	return {
+		appId,
+		commit,
+		containerId: service.containerId,
+		env: _.omit(service.config.environment, constants.privateAppEnvVars),
+		imageId: service.config.image,
+		releaseId: service.releaseId,
+	};
+};

src/device-api/v1.ts

@@ -17,9 +17,7 @@ import {
 	isBadRequestError,
 } from '../lib/errors';
 import * as hostConfig from '../host-config';
-import * as applicationManager from '../compose/application-manager';
 import { CompositionStepAction } from '../compose/composition-steps';
-import * as commitStore from '../compose/commit';
 
 const disallowedHostConfigPatchFields = ['local_ip', 'local_port'];
 
@@ -107,49 +105,25 @@ router.post('/v1/shutdown', handleDeviceAction('shutdown'));
 
 router.get('/v1/apps/:appId', async (req: AuthorizedRequest, res, next) => {
 	const appId = checkInt(req.params.appId);
-	eventTracker.track('GET app (v1)', { appId });
 	if (appId == null) {
 		return res.status(400).send('Missing app id');
 	}
 
+	// handle the case where the appId is out of scope
+	if (!req.auth.isScoped({ apps: [appId] })) {
+		return res.status(401).json({
+			status: 'failed',
+			message: 'Application is not available',
+		});
+	}
+
 	try {
-		const apps = await applicationManager.getCurrentApps();
-		const app = apps[appId];
-		const service = app?.services?.[0];
-		if (service == null) {
-			return res.status(400).send('App not found');
+		const app = await actions.getSingleContainerApp(appId);
+		return res.json(app);
+	} catch (e: unknown) {
+		if (isBadRequestError(e) || isNotFoundError(e)) {
+			return res.status(e.statusCode).send(e.statusMessage);
 		}
-
-		// handle the case where the appId is out of scope
-		if (!req.auth.isScoped({ apps: [app.appId] })) {
-			return res.status(401).json({
-				status: 'failed',
-				message: 'Unauthorized',
-			});
-		}
-
-		if (app.services.length > 1) {
-			return res
-				.status(400)
-				.send('Some v1 endpoints are only allowed on single-container apps');
-		}
-
-		// Because we only have a single app, we can fetch the commit for that
-		// app, and maintain backwards compatability
-		const commit = await commitStore.getCommitForApp(appId);
-
-		// Don't return data that will be of no use to the user
-		const appToSend = {
-			appId,
-			commit,
-			containerId: service.containerId,
-			env: _.omit(service.config.environment, constants.privateAppEnvVars),
-			imageId: service.config.image,
-			releaseId: service.releaseId,
-		};
-
-		return res.json(appToSend);
-	} catch (e) {
 		next(e);
 	}
 });

test/integration/device-api/actions.spec.ts

@@ -5,6 +5,7 @@ import * as request from 'supertest';
 import { setTimeout } from 'timers/promises';
 
 import * as deviceState from '~/src/device-state';
+import * as config from '~/src/config';
 import * as deviceApi from '~/src/device-api';
 import * as actions from '~/src/device-api/actions';
 import * as TargetState from '~/src/device-state/target-state';
@@ -357,6 +358,26 @@ describe('manages application lifecycle', () => {
 			});
 		});
 
+		it('should return information about a single-container app', async () => {
+			containers = await waitForSetup(targetState);
+			const containerId = containers[0].Id;
+			const imageHash = containers[0].Config.Image;
+
+			// Calling actions.getSingleContainerApp doesn't work because
+			// the action queries the database
+			const { body } = await request(BALENA_SUPERVISOR_ADDRESS).get(
+				'/v1/apps/1',
+			);
+
+			expect(body).to.have.property('appId', APP_ID);
+			expect(body).to.have.property('containerId', containerId);
+			expect(body).to.have.property('imageId', imageHash);
+			expect(body).to.have.property('releaseId', 1);
+			// Should return the environment of the single service
+			expect(body.env).to.have.property('BALENA_APP_ID', String(APP_ID));
+			expect(body.env).to.have.property('BALENA_SERVICE_NAME', serviceNames[0]);
+		});
+
 		// This test should be ordered last in this `describe` block, because the test compares
 		// the `CreatedAt` timestamps of volumes to determine whether purge was successful. Thus,
 		// ordering the assertion last will ensure some time has passed between the first `CreatedAt`

test/integration/device-api/v1.spec.ts

@@ -684,4 +684,64 @@ describe('device-api/v1', () => {
 				.expect(202);
 		});
 	});
+
+	describe('GET /v1/apps/:appId', () => {
+		let getSingleContainerAppStub: SinonStub;
+		beforeEach(() => {
+			getSingleContainerAppStub = stub(
+				actions,
+				'getSingleContainerApp',
+			).resolves({} as any);
+		});
+		afterEach(async () => {
+			getSingleContainerAppStub.restore();
+			// Remove all scoped API keys between tests
+			await db.models('apiSecret').whereNot({ appId: 0 }).del();
+		});
+
+		it('validates data from request body', async () => {
+			await request(api)
+				.get('/v1/apps/1234567')
+				.set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`);
+			expect(getSingleContainerAppStub).to.have.been.calledWith(1234567);
+		});
+
+		it('responds with 200 if request successful', async () => {
+			await request(api)
+				.get('/v1/apps/1234567')
+				.set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`)
+				.expect(200, {});
+		});
+
+		it('responds with 400 if invalid appId parameter', async () => {
+			await request(api)
+				.get('/v1/apps/badAppId')
+				.set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`)
+				.expect(400);
+		});
+
+		it('responds with 400 if action throws BadRequestError', async () => {
+			getSingleContainerAppStub.throws(new BadRequestError());
+			await request(api)
+				.get('/v1/apps/1234567')
+				.set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`)
+				.expect(400);
+		});
+
+		it("responds with 401 if caller's API key is not in scope of appId", async () => {
+			const scopedKey = await deviceApi.generateScopedKey(7654321, 'main');
+			await request(api)
+				.get('/v1/apps/1234567')
+				.set('Authorization', `Bearer ${scopedKey}`)
+				.expect(401);
+		});
+
+		it('responds with 503 for other errors that occur during request', async () => {
+			getSingleContainerAppStub.throws(new Error());
+			await request(api)
+				.get('/v1/apps/1234567')
+				.set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`)
+				.expect(503);
+		});
+	});
 });

test/legacy/41-device-api-v1.spec.ts

@@ -107,50 +107,6 @@ describe('SupervisorAPI [V1 Endpoints]', () => {
 		loggerStub.restore();
 	});
 
-	describe('GET /v1/apps/:appId', () => {
-		it('does not return information for an application when there is more than 1 container', async () => {
-			await request
-				.get('/v1/apps/2')
-				.set('Accept', 'application/json')
-				.set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`)
-				.expect(
-					sampleResponses.V1.GET['/apps/2 [Multiple containers running]']
-						.statusCode,
-				);
-		});
-
-		it('returns information about a specific application', async () => {
-			// Setup single container application
-			const container = mockedAPI.mockService({
-				containerId: 'abc123',
-				appId: 2,
-				releaseId: 77777,
-			});
-			const image = mockedAPI.mockImage({
-				appId: 2,
-			});
-			appMock.mockManagers([container], [], []);
-			appMock.mockImages([], false, [image]);
-			await mockedDockerode.testWithData(
-				{ containers: [container], images: [image] },
-				async () => {
-					// Make request
-					await request
-						.get('/v1/apps/2')
-						.set('Accept', 'application/json')
-						.set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`)
-						.expect(sampleResponses.V1.GET['/apps/2'].statusCode)
-						.expect('Content-Type', /json/)
-						.then((response) => {
-							expect(response.body).to.deep.equal(
-								sampleResponses.V1.GET['/apps/2'].body,
-							);
-						});
-				},
-			);
-		});
-	});
-
 	describe('GET /v1/device', () => {
 		it('returns MAC address', async () => {
 			const response = await request