1180 refactor foxx authz #1181

JoshuaSBrown · 2024-12-13T19:30:46Z

PR Description

Tasks

- A description of the PR has been provided, and a diagram included if it is a new feature.
- Formatter has been run
- 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

Refactor the authorization router by modularizing the code into separate modules for better maintainability and readability. Introduce new unit tests for the Record class and path handling to ensure robustness and correct error handling.

Enhancements:

Refactor the authorization router to improve code readability and maintainability by modularizing the logic into separate modules.

Tests:

Add new unit tests for the Record class and path handling to ensure correct functionality and error handling.

sourcery-ai · 2024-12-13T19:30:51Z

Reviewer's Guide by Sourcery

This PR refactors the authorization system by introducing new modules for better code organization and maintainability. The changes include splitting functionality into dedicated modules for record management, path handling, and authorization checks, along with comprehensive unit tests to ensure reliability.

Sequence diagram for the authorization check process

sequenceDiagram
    participant Client
    participant Router
    participant AuthzModule
    participant Record
    participant PathModule
    Client->>Router: GET /gridftp
    Router->>AuthzModule: isRecordActionAuthorized(client, data_key, req_perm)
    AuthzModule-->>Router: Authorization result
    Router->>Record: new Record(data_key)
    Record->>PathModule: splitPOSIXPath(file)
    PathModule-->>Record: Path components
    Record-->>Router: Path consistency result
    Router-->>Client: Authorization response

Class diagram for the refactored authorization system

classDiagram
    class Record {
        -error
        -err_msg
        -exists
        -loc
        -alloc
        -key
        -data_id
        +constructor(a_key)
        +exists() boolean
        +key() string
        +error() string
        +errorMessage() string
        +isManaged() boolean
        +isPathConsistent(a_path) boolean
    }
    class AuthzModule {
        +isRecordActionAuthorized(a_client, a_data_key, a_perm) boolean
    }
    class PathModule {
        +splitPOSIXPath(a_posix_path) string[]
    }
    Record --> AuthzModule : uses
    Record --> PathModule : uses

File-Level Changes

Change	Details	Files
Refactored authorization router to use new modular architecture	Split authorization logic into separate modules (Record, Path, and Authz) Improved error handling and messaging Added structured logging for authorization events Simplified path validation logic using new Path module	`core/database/foxx/api/authz_router.js`
Created new Record class for managing data records	Implemented methods for checking record existence and management status Added path consistency validation Created error handling system with codes and messages Added support for handling record relocations and ownership changes	`core/database/foxx/api/record.js`
Added POSIX path handling module	Created utility for splitting POSIX paths into components Added input validation for path strings Implemented filtering for empty path components	`core/database/foxx/api/posix_path.js`
Added authorization utility module	Created function for checking record action authorization Implemented permission validation logic Added support for admin permission checks	`core/database/foxx/api/authz.js`
Enhanced testing infrastructure	Added comprehensive unit tests for Record class Created test suite for POSIX path handling Updated CMake configuration to include new tests Added documentation for test configuration	`core/database/tests/test_foxx.sh` `core/database/CMakeLists.txt` `core/database/foxx/tests/record.test.js` `core/database/foxx/tests/path.test.js` `core/database/README.md`

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.

JoshuaSBrown · 2024-12-18T20:40:30Z

@sourcery-ai review

sourcery-ai

Hey @JoshuaSBrown - I've reviewed your changes and found some issues that need to be addressed.

Blocking issues:

Security issue: data_id variable is undefined in public read access check (link)

Here's what I looked at during the review

🟢 General issues: all looks good
🔴 Security: 1 blocking issue
🟢 Testing: all looks good
🟡 Complexity: 1 issue found
🟢 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.}

sourcery-ai · 2024-12-18T20:41:29Z

core/database/foxx/api/authz_router.js

+      // if record exists and if it is a public record
+      if (!client) {
+        if (record.exists()) {
+          if (req.queryParams.act != "read" || !g_lib.hasPublicRead(data_id)) {


🚨 issue (security): Security issue: data_id variable is undefined in public read access check

The data_id variable is not defined at this point in the code. Should use record.key() or similar method from the Record class to maintain consistency and prevent potential security vulnerabilities.

Nice catch.

Thank you for your feedback, we will generate more comments like this in the future according to the following instructions:

- Ensure the comment identifies a specific issue in the code, such as a security vulnerability or bug. - The comment should provide a clear explanation of the issue, including why it is a problem. - Suggest a specific solution or improvement to address the issue. - Use clear and concise language to communicate the problem and solution effectively.

core/database/foxx/api/record.js

core/database/foxx/api/authz.js

core/database/foxx/api/authz_router.js

core/database/foxx/api/record.js

…80-refactor-authz

core/database/foxx/api/record.js

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

Co-authored-by: Aaron Perez <aperez0295@gmail.com>

AronPerez

Questions about testing and some comments left in

core/database/foxx/api/support.js

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

t-ramz

I have a few questions and comments!

t-ramz · 2025-01-06T12:40:57Z

core/database/README.md

+
+Note The granularity of CMake is dependent on how they are defined in the CMakeLists.txt file. The tests are specified in
+CMake by passing a string that is matched against the chai test cases in the
+"it()" sections of the chai unit tests. Any test cases that match the pattern will run when that test is triggered.


It will also match against the "describe" blocks

core/database/foxx/api/authz.js

t-ramz · 2025-01-06T12:45:14Z

core/database/foxx/api/authz.js

+     *
+     * @param - the permission type that is being checked i.e.
+     *
+     * PERM_CREATE


If these permissions are stored somewhere as a type, the above jsdoc reference can show their type

That's a good suggestion. The client is not currently defined in a single location as it is what is pulled out of the database.

t-ramz · 2025-01-06T12:50:16Z

core/database/foxx/api/authz.js

+     * PERM_RD_DATA
+     **/
+    obj.isRecordActionAuthorized = function (a_client, a_data_key, a_perm) {
+        const data_id = "d/" + a_data_key;


I am asking out of curiosity, is it best practice to construct an ID? I feel like if we ask for a key and use an ID, it may be better to either 1. Just ask for the ID or 2. Search for the record based on the key, and use the record ID.

A reason that I ask is because I am unsure if we can be 100% certain that the ID will always be collection/key, is that the case?

Great question, and yes you can. Thie is from the official documentation.

https://docs.arangodb.com/stable/concepts/data-structure/documents/

All documents contain special attributes at the top-level that start with an underscore, known as system attributes:

The document key is stored as a string in the _key attribute.
The document identifier is stored as a string in the _id attribute.
The document revision is stored as a string in the _rev attribute.
You can specify a value for the _key attribute when creating a document. The _id attribute is automatically set based on the collection and _key. The _id and _key values are immutable once the document has been created. The _rev value is maintained by ArangoDB automatically.

keys are unique to the collection, ids globally unique.

I would say that if you don't conform to the pattern, it is likely an anti-pattern.

Ok yeah I see that in the docs. Cool to know.

t-ramz · 2025-01-06T13:02:20Z

core/database/foxx/api/support.js

@@ -1001,6 +1034,31 @@ module.exports = (function () {
        return false;
    };

+    /**
+     * @breif checks to make sure the client has admin permissions on an object


Typo? @brief

t-ramz · 2025-01-06T13:09:23Z

core/database/foxx/api/authz_router.js

-            var idx = req.queryParams.file.lastIndexOf("/");
-            var data_key = req.queryParams.file.substr(idx + 1);
-            var data_id = "d/" + data_key;
+            // Will split a posix path into an array


I think this comment is unnecessary because of the splitPOSIXPath jsdoc

t-ramz · 2025-01-06T13:12:41Z

core/database/foxx/api/authz_router.js

+                    throw [record.error(), record.errorMessage()];
+                }
+            } else {
+                // If the record does not exist then the path would noe be consistent.


Typo? noe -> not ?

t-ramz · 2025-01-06T13:15:09Z

core/database/foxx/api/authz_router.js

-            console.log("Loc is:");
-            console.log(loc);
-            if (!loc) {
+            if (record.exists()) {


There are a lot of checks for record existence. Is there a way to pull this check up a level or two in the nested conditionals?

Good catch, This is addressed in a follow up PR. see here. https://github.com/ORNL/DataFed/pull/1196/files#diff-2a4e90aa74276bb3ccc3605ae33282b2cfa475fd702f7239150b202e6a4ed152

t-ramz · 2025-01-06T13:18:56Z

core/database/foxx/api/record.js

+     * @brief Checks if the record exists in the database.
+     * @return {boolean} True if the record exists, otherwise false.
+     */
+    exists() {


Ok I see the exists() calls are inexpensive. That's good.

t-ramz · 2025-01-06T13:28:06Z

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

+        g_db.u.truncate();
+    });
+
+    it("unit_authz: if admin should return true", () => {


Should we also have a test for not admin, but has write? And another for no admin, no write?

Good catch, These are addressed in a follow up PR here: https://github.com/ORNL/DataFed/pull/1196/files#diff-29f2f796d64cb649168cf5d450247a757a3f3b2d0c252f8ad394c76ac48ee08c

JoshuaSBrown added 3 commits December 11, 2024 16:33

Add posix path js helper file

6f5a0fe

Add unit tests for poxix path separator

f920a71

First commit refactoring authz route

2c1f2cd

JoshuaSBrown changed the base branch from master to devel December 13, 2024 19:31

JoshuaSBrown added the Component: GridFTP Auth Module Relates to GridFTP authorization library label Dec 13, 2024

JoshuaSBrown self-assigned this Dec 13, 2024

JoshuaSBrown added 10 commits December 13, 2024 14:33

Merge branch 'devel' into 1180-refactor-authz

2c905a7

Add unit tests to CMake

420abaa

Add clean record.js object

83086b8

Add more specific pattern matching

e8c9121

Add README with test instructions.

9074897

Apply formatting

99dfbf1

Applied formatting

6b4a2b6

Apply formatting

a71e2dd

add missing pass file

83de46c

Add missing new command

c2431b6

JoshuaSBrown linked an issue Dec 18, 2024 that may be closed by this pull request

[Foxx, Database, Authz] - refactor authz foxx service #1180

Open

3 tasks

JoshuaSBrown added Type: Refactor Imlplementation change, same functionality Priority: High Highest priority Component: Scripts Helper scripts or admin scripts Component: Build Related to the build system Type: Test Related to unit or integration testing labels Dec 18, 2024

Negate if statement

2625b58

sourcery-ai bot reviewed Dec 18, 2024

View reviewed changes

JoshuaSBrown added 4 commits December 18, 2024 16:28

Add initial authz test

1c7e591

Add initial authz test

6ffc681

Added explantory comments

8397678

Addressed review feedback

cc66b9b

JoshuaSBrown added 8 commits December 19, 2024 08:39

Merge branch '1180-refactor-authz' of github.com:ORNL/DataFed into 11…

1f47bac

…80-refactor-authz

Merge branch 'devel' into 1180-refactor-authz

f82fcd3

Make corrections to unit tests

f409ce5

Fix authz test

124d378

Trigger foxx build when change in core database not web server

8af9492

Address merge conflict

ebef1b0

Address code review comment from ai

c7f13b6

Add changelog comment

0ada97c

JoshuaSBrown changed the title ~~1180 refactor authz~~ 1180 refactor foxx authz Dec 30, 2024

This was linked to issues Dec 30, 2024

[CI, Build, Foxx, test] - docs missing and foxx unit testing #1189

Open

[Foxx, Test, BE] - feature refactor foxx gridftp authz and test #1171

Open

AronPerez requested changes Dec 31, 2024

View reviewed changes

JoshuaSBrown and others added 3 commits December 31, 2024 11:47

Switch let to const.

a53f9d4

Co-authored-by: Aaron Perez <aperez0295@gmail.com>

Address review feedback

da9608e

Merge branch 'devel' into 1180-refactor-authz

dd71e7c

JoshuaSBrown requested a review from AronPerez January 2, 2025 21:14

AronPerez reviewed Jan 2, 2025

View reviewed changes

core/database/foxx/api/support.js Outdated Show resolved Hide resolved

core/database/foxx/tests/authz.test.js Show resolved Hide resolved

core/database/foxx/tests/authz.test.js Outdated Show resolved Hide resolved

Add better comments around database method

88be445

JoshuaSBrown requested a review from AronPerez January 2, 2025 21:35

JoshuaSBrown and others added 6 commits January 2, 2025 16:39

Apply formatting

9d3622b

Merge branch 'devel' into 1180-refactor-authz

ff1814b

Format authz test

eaf34fa

Format authz file

3771f9c

Apply formatting to posix path

7dae7ca

Address formatting issues

2bb1213

t-ramz reviewed Jan 6, 2025

View reviewed changes

t-ramz approved these changes Jan 7, 2025

View reviewed changes

AronPerez approved these changes Jan 7, 2025

View reviewed changes

Merge branch 'devel' into 1180-refactor-authz

eba25db

JoshuaSBrown merged commit f60d2f7 into devel Jan 7, 2025
13 of 14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1180 refactor foxx authz #1181

1180 refactor foxx authz #1181

JoshuaSBrown commented Dec 13, 2024 •

edited

Loading

sourcery-ai bot commented Dec 13, 2024 •

edited

Loading

Interacting with Sourcery

Customizing Your Experience

Getting Help

JoshuaSBrown commented Dec 18, 2024

sourcery-ai bot left a comment

sourcery-ai bot Dec 18, 2024

JoshuaSBrown Dec 18, 2024

sourcery-ai bot Dec 18, 2024

AronPerez left a comment

t-ramz left a comment

t-ramz Jan 6, 2025

t-ramz Jan 6, 2025

JoshuaSBrown Jan 7, 2025

t-ramz Jan 6, 2025

JoshuaSBrown Jan 6, 2025

JoshuaSBrown Jan 6, 2025

t-ramz Jan 6, 2025

t-ramz Jan 6, 2025

t-ramz Jan 6, 2025

t-ramz Jan 6, 2025

t-ramz Jan 6, 2025

JoshuaSBrown Jan 6, 2025

t-ramz Jan 6, 2025

t-ramz Jan 6, 2025

JoshuaSBrown Jan 6, 2025 •

edited

Loading

1180 refactor foxx authz #1181

1180 refactor foxx authz #1181

Conversation

JoshuaSBrown commented Dec 13, 2024 • edited Loading

PR Description

Tasks

Summary by Sourcery

sourcery-ai bot commented Dec 13, 2024 • edited Loading

Reviewer's Guide by Sourcery

Sequence diagram for the authorization check process

Class diagram for the refactored authorization system

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

JoshuaSBrown commented Dec 18, 2024

sourcery-ai bot left a comment

Choose a reason for hiding this comment

sourcery-ai bot Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sourcery-ai bot Dec 18, 2024

Choose a reason for hiding this comment

AronPerez left a 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

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

JoshuaSBrown Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

JoshuaSBrown commented Dec 13, 2024 •

edited

Loading

sourcery-ai bot commented Dec 13, 2024 •

edited

Loading

JoshuaSBrown Jan 6, 2025 •

edited

Loading