From fb99dc27326b7cd609c893dde0158d544ac32251 Mon Sep 17 00:00:00 2001 From: JoshuaSBrown Date: Thu, 16 Jan 2025 23:23:13 -0500 Subject: [PATCH] Address feedback --- core/database/foxx/api/controllers/record.js | 83 +++----- core/database/foxx/api/controllers/repo.js | 2 +- core/database/foxx/tests/authz.test.js | 192 +++++++----------- core/database/foxx/tests/authz_router.test.js | 4 +- core/database/foxx/tests/record.test.js | 16 +- core/database/foxx/tests/repo.test.js | 159 ++++++--------- 6 files changed, 184 insertions(+), 272 deletions(-) diff --git a/core/database/foxx/api/controllers/record.js b/core/database/foxx/api/controllers/record.js index 44a07a696..a04efaca4 100644 --- a/core/database/foxx/api/controllers/record.js +++ b/core/database/foxx/api/controllers/record.js @@ -182,70 +182,53 @@ class Record { * @returns {boolean} True if consistent, otherwise false. */ isPathConsistent(a_path) { - // This function will populate the this.#loc member and the this.#alloc - // member if (!this.isManaged()) { return false; } - // If there is a new repo we need to check the path there and use that - if (this.#loc.hasOwnProperty("new_repo") && this.#loc.new_repo) { - // Below we get the allocation associated with data item by - // 1. Checking if the data item is in flight, is in the process - // of being moved to a new location or new owner and using that - // oweners id. - // 2. Using the loc.uid parameter if not inflight to get the owner - // id. - const new_alloc = g_db.alloc.firstExample({ - _from: this.#loc.new_owner ? this.#loc.new_owner : this.#loc.uid, - _to: this.#loc.new_repo, - }); - - // If no allocation is found for the item throw an error - // if the paths do not align also throw an error. - if (!new_alloc) { - this.#error = g_lib.ERR_PERM_DENIED; - this.#err_msg = - "Permission denied, '" + this.#key + "' is not part of an allocation '"; - return false; - } - - this.#repo = g_db._document(this.#loc.new_repo); + const handleError = (errorCode, errorMessage) => { + this.#error = errorCode; + this.#err_msg = errorMessage; + return false; + }; - if (!this.#repo) { - this.#error = g_lib.ERR_INTERNAL_FAULT; - this.#err_msg = - "Unable to find repo that record is meant to be allocated too, '" + - this.#loc.new_repo + - "' record '" + - this.#data_id; - return false; - } + const getRepo = (repoId) => + g_db._document(repoId) || + handleError( + g_lib.ERR_INTERNAL_FAULT, + `Unable to find repo for allocation: '${repoId}', record: '${this.#data_id}'`, + ); - // If path is missing the starting "/" add it back in - if (!a_path.startsWith("/") && this.#repo.path.startsWith("/")) { - a_path = "/" + a_path; - } + const formatPath = (path, repoPath) => + !path.startsWith("/") && repoPath.startsWith("/") ? `/${path}` : path; - let stored_path = this._pathToRecord(this.#loc, this.#repo.path); + const validatePath = (loc, repoPath, inputPath) => + this._comparePaths(this._pathToRecord(loc, repoPath), inputPath); - if (!this._comparePaths(stored_path, a_path)) { + const processRepo = (repoId, loc, inputPath) => { + this.#repo = getRepo(repoId); + if (!this.#repo) { return false; } - } else { - this.#repo = g_db._document(this.#loc._to); - if (!a_path.startsWith("/") && this.#repo.path.startsWith("/")) { - a_path = "/" + a_path; - } - let stored_path = this._pathToRecord(this.#loc, this.#repo.path); + inputPath = formatPath(inputPath, this.#repo.path); + return validatePath(loc, this.#repo.path, inputPath); + }; - // If there is no new repo check that the paths align - if (!this._comparePaths(stored_path, a_path)) { - return false; + if (this.#loc.new_repo) { + const ownerId = this.#loc.new_owner || this.#loc.uid; + const newAlloc = g_db.alloc.firstExample({ _from: ownerId, _to: this.#loc.new_repo }); + + if (!newAlloc) { + return handleError( + g_lib.ERR_PERM_DENIED, + `Permission denied, '${this.#key}' is not part of an allocation.`, + ); } + return processRepo(this.#loc.new_repo, this.#loc, a_path); } - return true; + + return processRepo(this.#loc._to, this.#loc, a_path); } } diff --git a/core/database/foxx/api/controllers/repo.js b/core/database/foxx/api/controllers/repo.js index 0787ee4cc..ddb7119d4 100644 --- a/core/database/foxx/api/controllers/repo.js +++ b/core/database/foxx/api/controllers/repo.js @@ -136,7 +136,7 @@ class Repo { pathType(a_path) { // Ensure the repo exists if (!this.#exists) { - throw [g_lib.ERR_PERM_DENIED, "Repo does not exist " + this.#repo_id]; + throw [g_lib.ERR_NOT_FOUND, "Repo does not exist " + this.#repo_id]; } let repo = g_db._document(this.#repo_id); diff --git a/core/database/foxx/tests/authz.test.js b/core/database/foxx/tests/authz.test.js index b5365e8f5..238fd1a78 100644 --- a/core/database/foxx/tests/authz.test.js +++ b/core/database/foxx/tests/authz.test.js @@ -76,12 +76,11 @@ describe("Authz functions", () => { let req_perm = g_lib.PERM_CREATE; - expect(() => - authzModule.isRecordActionAuthorized(client, data_key, req_perm), - ).to.throw().and.satisfy((error) => { - return Array.isArray(error) && - error[0] === g_lib.ERR_NOT_FOUND; - }); + expect(() => authzModule.isRecordActionAuthorized(client, data_key, req_perm)) + .to.throw() + .and.satisfy((error) => { + return Array.isArray(error) && error[0] === g_lib.ERR_NOT_FOUND; + }); }); }); @@ -170,14 +169,11 @@ describe("Authz functions", () => { let data_key = "bananas"; let data_id = "d/" + data_key; - g_db.d.save( - { - _key: data_key, - _id: data_id, - creator: "u/george", - }, - { waitForSync: true }, - ); + g_db.d.save({ + _key: data_key, + _id: data_id, + creator: "u/george", + }); let bob = { _key: "bob", @@ -191,16 +187,13 @@ describe("Authz functions", () => { is_admin: false, }; - g_db.u.save(bob, { waitForSync: true }); - g_db.u.save(george, { waitForSync: true }); + g_db.u.save(bob); + g_db.u.save(george); - g_db.owner.save( - { - _from: data_id, - _to: "u/george", - }, - { waitForSync: true }, - ); + g_db.owner.save({ + _from: data_id, + _to: "u/george", + }); let req_perm = g_lib.PERM_CREATE; expect(authzModule.isRecordActionAuthorized(bob, data_key, req_perm)).to.be.false; @@ -212,14 +205,11 @@ describe("Authz functions", () => { let data_key = "apples"; let data_id = "d/" + data_key; - g_db.d.save( - { - _key: data_key, - _id: data_id, - creator: "u/jack", - }, - { waitForSync: true }, - ); + g_db.d.save({ + _key: data_key, + _id: data_id, + creator: "u/jack", + }); let jack = { _key: "jack", @@ -227,27 +217,21 @@ describe("Authz functions", () => { is_admin: false, }; - g_db.u.save(jack, { waitForSync: true }); + g_db.u.save(jack); let fruity_project_id = "p/fruity"; - g_db.p.save( - { - _key: "fruity", - _id: fruity_project_id, - name: "Project Fruity", - }, - { waitForSync: true }, - ); + g_db.p.save({ + _key: "fruity", + _id: fruity_project_id, + name: "Project Fruity", + }); let condiments_project_id = "p/condiments"; - g_db.p.save( - { - _key: "condiments", - _id: condiments_project_id, - name: "Project Condiments", - }, - { waitForSync: true }, - ); + g_db.p.save({ + _key: "condiments", + _id: condiments_project_id, + name: "Project Condiments", + }); let mandy_admin_id = "u/mandy"; let mandy = { @@ -255,41 +239,29 @@ describe("Authz functions", () => { _id: mandy_admin_id, is_admin: false, }; - g_db.u.save(mandy, { waitForSync: true }); + g_db.u.save(mandy); let amy_admin_id = "u/amy"; - g_db.u.save( - { - _key: "amy", - _id: amy_admin_id, - is_admin: false, - }, - { waitForSync: true }, - ); - - g_db.owner.save( - { - _from: data_id, - _to: fruity_project_id, - }, - { waitForSync: true }, - ); - - g_db.admin.save( - { - _from: fruity_project_id, - _to: amy_admin_id, - }, - { waitForSync: true }, - ); - - g_db.admin.save( - { - _from: condiments_project_id, - _to: mandy_admin_id, - }, - { waitForSync: true }, - ); + g_db.u.save({ + _key: "amy", + _id: amy_admin_id, + is_admin: false, + }); + + g_db.owner.save({ + _from: data_id, + _to: fruity_project_id, + }); + + g_db.admin.save({ + _from: fruity_project_id, + _to: amy_admin_id, + }); + + g_db.admin.save({ + _from: condiments_project_id, + _to: mandy_admin_id, + }); let req_perm = g_lib.PERM_CREATE; // Non-project admin should not have permission @@ -302,14 +274,11 @@ describe("Authz functions", () => { let data_key = "cherry"; let data_id = "d/" + data_key; - g_db.d.save( - { - _key: data_key, - _id: data_id, - creator: "tim", - }, - { waitForSync: true }, - ); + g_db.d.save({ + _key: data_key, + _id: data_id, + creator: "tim", + }); let tim = { _key: "tim", @@ -319,14 +288,11 @@ describe("Authz functions", () => { // A project is the owner let project_id = "p/red_fruit"; - g_db.p.save( - { - _key: "red_fruit", - _id: project_id, - name: "Project Red Fruit", - }, - { waitForSync: true }, - ); + g_db.p.save({ + _key: "red_fruit", + _id: project_id, + name: "Project Red Fruit", + }); let bob_id = "u/bob"; @@ -336,25 +302,19 @@ describe("Authz functions", () => { is_admin: false, }; - g_db.u.save(bob, { waitForSync: true }); - - g_db.owner.save( - { - _from: data_id, - _to: project_id, - }, - { waitForSync: true }, - ); - - g_db.admin.save( - { - _from: project_id, - _to: bob_id, - }, - { waitForSync: true }, - ); - - g_db.u.save(tim, { waitForSync: true }); + g_db.u.save(bob); + + g_db.owner.save({ + _from: data_id, + _to: project_id, + }); + + g_db.admin.save({ + _from: project_id, + _to: bob_id, + }); + + g_db.u.save(tim); let req_perm = g_lib.PERM_READ; diff --git a/core/database/foxx/tests/authz_router.test.js b/core/database/foxx/tests/authz_router.test.js index 53ec97d26..1f2dc147a 100644 --- a/core/database/foxx/tests/authz_router.test.js +++ b/core/database/foxx/tests/authz_router.test.js @@ -73,7 +73,7 @@ function defaultWorkingSetup() { _key: valid_key, owner: james_id, }); - g_db.repo.save(repo_data, { waitForSync: true }); + g_db.repo.save(repo_data); g_db.u.save(base_user_data); @@ -156,7 +156,7 @@ function defaultWorkingSetupProject() { _key: repo_key, path: repo_path, }; - g_db.repo.save(repo_data, { waitForSync: true }); + g_db.repo.save(repo_data); // Create edges g_db.item.save({ diff --git a/core/database/foxx/tests/record.test.js b/core/database/foxx/tests/record.test.js index b1bbf7ae9..cab48aa22 100644 --- a/core/database/foxx/tests/record.test.js +++ b/core/database/foxx/tests/record.test.js @@ -15,7 +15,10 @@ function recordRepoAndUserSetup(record_key, user_id, repo_data) { _id: record_id, }); } - g_db.repo.save(repo_data); + + if (!g_db._exists(repo_data._id)) { + g_db.repo.save(repo_data); + } if (!g_db._exists(user_id)) { g_db.u.save({ @@ -30,6 +33,7 @@ describe("Record Class", () => { g_db.alloc.truncate(); g_db.loc.truncate(); g_db.repo.truncate(); + g_db.u.truncate(); }); it("unit_record: should initialize correctly and check record existence is invalid", () => { @@ -46,6 +50,7 @@ describe("Record Class", () => { const owner_id = "u/bob"; const repo_id = "repo/datafed-at-com"; const repo_data = { + _id: repo_id, _key: "datafed-at-com", }; // Create nodes @@ -75,6 +80,7 @@ describe("Record Class", () => { const repo_id = "repo/datafed-at-org"; // Create nodes const repo_data = { + _id: repo_id, _key: "datafed-at-org", }; recordRepoAndUserSetup(valid_key, owner_id, repo_data); @@ -95,6 +101,7 @@ describe("Record Class", () => { const repo_id = "repo/datafed-banana-com"; // Create nodes const repo_data = { + _id: repo_id, _key: "datafed-banana-com", }; recordRepoAndUserSetup(valid_key, owner_id, repo_data); @@ -120,6 +127,7 @@ describe("Record Class", () => { const repo_id = "repo/datafed-best-com"; // Create nodes const repo_data = { + _id: repo_id, _key: "datafed-best-com", }; recordRepoAndUserSetup(valid_key, owner_id, repo_data); @@ -177,6 +185,7 @@ describe("Record Class", () => { const repo_id = "repo/datafed-fine-com"; // Create nodes const repo_data = { + _id: repo_id, _key: "datafed-fine-com", path: "/correct/file/path", }; @@ -208,6 +217,7 @@ describe("Record Class", () => { const repo_id = "repo/datafed-cool-com"; // Create nodes const repo_data = { + _id: repo_id, _key: "datafed-cool-com", path: "/correct/file/path", }; @@ -242,10 +252,12 @@ describe("Record Class", () => { // Create nodes const repo_data = { + _id: repo_id, _key: "orange-at-org", path: "/old/file/path", }; const repo_data_new = { + _id: "repo/watermelon-at-org", _key: "watermelon-at-org", path: "/correct/file/path", }; @@ -282,10 +294,12 @@ describe("Record Class", () => { const new_repo_id = "repo/hamburger"; // Create nodes const repo_data = { + _id: "repo/passionfruit", _key: "passionfruit", path: "/old/file/path", }; const repo_data_new = { + _id: "repo/hamburger", _key: "hamburger", path: "/new/file/path", }; diff --git a/core/database/foxx/tests/repo.test.js b/core/database/foxx/tests/repo.test.js index 0ee5ab6bb..5e13469e1 100644 --- a/core/database/foxx/tests/repo.test.js +++ b/core/database/foxx/tests/repo.test.js @@ -250,32 +250,23 @@ describe("Testing Repo class", () => { it("unit_repo: testing isAdmin, for a user, with repo admin access", () => { const path = "/mnt/repo_root"; const repo_id = "repo/bam"; - g_db.repo.save( - { - _id: repo_id, - _key: "bam", - path: path, - }, - { waitForSync: true }, - ); + g_db.repo.save({ + _id: repo_id, + _key: "bam", + path: path, + }); const user_id = "u/hone"; - g_db.u.save( - { - _id: user_id, - _key: "hone", - is_admin: false, - }, - { waitForSync: true }, - ); + g_db.u.save({ + _id: user_id, + _key: "hone", + is_admin: false, + }); - g_db.admin.save( - { - _from: repo_id, - _to: user_id, - }, - { waitForSync: true }, - ); + g_db.admin.save({ + _from: repo_id, + _to: user_id, + }); const repo = new Repo(repo_id); @@ -285,24 +276,18 @@ describe("Testing Repo class", () => { it("unit_repo: testing isAdmin, for a user, with no privileges", () => { const path = "/mnt/repo_root"; const repo_id = "repo/bam"; - g_db.repo.save( - { - _id: repo_id, - _key: "bam", - path: path, - }, - { waitForSync: true }, - ); + g_db.repo.save({ + _id: repo_id, + _key: "bam", + path: path, + }); const user_id = "u/hone"; - g_db.u.save( - { - _id: user_id, - _key: "hone", - is_admin: false, - }, - { waitForSync: true }, - ); + g_db.u.save({ + _id: user_id, + _key: "hone", + is_admin: false, + }); const repo = new Repo(repo_id); @@ -314,37 +299,28 @@ describe("Testing Repo class", () => { const path = "/mnt/lair"; const repo_key = "paper"; const repo_id = "repo/" + repo_key; - g_db.repo.save( - { - _id: repo_id, - _key: repo_key, - path: path, - }, - { waitForSync: true }, - ); + g_db.repo.save({ + _id: repo_id, + _key: repo_key, + path: path, + }); // Create user const user_key = "randy"; const user_id = "u/randy"; - g_db.u.save( - { - _id: user_id, - _key: user_key, - is_admin: false, - }, - { waitForSync: true }, - ); + g_db.u.save({ + _id: user_id, + _key: user_key, + is_admin: false, + }); // Create project const project_key = "bigthing"; const project_id = "p/" + project_key; - g_db.p.save( - { - _id: project_id, - _key: project_key, - }, - { waitForSync: true }, - ); + g_db.p.save({ + _id: project_id, + _key: project_key, + }); // Create allocation connecting repo to project g_db.alloc.save({ @@ -367,7 +343,7 @@ describe("Testing Repo class", () => { // Create a repo with no allocations const repo_key = "emptyrepo"; const repo_id = "repo/" + repo_key; - g_db.repo.save({ _id: repo_id, _key: repo_key, path: "/mnt/empty" }, { waitForSync: true }); + g_db.repo.save({ _id: repo_id, _key: repo_key, path: "/mnt/empty" }); const repo = new Repo(repo_id); expect(repo.exists()).to.be.true; @@ -378,13 +354,13 @@ describe("Testing Repo class", () => { // Create repo const repo_key = "multiproject"; const repo_id = "repo/" + repo_key; - g_db.repo.save({ _id: repo_id, _key: repo_key, path: "/mnt/multi" }, { waitForSync: true }); + g_db.repo.save({ _id: repo_id, _key: repo_key, path: "/mnt/multi" }); // Create multiple projects const project_keys = ["proj1", "proj2", "proj3"]; const project_ids = project_keys.map((key) => `p/${key}`); project_ids.forEach((project_id) => { - g_db.p.save({ _id: project_id, _key: project_id.split("/")[1] }, { waitForSync: true }); + g_db.p.save({ _id: project_id, _key: project_id.split("/")[1] }); g_db.alloc.save({ _from: project_id, _to: repo_id }); }); @@ -398,15 +374,12 @@ describe("Testing Repo class", () => { // Create repo const repo_key = "invalidalloc"; const repo_id = "repo/" + repo_key; - g_db.repo.save( - { _id: repo_id, _key: repo_key, path: "/mnt/invalid" }, - { waitForSync: true }, - ); + g_db.repo.save({ _id: repo_id, _key: repo_key, path: "/mnt/invalid" }); // Create invalid allocations (e.g., user to repo) const user_key = "invaliduser"; const user_id = "u/" + user_key; - g_db.u.save({ _id: user_id, _key: user_key }, { waitForSync: true }); + g_db.u.save({ _id: user_id, _key: user_key }); g_db.alloc.save({ _from: user_id, _to: repo_id }); const repo = new Repo(repo_id); @@ -421,14 +394,8 @@ describe("Testing Repo class", () => { const repo2_key = "repo2"; const repo2_id = "repo/" + repo2_key; - g_db.repo.save( - { _id: repo1_id, _key: repo1_key, path: "/mnt/repo1" }, - { waitForSync: true }, - ); - g_db.repo.save( - { _id: repo2_id, _key: repo2_key, path: "/mnt/repo2" }, - { waitForSync: true }, - ); + g_db.repo.save({ _id: repo1_id, _key: repo1_key, path: "/mnt/repo1" }); + g_db.repo.save({ _id: repo2_id, _key: repo2_key, path: "/mnt/repo2" }); // Create projects const project1_key = "project1"; @@ -439,9 +406,9 @@ describe("Testing Repo class", () => { const project2_id = "p/" + project2_key; const project3_id = "p/" + project3_key; - g_db.p.save({ _id: project1_id, _key: project1_key }, { waitForSync: true }); - g_db.p.save({ _id: project2_id, _key: project2_key }, { waitForSync: true }); - g_db.p.save({ _id: project3_id, _key: project3_key }, { waitForSync: true }); + g_db.p.save({ _id: project1_id, _key: project1_key }); + g_db.p.save({ _id: project2_id, _key: project2_key }); + g_db.p.save({ _id: project3_id, _key: project3_key }); // Allocate projects to repos g_db.alloc.save({ _from: project1_id, _to: repo1_id }); @@ -469,13 +436,13 @@ describe("Testing Repo class", () => { const repo_key = "fine_wine"; const repo_id = "repo/" + repo_key; - g_db.repo.save({ _id: repo_id, _key: repo_key, path: "/mnt/white" }, { waitForSync: true }); + g_db.repo.save({ _id: repo_id, _key: repo_key, path: "/mnt/white" }); // Create projects const project_key = "zinfandel"; const project_id = "p/" + project_key; - g_db.p.save({ _id: project_id, _key: project_key }, { waitForSync: true }); + g_db.p.save({ _id: project_id, _key: project_key }); // Allocate projects to repos g_db.alloc.save({ _from: project_id, _to: repo_id }); @@ -517,15 +484,12 @@ describe("Testing Repo class", () => { // Create a repository const repo_key = "direct_alloc_repo"; const repo_id = "repo/" + repo_key; - g_db.repo.save( - { _id: repo_id, _key: repo_key, path: "/mnt/direct_alloc" }, - { waitForSync: true }, - ); + g_db.repo.save({ _id: repo_id, _key: repo_key, path: "/mnt/direct_alloc" }); // Create a user const user_key = "alice"; const user_id = "u/" + user_key; - g_db.u.save({ _id: user_id, _key: user_key }, { waitForSync: true }); + g_db.u.save({ _id: user_id, _key: user_key }); // Direct allocation from user to repo g_db.alloc.save({ _from: user_id, _to: repo_id }); @@ -540,15 +504,12 @@ describe("Testing Repo class", () => { // Create a repository const repo_key = "no_alloc_repo"; const repo_id = "repo/" + repo_key; - g_db.repo.save( - { _id: repo_id, _key: repo_key, path: "/mnt/no_alloc" }, - { waitForSync: true }, - ); + g_db.repo.save({ _id: repo_id, _key: repo_key, path: "/mnt/no_alloc" }); // Create a user const user_key = "charlie"; const user_id = "u/" + user_key; - g_db.u.save({ _id: user_id, _key: user_key }, { waitForSync: true }); + g_db.u.save({ _id: user_id, _key: user_key }); // Test repo const repo = new Repo(repo_id); @@ -563,19 +524,13 @@ describe("Testing Repo class", () => { const repo_key_2 = "different_repo"; const repo_id_2 = "repo/" + repo_key_2; - g_db.repo.save( - { _id: repo_id_1, _key: repo_key_1, path: "/mnt/other" }, - { waitForSync: true }, - ); - g_db.repo.save( - { _id: repo_id_2, _key: repo_key_2, path: "/mnt/different" }, - { waitForSync: true }, - ); + g_db.repo.save({ _id: repo_id_1, _key: repo_key_1, path: "/mnt/other" }); + g_db.repo.save({ _id: repo_id_2, _key: repo_key_2, path: "/mnt/different" }); // Create a user const user_key = "dave"; const user_id = "u/" + user_key; - g_db.u.save({ _id: user_id, _key: user_key }, { waitForSync: true }); + g_db.u.save({ _id: user_id, _key: user_key }); // Create allocation to a different repo g_db.alloc.save({ _from: user_id, _to: repo_id_1 }); // Allocating user to repo 1