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.

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

Update permissions aren't being enforced in registry server #4444

Closed
tokoko opened this issue Aug 25, 2024 · 3 comments · Fixed by #4449
Closed

Update permissions aren't being enforced in registry server #4444

tokoko opened this issue Aug 25, 2024 · 3 comments · Fixed by #4449

Comments

@tokoko
Copy link
Collaborator

tokoko commented Aug 25, 2024

Current Behavior

apply_x methods in registry server check permissions on the object that's passed in the request. This is correct for object creation, but overlooks updates. During updates we should be checking permissions on the existing object as well. With current behavior, user is allowed to overwrite objects even when updates aren't allowed.

Possible Solution

Pseudocode for a typical apply method should look something like this:

  • Check if the object already exists in the registry
  • Assert update permission on the existing object (if it exists)
  • Assert create permission on the object contained in the request
  • Proceed with apply
@dmartinol
Copy link
Contributor

@tokoko could you explain why the following code is not covering the update use case?

        self.proxied_registry.apply_entity(
            entity=cast(
                Entity,
                assert_permissions(
                    resource=Entity.from_proto(request.entity),
                    actions=CRUD,
                ),
            ),
            project=request.project,
            commit=request.commit,
        )

IIUC, it runs the assert check before trying the apply, so the update should not happen if not authorized.

@tokoko
Copy link
Collaborator Author

tokoko commented Aug 26, 2024

The problem isn't the order, it's the fact that assert is happening on a wrong object. Say I have an update permission on objects tagged like {"team": "A"} and I'm applying an update to an entity that already exists and is tagged like {"team": "B"}. The code above checks only the fact that I'm authorized to create entities of team A and overwrites team B's object because existing object is never touched during permission assertion.

There should be two separate asserts, one on the object that's already in the registry and another one on the object that's supplied as part of the request.

@dmartinol
Copy link
Contributor

Please TAL at #4449

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants