Merge pull request #22 from duckduckgo/jmatthews/pixel-owner-validation

Check that pixel owners are Github userids in a list of approved ids as part of pixel validation

Author: jeannamatthews

bin/validate_schema.mjs

@@ -4,6 +4,7 @@ import fs from 'fs';
 import JSON5 from 'json5';
 import path from 'path';
 import yargs from 'yargs';
+import yaml from 'js-yaml';
 
 import { DefinitionsValidator } from '../src/definitions_validator.mjs';
 import { logErrors } from '../src/error_utils.mjs';
@@ -25,6 +26,12 @@ const argv = yargs(hideBin(process.argv))
             },
         });
     })
+    .option('githubUserMap', {
+        alias: 'g',
+        describe: 'Path to the GitHub user map YAML file',
+        type: 'string',
+        demandOption: false,
+    })
     .option('file', {
         alias: 'f',
         type: 'string',
@@ -51,25 +58,38 @@ const experiments = fileUtils.readExperimentsDef(mainDir);
 logErrors('ERROR in native_experiments.json:', validator.validateExperimentsDefinition(experiments));
 
 // 3) Validate pixels and params
-function validateFile(file) {
+function validateFile(file, userMap) {
     console.log(`Validating pixels definition: ${file}`);
     const pixelsDef = JSON5.parse(fs.readFileSync(file));
-    logErrors(`ERROR in ${file}:`, validator.validatePixelsDefinition(pixelsDef));
+    logErrors(`ERROR in ${file}:`, validator.validatePixelsDefinition(pixelsDef, userMap));
 }
 
-function validateFolder(folder) {
+function validateFolder(folder, userMap) {
     fs.readdirSync(folder, { recursive: true }).forEach((file) => {
         const fullPath = path.join(folder, file);
         if (fs.statSync(fullPath).isDirectory()) {
             return;
         }
 
-        validateFile(fullPath);
+        validateFile(fullPath, userMap);
     });
 }
+let userMap = null;
+
+if (argv.githubUserMap) {
+    console.log(`Reading GitHub user map from: ${argv.githubUserMap}`);
+    try {
+        userMap = yaml.load(fs.readFileSync(argv.githubUserMap, 'utf8'));
+    } catch (error) {
+        console.error(`Error reading GitHub user map from ${argv.githubUserMap}:`, error.message);
+        process.exit(1);
+    }
+} else {
+    console.log('No GitHub user map provided, skipping owner validation.');
+}
 
 if (argv.file) {
-    validateFile(path.join(pixelsDir, argv.file));
+    validateFile(path.join(pixelsDir, argv.file), userMap);
 } else {
-    validateFolder(pixelsDir);
+    validateFolder(pixelsDir, userMap);
 }

src/definitions_validator.mjs

@@ -84,7 +84,7 @@ export class DefinitionsValidator {
      * @param {*} pixelsDef - object containing multiple pixel definitions
      * @returns {Array<string>} - array of error messages
      */
-    validatePixelsDefinition(pixelsDef) {
+    validatePixelsDefinition(pixelsDef, userMap) {
         // 1) Validate that pixel definition conforms to schema
         if (!this.#ajvValidatePixels(pixelsDef)) {
             // Doesn't make sense to check the rest if main definition is invalid
@@ -94,13 +94,23 @@ export class DefinitionsValidator {
         // 2) Validate that:
         // (a) there are no duplicate prefixes and
         // (b) shortcuts, params, and suffixes can be compiled into a separate schema
+        // (c) all owners are valid github usernames in the provided userMap
         const errors = [];
         Object.entries(pixelsDef).forEach(([pixelName, pixelDef]) => {
             if (this.#definedPrefixes.has(pixelName)) {
                 errors.push(`${pixelName} --> Conflicting/duplicated definitions found!`);
                 return;
             }
 
+            // If a github user map is provided, check if all owners are valid
+            if (userMap) {
+                for (const owner of pixelDef.owners) {
+                    if (!userMap[owner]) {
+                        errors.push(`Owner ${owner} for pixel ${pixelName} not in list of acceptable github user names`);
+                    }
+                }
+            }
+
             this.#definedPrefixes.add(pixelName);
             try {
                 this.#paramsValidator.compileSuffixesSchema(pixelDef.suffixes);

tests/integration_test.mjs

@@ -11,7 +11,9 @@ const validDefsPath = path.join('tests', 'test_data', 'valid');
 const liveValidationResultsPath = path.join(validDefsPath, 'expected_processing_results');
 const validCaseInsensitiveDefsPath = path.join('tests', 'test_data', 'valid_case_insensitive');
 const invalidDefsPath = path.join('tests', 'test_data', 'invalid');
-describe('Invalid defs', () => {
+const validUserMapPath = path.join('tests', 'test_data', 'valid', 'user_map.yml');
+
+describe('Invalid defs without user map', () => {
     it('should output all required params', (done) => {
         exec(`npm run validate-ddg-pixel-defs ${invalidDefsPath}`, (error, _, stderr) => {
             const pixelPath = path.join(invalidDefsPath, 'pixels', 'pixels.json');
@@ -34,7 +36,27 @@ describe('Invalid defs', () => {
     }).timeout(timeout);
 });
 
-describe('Valid defs', () => {
+describe('Invalid owner with user map', () => {
+    it('should output error for invalid owner', (done) => {
+        // Careful: We need the -- to pass the -g flag to the script
+        exec(`npm run validate-ddg-pixel-defs -- ${invalidDefsPath} -g ${validUserMapPath}`, (error, _, stderr) => {
+            const pixelPath = path.join(invalidDefsPath, 'pixels', 'invalid_owner.json');
+
+            // All of these should be present in the output
+            const expectedErrors = [
+                `ERROR in ${pixelPath}: Owner username_not_in_user_map for pixel pixel_with_invalid_owner not in list of acceptable github user names`,
+            ];
+
+            const errors = stderr.trim().split('\n');
+            expect(errors).to.include.members(expectedErrors);
+            expect(error.code).to.equal(1);
+
+            done();
+        });
+    }).timeout(timeout);
+});
+
+describe('Valid defs without user map', () => {
     it('should exit normally', (done) => {
         exec(`npm run validate-ddg-pixel-defs ${validDefsPath}`, (error, _, stderr) => {
             expect(stderr.length).to.equal(0);
@@ -45,6 +67,17 @@ describe('Valid defs', () => {
     }).timeout(timeout);
 });
 
+describe('Valid defs with user map', () => {
+    it('should exit normally', (done) => {
+        exec(`npm run validate-ddg-pixel-defs -- ${validDefsPath} -g ${validUserMapPath}`, (error, _, stderr) => {
+            expect(stderr.length).to.equal(0);
+            expect(error).to.equal(null);
+
+            done();
+        });
+    }).timeout(timeout);
+});
+
 describe('Validate live pixels', () => {
     it('case sensitive - should produce expected errors', (done) => {
         exec(`npm run preprocess-defs ${validDefsPath}`, (error, _, stderr) => {

tests/test_data/invalid/pixels/invalid_owner.json

@@ -0,0 +1,7 @@
+{
+    "pixel_with_invalid_owner": {
+        "description": "Test pixel with invalid owner.",
+        "owners": ["username_not_in_user_map"],
+        "triggers": ["other"]
+    }
+}

tests/test_data/valid/user_map.yml

@@ -0,0 +1,3 @@
+# Format is GITHUB_USERNAME: ASANA_USER_ID
+ddg_username: '1234567890'
+tester: '1234567891'