GitOps Account - improved API access verification (#536)

* GitOps Account - improved API access verification

* chore: removed unnecessary check for path

* chore: fixes test fixture

Author: petruki

npm-shrinkwrap.json

@@ -49,8 +49,8 @@
     "helmet": "^8.0.0",
     "jsonwebtoken": "^9.0.2",
     "moment": "^2.30.1",
-    "mongodb": "^6.9.0",
-    "mongoose": "^8.7.2",
+    "mongodb": "^6.10.0",
+    "mongoose": "^8.7.3",
     "pino": "^9.5.0",
     "pino-pretty": "^11.3.0",
     "swagger-ui-express": "^5.0.1",

package.json

@@ -27,6 +27,7 @@ export const SwitcherKeys = Object.freeze({
     ACCOUNT_OUT_NOTIFY: 'ACCOUNT_OUT_NOTIFY',
     SLACK_INTEGRATION: 'SLACK_INTEGRATION',
     GITOPS_INTEGRATION: 'GITOPS_INTEGRATION',
+    GITOPS_SUBSCRIPTION: 'GITOPS_SUBSCRIPTION',
     RATE_LIMIT: 'RATE_LIMIT',
     HTTPS_AGENT: 'HTTPS_AGENT'
 });
@@ -214,6 +215,16 @@ export async function checkGitopsIntegration(value) {
     switcherFlagResult(featureFlag, 'GitOps Integration is not available.');
 }
 
+export function notifyGitopsSubscription(action) {
+    if (process.env.SWITCHER_API_ENABLE != 'true') {
+        return;
+    }
+
+    Client.getSwitcher(SwitcherKeys.GITOPS_SUBSCRIPTION)
+        .checkValue(action)
+        .isItOn();
+}
+
 export function notifyAcCreation(adminid) {
     if (process.env.SWITCHER_API_ENABLE != 'true') {
         return;

src/external/switcher-api-facade.js

@@ -4,13 +4,21 @@ import { responseExceptionSilent } from '../exceptions/index.js';
 import { auth, gitopsAuth } from '../middleware/auth.js';
 import { validate } from '../middleware/validators.js';
 import { featureFlag, validateChanges } from '../middleware/gitops.js';
+import { notifyGitopsSubscription } from '../external/switcher-api-facade.js';
 import * as Service from '../services/gitops/index.js';
+import { verifyOwnership } from '../helpers/index.js';
+import { ActionTypes, RouterTypes } from '../models/permission.js';
 
 const router = new express.Router();
-const regex = new RegExp(/^\d+[smh]$/);
+
+// Allow only values like 1s, 1m, 1h
+const regexWindowInterval = new RegExp(/^\d+[smh]$/);
+
+// Allow slash, alphanumeric, hyphen, underscore, dot only
+const regexPath = new RegExp(/^[a-zA-Z0-9/_\-.]+$/);
 
 const windowValidation = (value) => {
-    if (!regex.test(value)) {
+    if (!regexWindowInterval.test(value)) {
         throw new Error('Invalid window value');
     }
 
@@ -25,11 +33,23 @@ const windowValidation = (value) => {
     return true;
 };
 
+const pathValidation = (value) => {
+    if (value.startsWith('/') || value.endsWith('/') || value.includes('//')) {
+        throw new Error('Invalid path value - cannot start or end with / or contain //');
+    }
+
+    if (!regexPath.test(value)) {
+        throw new Error('Invalid path value - only alphanumeric characters and / are allowed');
+    }
+
+    return true;
+};
+
 const accountValidators = [
     body('token').isString().optional(),
     body('repository').isURL().withMessage('Invalid repository URL'),
     body('branch').isString().withMessage('Invalid branch name'),
-    body('path').isString().optional().withMessage('Invalid path'),
+    body('path').isString().optional().custom(pathValidation),
     body('environment').isString().withMessage('Invalid environment name'),
     body('domain.id').isMongoId().withMessage('Invalid domain ID'),
     body('domain.name').isString().withMessage('Invalid domain name'),
@@ -38,6 +58,16 @@ const accountValidators = [
     body('settings.forceprune').isBoolean().withMessage('Invalid forceprune flag'),
 ];
 
+const verifyOwnershipMiddleware = async (req, res, next) => {
+    try {
+        const domainId = req.body?.domain.id || req.params.domain;
+        await verifyOwnership(req.admin, domainId, domainId, ActionTypes.UPDATE, RouterTypes.ADMIN);
+        next();
+    } catch (e) {
+        responseExceptionSilent(res, e, 403, 'Permission denied');
+    }
+};
+
 router.post('/gitops/v1/push', gitopsAuth, featureFlag, [
     body('environment').isString(),
     body('changes').isArray(),
@@ -58,9 +88,11 @@ router.post('/gitops/v1/push', gitopsAuth, featureFlag, [
 });
 
 router.post('/gitops/v1/account/subscribe', auth, accountValidators, validate, 
-    featureFlag, async (req, res) => {
+    featureFlag, verifyOwnershipMiddleware, async (req, res) => {
     try {
         const account = await Service.subscribeAccount(req.body);
+        notifyGitopsSubscription('subscribe');
+
         res.status(201).send(account);
     } catch (e) {
         responseExceptionSilent(res, e, 500, 'Account subscription failed');
@@ -70,17 +102,19 @@ router.post('/gitops/v1/account/subscribe', auth, accountValidators, validate,
 router.post('/gitops/v1/account/unsubscribe', auth, [
     body('environment').isString(),
     body('domain.id').isMongoId().withMessage('Invalid domain ID'),
-], validate, featureFlag, async (req, res) => {
+], validate, featureFlag, verifyOwnershipMiddleware, async (req, res) => {
     try {
         await Service.unsubscribeAccount(req.body);
+        notifyGitopsSubscription('unsubscribe');
+
         res.status(200).send();
     } catch (e) {
         responseExceptionSilent(res, e, 500, 'Account unsubscription failed');
     }
 });
 
 router.put('/gitops/v1/account', auth, accountValidators, validate, 
-    featureFlag, async (req, res) => {
+    featureFlag, verifyOwnershipMiddleware, async (req, res) => {
     try {
         const account = await Service.updateAccount(req.body);
         res.status(200).send(account);
@@ -93,10 +127,10 @@ router.put('/gitops/v1/account/tokens', auth, [
     body('token').isString(),
     body('environments').isArray(),
     body('domain.id').isMongoId().withMessage('Invalid domain ID'),
-], validate, featureFlag, async (req, res) => {
+], validate, featureFlag, verifyOwnershipMiddleware, async (req, res) => {
     try {
-        const account = await Service.updateAccountTokens(req.body);
-        res.status(200).send(account);
+        const result = await Service.updateAccountTokens(req.body);
+        res.status(200).send(result);
     } catch (e) {
         responseExceptionSilent(res, e, 500, 'Account token update failed');
     }
@@ -105,7 +139,7 @@ router.put('/gitops/v1/account/tokens', auth, [
 router.put('/gitops/v1/account/forcesync', auth, [
     body('environment').isString(),
     body('domain.id').isMongoId().withMessage('Invalid domain ID'),
-], validate, featureFlag, async (req, res) => {
+], validate, featureFlag, verifyOwnershipMiddleware, async (req, res) => {
     try {
         const account = await Service.forceSyncAccount(req.body);
         res.status(200).send(account);
@@ -117,7 +151,7 @@ router.put('/gitops/v1/account/forcesync', auth, [
 router.get('/gitops/v1/account/:domain', auth, [
     check('domain').isMongoId().withMessage('Invalid domain ID'),
     check('environment').optional().isString(),
-], validate, featureFlag, async (req, res) => {
+], validate, featureFlag, verifyOwnershipMiddleware, async (req, res) => {
     try {
         const accounts = await Service.fetchAccounts(req.params.domain, req.query.environment || null);
         res.status(200).send(accounts);

src/routers/gitops.js

@@ -30,11 +30,33 @@ export async function pushChanges(domainId, environment, changes) {
 }
 
 export async function subscribeAccount(account) {
-    return GitOpsFacade.createAccount(account);
+    return GitOpsFacade.createAccount({
+        repository: account.repository,
+        environment: account.environment,
+        branch: account.branch,
+        token: account.token,
+        path: account.path,
+        settings: account.settings,
+        domain: {
+            id: account.domain.id,
+            name: account.domain.name
+        }
+    });
 }
 
 export async function updateAccount(account) {
-    return GitOpsFacade.updateAccount(account);
+    return GitOpsFacade.updateAccount({
+        repository: account.repository,
+        environment: account.environment,
+        branch: account.branch,
+        token: account.token,
+        path: account.path,
+        settings: account.settings,
+        domain: {
+            id: account.domain.id,
+            name: account.domain.name
+        }
+    });
 }
 
 export async function updateAccountTokens(account) {

src/services/gitops/index.js

@@ -262,6 +262,11 @@ describe('Testing domain [Adm-GraphQL] ', () => {
         expect(req.statusCode).toBe(200);
         expect(JSON.parse(req.text)).toMatchObject(JSON.parse(graphqlUtils.expected111));
     });
+});
+
+describe('Testing domain [Adm-GraphQL] - Permission', () => {
+
+    afterAll(setupDatabase);
 
     test('CLIENT_SUITE - Should return list of Groups permissions', async () => {
         const req = await request(app)
@@ -366,11 +371,6 @@ describe('Testing domain [Adm-GraphQL] ', () => {
         expect(JSON.parse(req.text)).not.toBe(null);
         expect(JSON.parse(req.text).data.permission).toStrictEqual([]);
     });
-});
-
-describe('Testing domain [Adm-GraphQL] - Permission', () => {
-
-    afterAll(setupDatabase);
 
     test('CLIENT_SUITE - Should return domain partial structure based on permission', async () => {
         // Given
@@ -393,7 +393,7 @@ describe('Testing domain [Adm-GraphQL] - Permission', () => {
         expect(JSON.parse(req.text)).toMatchObject(JSON.parse(graphqlUtils.expected1071));
     });
 
-    test('CLIENT_SUITE - Should NOT return complete domain structure - no valid COnfig permission', async () => {
+    test('CLIENT_SUITE - Should NOT return complete domain structure - no valid Config permission', async () => {
         // Given
         const admin = await Admin.findById(adminAccountId).exec();
         await setPermissionsToTeam(admin.teams[0], {

tests/client-api.test.js

@@ -182,13 +182,21 @@ export const permissionConfigs2 = {
     environments: [EnvType.DEFAULT]
 };
 
+export const permissionAdminId = new mongoose.Types.ObjectId();
+export const permissionAdmin = {
+    _id: permissionAdminId,
+    action: ActionTypes.UPDATE,
+    active: true,
+    router: RouterTypes.ADMIN
+};
+
 export const teamId = new mongoose.Types.ObjectId();
 export const team = {
     _id: teamId,
     domain: domainId,
     name: 'Team Dev',
     active: true,
-    permissions: [permissionConfigsId, permissionConfigs2Id]
+    permissions: [permissionConfigsId, permissionConfigs2Id, permissionAdminId]
 };
 
 export const slack = {
@@ -229,6 +237,7 @@ export const setupDatabase = async () => {
     await new Team(team).save();
     await new Permission(permissionConfigs).save();
     await new Permission(permissionConfigs2).save();
+    await new Permission(permissionAdmin).save();
 
     await new GroupConfig(groupConfigDocument).save();
     await new Config(configDocument).save();