Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

JSON Patch Operation Read Permission Issue #2986

Open
justin-tay opened this issue May 18, 2023 · 2 comments · May be fixed by #3040
Open

JSON Patch Operation Read Permission Issue #2986

justin-tay opened this issue May 18, 2023 · 2 comments · May be fixed by #3040

Comments

@justin-tay
Copy link
Collaborator

Encountering an issue where running a transaction with 2 operations fails with a read permission denied error which if split into 2 separate transactions succeeds.

Expected Behavior

There shouldn't be a difference in outcome if the 2 separate operations in 2 separate transactions succeeds but the same 2 operations in a single transaction fails.

Current Behavior

This is happening partially because relationships are updated in a separate step.

I have a sample project where I have defined 3 models.

  • Author -> AuthorBook -> Book

where AuthorBook is an entity and has a one to one relationship with Author and Book.

Author has a read permission that allows read if the user has access to Book. (The sample hardcodes this to allow access if the Book has id 1 or 2)

After the setup the state looks like this

  • Book(b1)
  • Book(b2)
  • Author(a1) with AuthorBook(a1,b1)

The operations executed essentially adds AuthorBook(a1,b2) to Author and remove AuthorBook(a1,b1). When executing the transaction the failure occurs when the post processing of the relationships are being performed when Author needs to be read to apply the relationships.

The state of Author at the point of time the expression is evaluated is

  • Author(a1) with AuthorBook(a1,null) which fails because the user doesn't have access to either b1 or b2 at this point in time

This is because AuthorBook(a1,b1) was already removed. In the prior step when AuthorBook(a1,b2) was to be set on Author what was set was AuthorBook(a1,null) because it was split into the 2 phases.

Possible Solution

I don't really know enough of how the permission evaluation works but if all inline checks are deferred, similarly to what's happening for newly created resources, when performing postProcessRelationships then it should work. I don't know if this is a proper design or will break anything else though.

Steps to Reproduce (for bugs)

I have committed a project at https://github.com/justin-tay/elide-spring-boot-example/tree/permission-issue_elide-7.x which demonstrates the issue. The queries are in the README.md but I have reproduced them below.

Setup

curl --location --request PATCH 'http://localhost:8080/api/v1' \
--header 'Content-Type: application/vnd.api+json; ext=jsonpatch' \
--data '[
    {
        "op": "add",
        "path": "/author",
        "value": {
            "type": "author",
            "id": "7f14b0c3-d0ed-436c-9843-6bfbf7342df6",
            "attributes": {
                "name": "Ernest Hemingway"
            }
        }
    },
    {
        "op": "add",
        "path": "/book",
        "value": {
            "type": "book",
            "id": "1",
            "attributes": {
                "title": "The Old Man and the Sea"
            },
            "relationships": {}
        }
    },
    {
        "op": "add",
        "path": "/book",
        "value": {
            "type": "book",
            "id": "2",
            "attributes": {
                "title": "For Whom the Bell Tolls"
            },
            "relationships": {}
        }
    },
    {
        "op": "add",
        "path": "/author/7f14b0c3-d0ed-436c-9843-6bfbf7342df6/authorBooks",
        "value": {
            "type": "authorBook",
            "id": "e2d0e822-d94d-4e36-a592-ef4f2af89f75",
            "attributes": {},
            "relationships": {
                "book": {
                    "data": {
                        "type": "book",
                        "id": "1"
                    }
                }
            }
        }
    }
]'

Operation with issue

curl --location --request PATCH 'http://localhost:8080/api/v1' \
--header 'Content-Type: application/vnd.api+json; ext=jsonpatch' \
--data '[
  {
    "op": "add",
    "path": "/author/1/authorBooks",
    "value": {
      "type": "authorBook",
      "id": "bd71bc1a-dc41-4adb-bf17-610149429f68",
      "attributes": {},
      "relationships": {
        "book": {
          "data": {
            "type": "book",
            "id": "2"
          }
        }
      }
    }
  },
  {
    "op": "remove",
    "path": "/author/1/authorBooks",
    "value": {
      "type": "authorBook",
      "id": "1",
      "attributes": {},
      "relationships": {}
    }
  }
]'
@justin-tay justin-tay linked a pull request Jul 25, 2023 that will close this issue
Draft
@aklish
Copy link
Member

aklish commented Aug 26, 2023

The danger in derring inline checks is that an attacker can overwrite the value of a field in a transaction. The check would have failed but because it was deferred to after their write, it now succeeds.

For example. Imagine I have a user record with a field called role. The attacker's role is READ_ONLY. They can create a transaction which overwrites the role to WRITE and then perform another operation that depends on that role. Because all checks are deferred until the end, they now can perform actions with the WRITE role.

@justin-tay
Copy link
Collaborator Author

Is there any way to fix this scenario then?

I see that actually the checks were already run once during the initial pass, so even with the pull request if it did the remove authorBook operation first instead of add the request would fail as expected.

If I deferred the check until the end of each post process relationship action instead of until the transaction commit would that work? Or is there some other way to handle this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Defer inline checks when processing relationships justin-tay/elide
2 participants
@aklish @justin-tay