1199 feature add support for scoped lookup #1225

JoshuaSBrown · 2025-01-13T12:34:53Z

PR Description

Replaces authz, in foxx with scoped lookup enforcing stricter authorization.

Tasks

- A description of the PR has been provided, and a diagram included if it is a new feature.
- Formatter has been run
- CHANGELOG comment has been added
- Labels have been assigned to the pr
- A reviwer has been added
- A user has been assigned to work on the pr
- If new feature a unit test has been added

Summary by Sourcery

Implement scoped lookup for stricter authorization in Foxx, replacing the previous authz implementation.

New Features:

Added support for scoped lookup, enabling more granular control over data access permissions.

Tests:

Added unit tests for scoped lookup functionality and project access control.

…ects.

…ect.

sourcery-ai · 2025-01-13T12:34:57Z

Reviewer's Guide by Sourcery

This pull request replaces the existing authorization module with a new scoped lookup implementation for stricter authorization enforcement. It introduces a new hasAccess method in the Repo class to check user permissions based on project allocations and admin privileges. The GridFTP authz strategies have been updated to use the new hasAccess method and now require a client ID for authorization checks.

Sequence diagram for authorization lookup flow

sequenceDiagram
    participant Client
    participant AuthzRouter
    participant Repo
    participant Project

    Client->>AuthzRouter: Request access to path
    AuthzRouter->>Repo: hasAccess(client_id)
    alt Direct allocation
        Repo-->>AuthzRouter: true
    else Project allocation
        Repo->>Project: hasAccess(client_id)
        Project-->>Repo: true/false
        Repo-->>AuthzRouter: true/false
    end
    AuthzRouter-->>Client: Authorization result

Class diagram showing the new authorization structure

classDiagram
    class Repo {
        -repo_id: string
        +isAdmin(client_id: string): boolean
        +hasAccess(client_id: string): boolean
        +getProjectIds(): string[]
    }
    class Project {
        -project_id: string
        -project: object
        +exists(): boolean
        +hasAccess(client_id: string): boolean
        +isMember(client_id: string): boolean
        +isAdmin(client_id: string): boolean
        +getRepositoryIds(): Set
        +hasAllocationOnRepo(repo_id: string): boolean
    }
    class User {
        -user_id: string
        -user: object
        +exists(): boolean
        +getGroupIds(): string[]
        +getProjectIds(): string[]
        +getRepos(): Set
    }
    Repo -- Project : allocations
    Project -- User : membership

File-Level Changes

Change	Details	Files
Added support for scoped lookup in authorization.	Introduced a new authorization module that uses scoped lookup to enforce stricter authorization checks. Implemented new strategy methods for handling different GridFTP actions based on path categorization. Added functions to check user permissions based on project allocations and admin privileges.	`core/database/foxx/api/controllers/authz.js`
Updated Repo class to support scoped lookup.	Added `isAdmin`, `hasAccess`, and `getProjectIds` methods to the `Repo` class. Added unit tests for the new methods in the `Repo` class. Refactored existing code to use the new methods for authorization checks.	`core/database/foxx/api/controllers/repo.js` `core/database/foxx/tests/repo.test.js`
Updated authz router to use the new Repo class methods.	Updated the authz router to use the new `hasAccess` method for authorization checks. Added logging for authorization successes and failures.	`core/database/foxx/api/authz_router.js`
Added unit tests for the new authorization module.	Added unit tests for the new authorization module. Added a new test target for the authorization module in the CMakeLists.txt file.	`core/database/foxx/tests/authz.test.js` `core/database/CMakeLists.txt`
Refactored file paths and imports.	Moved files related to controllers into a new `controllers` directory. Updated import paths to reflect the new file structure.	`core/database/foxx/api/repo.js` `core/database/foxx/api/record.js` `core/database/foxx/tests/path.test.js` `core/database/foxx/tests/record.test.js`
Added Project and User classes.	Created new classes for managing projects and users. Added methods for checking user permissions and project allocations. Added unit tests for the new classes.	`core/database/foxx/api/controllers/project.js` `core/database/foxx/api/controllers/user.js` `core/database/foxx/tests/project.test.js`

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time. You can also use
this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

sourcery-ai

Hey @JoshuaSBrown - I've reviewed your changes and they look great!

Here's what I looked at during the review

🟡 General issues: 3 issues found
🟢 Security: all looks good
🟡 Testing: 3 issues found
🟢 Complexity: all looks good
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

core/database/foxx/api/controllers/authz.js

sourcery-ai · 2025-01-13T12:36:15Z

core/database/foxx/api/controllers/user.js

+            })
+            .toArray();
+
+        const repo_ids = [...new Set(allocs.map((obj) => obj._to))];


issue (bug_risk): The 'allocs' variable is undefined in getRepos method - should be 'user_allocs'.

Also 'group_ids' is used in the query but getProjectIds() was called instead of getGroupIds().

Sourcery has a good catch! The resulting object should also be a set, not an array!

You're absolutely right, thanks for catching that! I've missed the incorrect variable name and the fact that the result should be a set. My apologies for the oversight. I've updated my suggested fix to reflect both of these points. Please double-check it to make sure it aligns with your intentions.

sourcery-ai · 2025-01-13T12:36:15Z

core/database/foxx/api/controllers/project.js

+            return false;
+        }
+        const repo_ids = this.getRepositoryIds();
+        return repo_ids.has(a_repo_id);


issue (bug_risk): getRepositoryIds() returns an array but Set.has() is called on it.

This will always return false since arrays don't have a has() method. Consider converting repo_ids to a Set or use includes() instead.

Sourcery left a good callout

You're welcome! I'm glad I could help. Since you agree with the assessment, I suggest changing the getRepositoryIds function to return a Set directly. This will be more efficient than creating an array and then converting it to a Set in hasAllocationOnRepo. Something like this:

getRepositoryIds() { const allocs = this.getAllocations(); return new Set(allocs.map((obj) => obj._to)); }

This way, the hasAllocationOnRepo function will work correctly with the has() method. Let me know if you have any other questions.

sourcery-ai · 2025-01-13T12:36:16Z

core/database/foxx/tests/repo.test.js

+        const repo = new Repo(repo_id);
+        expect(repo.isAdmin(user_id)).to.be.true;
+    });
+


suggestion (testing): Missing test case for system admin with no repo admin access.

Add a test case where a system admin doesn't have specific admin access to the repo, and verify isAdmin returns false.

Suggested change

const repo = new Repo(repo_id);

expect(repo.isAdmin(user_id)).to.be.true;

});

const repo = new Repo(repo_id);

expect(repo.isAdmin(user_id)).to.be.true;

});

it("should return false for system admin without repo admin access", () => {

const repo_id = "r/test";

g_db.r.save({

_id: repo_id,

_key: "test",

admins: [] // Empty admins list

});

const user_id = "u/systemadmin";

g_db.u.save({

_id: user_id,

_key: "systemadmin",

is_admin: true // System admin but not repo admin

});

const repo = new Repo(repo_id);

expect(repo.isAdmin(user_id)).to.be.false;

});

sourcery-ai · 2025-01-13T12:36:16Z

core/database/foxx/tests/repo.test.js

+        // Direct allocation from user to repo
+        g_db.alloc.save({ _from: user_id, _to: repo_id });
+
+        // Test repo
+        const repo = new Repo(repo_id);
+        expect(repo.exists()).to.be.true;


suggestion (testing): Edge case: User has direct allocation AND is a project member.

Add a test where the user has both a direct allocation and is a member of a project allocated to the repo. Verify that hasAccess still returns true.

Suggested change

// Direct allocation from user to repo

g_db.alloc.save({ _from: user_id, _to: repo_id });

// Test repo

const repo = new Repo(repo_id);

expect(repo.exists()).to.be.true;

// Direct allocation from user to repo

g_db.alloc.save({ _from: user_id, _to: repo_id });

// Create a project and add user as member

const project_key = "test_project";

const project_id = "p/" + project_key;

g_db.p.save({ _id: project_id, _key: project_key }, { waitForSync: true });

g_db.member.save({ _from: project_id, _to: user_id });

// Allocate project to repo

g_db.alloc.save({ _from: project_id, _to: repo_id });

// Test repo

const repo = new Repo(repo_id);

expect(repo.exists()).to.be.true;

sourcery-ai · 2025-01-13T12:36:16Z

core/database/foxx/tests/repo.test.js

+        expect(repo.hasAccess(user_id)).to.be.false; // should return false, no allocation exists
+    });
+
+    it("unit_repo: testing hasAccess for a user with allocation to a different repo", () => {


suggestion (testing): Edge case: User has allocation to a different repo AND is a system admin.

Add a test where the user has an allocation to a different repo but is also a system admin. Verify the expected behavior of hasAccess in this scenario.

Suggested implementation:

it("unit_repo: testing hasAccess for a user with allocation to a different repo", () => { // Create two repositories const repo_key_1 = "other_repo"; const repo_id_1 = "repo/" + repo_key_1; 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 }, ); // Create user with allocation to repo_1 only const user_key = "alice"; const user_id = "u/" + user_key; g_db.u.save({ _id: user_id, _key: user_key }, { waitForSync: true }); // Create allocation for repo_1 g_db.allocation.save( { _id: "allocation/test", _key: "test", repo: repo_id_1, u: user_id }, { waitForSync: true }, ); // Test access const repo2 = new Repo(repo_id_2); expect(repo2.hasAccess(user_id)).to.be.false; }); it("unit_repo: testing hasAccess for a system admin with allocation to a different repo", () => { // Create two repositories const repo_key_1 = "other_repo";

g_db.repo.save( { _id: repo_id_2, _key: repo_key_2, path: "/mnt/different" }, { waitForSync: true }, ); // Create system admin user with allocation to repo_1 only const user_key = "admin_bob"; const user_id = "u/" + user_key; g_db.u.save({ _id: user_id, _key: user_key, system_admin: true }, { waitForSync: true }); // Create allocation for repo_1 g_db.allocation.save( { _id: "allocation/admin_test", _key: "admin_test", repo: repo_id_1, u: user_id }, { waitForSync: true }, ); // Test access - should be true because user is system admin, regardless of allocation const repo2 = new Repo(repo_id_2); expect(repo2.hasAccess(user_id)).to.be.true;

sourcery-ai · 2025-01-13T12:36:16Z

core/database/foxx/api/controllers/project.js

+        const allocs = g_db.alloc
+            .byExample({
+                _from: this.#project_id,
+            })
+            .toArray();
+        return allocs;


suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)

Suggested change

const allocs = g_db.alloc

.byExample({

_from: this.#project_id,

})

.toArray();

return allocs;

return g_db.alloc

.byExample({

_from: this.#project_id,

})

.toArray();

Explanation
Something that we often see in people's code is assigning to a result variable
and then immediately returning it.

Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.

Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.

sourcery-ai · 2025-01-13T12:36:16Z

core/database/foxx/tests/project.test.js

+"use strict";
+
+const chai = require("chai");
+const expect = chai.expect;


suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)

Suggested change

const expect = chai.expect;

const {expect} = chai;

Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.

From the Airbnb Javascript Style Guide

AronPerez · 2025-01-16T19:58:42Z

core/database/foxx/tests/project.test.js

+const g_lib = require("../api/support");
+const arangodb = require("@arangodb");
+
+describe("Project Class", () => {


Suggested change

describe("Project Class", () => {

describe("Project", () => {

AronPerez · 2025-01-16T20:01:47Z

core/database/foxx/tests/path.test.js

@@ -2,7 +2,7 @@

 const chai = require("chai");
 const expect = chai.expect;
-const pathModule = require("../api/posix_path"); // Replace with the actual file name
+const pathModule = require("../api/utils/posix_path"); // Replace with the actual file name


Suggested change

const pathModule = require("../api/utils/posix_path"); // Replace with the actual file name

const pathModule = require("../api/utils/posix_path");

AronPerez · 2025-01-16T20:01:59Z

core/database/foxx/tests/authz.test.js

@@ -1,7 +1,7 @@
 "use strict";
 const chai = require("chai");
 const expect = chai.expect;
-const authzModule = require("../api/authz");
+const authzModule = require("../api/controllers/authz"); // Replace with the actual file name


Suggested change

const authzModule = require("../api/controllers/authz"); // Replace with the actual file name

const authzModule = require("../api/controllers/authz");

AronPerez · 2025-01-16T20:11:48Z

core/database/foxx/api/controllers/user.js

+const g_db = require("@arangodb").db;
+const g_lib = require("../support");
+
+class User {


This seems like a user model https://docs.arangodb.com/3.12/concepts/data-models/
My question is what's handling the user route interactions?

AronPerez · 2025-01-16T20:12:38Z

core/database/foxx/api/controllers/project.js

+const g_db = require("@arangodb").db;
+const g_lib = require("../support");
+
+class Project {


See my questions about controllers/user.js

AronPerez · 2025-01-16T20:15:57Z

core/database/foxx/api/controllers/authz.js

+const { Repo, PathType } = require("./repo");
+const { Project } = require("./project");
+
+module.exports = (function () {


This seems like some kind of module or service layer logic. Is this directly handling some API routes request? Also how would this be a controller?

AronPerez · 2025-01-16T20:16:37Z

core/database/foxx/api/authz_router.js

+                            " SUCCESS",
+                    );
+                } else {
+                    console.log(


Should this be console.error?

Suggested change

console.log(

console.error(

Good correction!

AronPerez · 2025-01-16T20:27:19Z

core/database/foxx/api/controllers/repo.js

@@ -61,7 +61,7 @@ class Repo {
        const collection = g_db._collection("repo");

        // This function is designed to check if the provided key exists in the
-        // database as a record. Searches are only made in the 'd' collection
+        // database. Searches are only made in the 'repo' collection


This makes me think this is a model

t-ramz

I have a few questions and comments.

t-ramz · 2025-01-16T18:39:25Z

core/database/foxx/api/controllers/authz.js

+     **/
+    obj.readRecord = function (client, path, a_repo) {
+        const permission = g_lib.PERM_RD_DATA;
+        // Will split a posix path into an array


This comment on splitPOSIXPath seems redundant since we have a JSDoc on it.

t-ramz · 2025-01-16T18:39:44Z

core/database/foxx/api/controllers/authz.js

+    obj.createRecord = function (client, path, a_repo) {
+        const permission = g_lib.PERM_WR_DATA;
+
+        // Will split a posix path into an array


Also redundant

core/database/foxx/api/controllers/authz.js

t-ramz · 2025-01-16T21:10:22Z

core/database/foxx/api/controllers/authz.js

+     * /mnt/large/data/project                 - REPO_PATH
+     * /mnt/large/data/user                    - REPO_PATH
+     */
+    obj.lookupRepo = function (client, path, a_repo) {


How does this differ from lookupRepoRoot?

There isn't a difference in this case. I'll just consolidate.

core/database/foxx/api/controllers/authz.js

t-ramz

A few more comments on this

t-ramz · 2025-01-16T21:24:28Z

core/database/foxx/api/controllers/project.js

+            return false;
+        }
+        const repo_ids = this.getRepositoryIds();
+        return repo_ids.has(a_repo_id);


Sourcery left a good callout

t-ramz · 2025-01-16T21:25:49Z

core/database/foxx/api/controllers/project.js

+     **/
+    getRepositoryIds() {
+        const allocs = this.getAllocations();
+        return [...new Set(allocs.map((obj) => obj._to))];


maybe this is as simple as return new Set([...allocs.map((obj) => obj._to)]);

t-ramz · 2025-01-16T21:45:40Z

core/database/foxx/api/controllers/user.js

+    }
+
+    /**
+     * Get all the repos the user has access too.


Spelling too -> to

t-ramz · 2025-01-16T21:47:30Z

core/database/foxx/api/controllers/user.js

+            })
+            .toArray();
+
+        const repo_ids = [...new Set(allocs.map((obj) => obj._to))];


Sourcery has a good catch! The resulting object should also be a set, not an array!

t-ramz · 2025-01-16T22:05:11Z

core/database/foxx/api/controllers/repo.js

+        // The maximum number of edges connecting a user to a repo is 3
+        // user <- g -> p -> repo
+        // We only need to verify one path exists
+        let qry = "FOR v, e, p IN 1..3 ANY @user_id admin, member, owner, alloc";


Are admin, member, owner, alloc the edges being searched here?

t-ramz

Approved contingent upon some small bugs being addressed.

[DAPS, Foxx] - 1180 refactor Part 2 authz

JoshuaSBrown added 11 commits January 2, 2025 07:27

Add tests for foxx project class

ed4398b

Create initial strategy methods for authz

5b5ee4e

Implement strategy method calls in authz_router

16d0d15

Add methods to repo class, for checking allocations, admins, and proj…

86239e4

…ects.

Add repo unit tests.

2c616d2

Add abstractions for project and user, as well as unit tests for proj…

c58ebab

…ect.

Move controllers into controller folder

64bc44f

Pass in repo object to strategy methods

552cf93

Pull in latest changes

d131a14

Apply formatting to javasript in foxx

1396f7f

Address conflicts from formatting

0ae304a

sourcery-ai bot reviewed Jan 13, 2025

View reviewed changes

JoshuaSBrown added 11 commits January 13, 2025 07:41

Address JSDoc differences

4f62a05

Address merge conflicts

ca85a70

Address merge conflicts

85d1b1f

Address merge conflict

8437b65

Adjust controllers update from 1180

001f32d

Address differentces in authz.js

64a2fc2

Further corrections to branch

233d2a5

Address merge conflict

88c3d6c

Address leftover diff header

bc34ad3

Return true if access allowed

584b196

Remove debug commands

43ccca1

JoshuaSBrown force-pushed the 1199-feature-add-support-for-scoped-lookup branch from 7b2f123 to 43ccca1 Compare January 15, 2025 12:39

Merge in latest changes

49c91f5

JoshuaSBrown mentioned this pull request Jan 16, 2025

[DAPS, Foxx] - 1180 refactor Part 2 authz #1224

Merged

7 tasks

AronPerez requested changes Jan 16, 2025

View reviewed changes

t-ramz reviewed Jan 16, 2025

View reviewed changes

Address merge conflict

6c586fb

t-ramz reviewed Jan 16, 2025

View reviewed changes

t-ramz approved these changes Jan 16, 2025

View reviewed changes

JoshuaSBrown and others added 11 commits January 16, 2025 22:01

Remove superflous comments

86b5349

Apply formatting

5a291c4

Address eslint complaints about JSDoc

f51b41f

Test that NOT_FOUND error is thrown

cdb0dca

Address feedback

fb99dc2

Merge pull request #1224 from ORNL/1180-refactor-authz

e7972b8

[DAPS, Foxx] - 1180 refactor Part 2 authz

Merge branch 'devel' into 1199-feature-add-support-for-scoped-lookup

8249a5b

Add in missing permission to lookupRecord authz call

933e39d

Turn off automatic approval for lookup and chdir in repo

a18ab2c

Add missing gcs build trigger

fe578ed

Remove commented out code for lookup and chdir

fa1ee90

-        const repo = new Repo(repo_id);
-        expect(repo.isAdmin(user_id)).to.be.true;
-    });
+        const repo = new Repo(repo_id);
+        expect(repo.isAdmin(user_id)).to.be.true;
+    });
+    it("should return false for system admin without repo admin access", () => {
+        const repo_id = "r/test";
+        g_db.r.save({
+            _id: repo_id,
+            _key: "test",
+            admins: []  // Empty admins list
+        });
+        const user_id = "u/systemadmin";
+        g_db.u.save({
+            _id: user_id,
+            _key: "systemadmin",
+            is_admin: true  // System admin but not repo admin
+        });
+        const repo = new Repo(repo_id);
+        expect(repo.isAdmin(user_id)).to.be.false;
+    });

	describe("Project Class", () => {
	describe("Project", () => {

	const pathModule = require("../api/utils/posix_path"); // Replace with the actual file name
	const pathModule = require("../api/utils/posix_path");

	const authzModule = require("../api/controllers/authz"); // Replace with the actual file name
	const authzModule = require("../api/controllers/authz");

1199 feature add support for scoped lookup #1225

Are you sure you want to change the base?

1199 feature add support for scoped lookup #1225

Conversation

JoshuaSBrown commented Jan 13, 2025 • edited by sourcery-ai bot Loading

PR Description

Tasks

Summary by Sourcery

sourcery-ai bot commented Jan 13, 2025 • edited Loading

Reviewer's Guide by Sourcery

Sequence diagram for authorization lookup flow

Class diagram showing the new authorization structure

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

sourcery-ai bot left a comment

Choose a reason for hiding this comment

sourcery-ai bot Jan 13, 2025

Choose a reason for hiding this comment

t-ramz Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

sourcery-ai bot Jan 16, 2025

Choose a reason for hiding this comment

sourcery-ai bot Jan 13, 2025

Choose a reason for hiding this comment

t-ramz Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

sourcery-ai bot Jan 16, 2025

Choose a reason for hiding this comment

sourcery-ai bot Jan 13, 2025

Choose a reason for hiding this comment

sourcery-ai bot Jan 13, 2025

Choose a reason for hiding this comment

sourcery-ai bot Jan 13, 2025

Choose a reason for hiding this comment

sourcery-ai bot Jan 13, 2025

Choose a reason for hiding this comment

sourcery-ai bot Jan 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-ramz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-ramz left a comment

Choose a reason for hiding this comment

t-ramz Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-ramz Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-ramz left a comment

Choose a reason for hiding this comment

JoshuaSBrown commented Jan 13, 2025 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Jan 13, 2025 •

edited

Loading

t-ramz Jan 16, 2025 •

edited

Loading

t-ramz Jan 16, 2025 •

edited

Loading

t-ramz Jan 16, 2025 •

edited

Loading

t-ramz Jan 16, 2025 •

edited

Loading