Skip to content

Extra context for permission checks #440

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

Open
xsenyaaax wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Extra context for permission checks #440

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 26 additions & 8 deletions invenio_requests/services/events/service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ def create(
uow=None,
expand=False,
notify=True,
**kwargs
):
"""Create a request event.

Expand All @@ -55,7 +56,15 @@ def create(
:param dict data: Input data according to the data schema.
"""
request = self._get_request(request_id)
self.require_permission(identity, "create_comment", request=request)
self.require_permission(
identity,
"create_comment",
request=request,
data=data,
Copy link
Member

@zzacharo zzacharo Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you run specific permission checks on comment actions based on request and event_type? I am trying to understand if we don't overpass kwargs down to require_permission. Also, I would be interested to know on how you use them in your use cases :)

Copy link
Contributor

@mesemus mesemus Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Zach,

General intro (sorry, it will be a bit longer):

What we do in our overlay is that each community can have one or more associated workflows (one of them being "default"). The workflow handles the exact deposition/record lifecycle, regardless of whether the record is in draft or published state.

When a record is created within a community (we don't allow creating records outside of a community), we take the default workflow from the community and set a workflow attribute (string) on the record's parent. This workflow then identifies the exact deposition/record lifecycle workflow.

Additionally, each record has a state field (initially set to draft), and record-level permissions use it for checks.

Workflows are defined in Python (invenio.cfg) as a combination of record permissions and requests permissions:

WORKFLOWS = {
    "default": Workflow(
        label=_("Default workflow for normal communities"),
        permission_policy_cls=DefaultWorkflowPermissions,
        request_policy_cls=DefaultWorkflowRequests,
    ),
    ...
}

Example from the record permission policy part:

# piece of DefaultWorkflowPermissions

can_read = [
      IfInState(
          "published",
          then_=[
              IfRestricted(
                  "record",
                  then_=[PrimaryCommunityMembers()],
                  else_=[AnyUser()],
              )
          ],
      ),
      IfInState(
          "retracting",
          then_=[
              RecordOwner(),
              PrimaryCommunityRole("curator")
          ],
      ),
]

When permission is checked on a record (for example, can_read), the record's permission policy will look at the record's parent, find the workflow ID, resolve it to the record permission policy associated with the workflow, and execute can_read on it. There, IfInState looks at the state field in the record metadata, and if the record is in retracting state, only the record owner and curator are given access.

Now, requests

The second part of the workflow defines who can create requests, when they can create them, what events can occur on those requests, who can accept/decline them, etc. It also defines escalations if the request isn't handled within a specified time interval.

Example:

class GenericCommunityWorkflowRequests(WorkflowRequestPolicy):
    delete_published_record = WorkflowRequest(
        requesters=[
            IfNotHarvested(
                then_=IfInState(
                    "published",
                    then_=[
                        RecordOwners(),
                        PrimaryCommunityRole("curator"),
                        PrimaryCommunityRole("owner"),
                    ],
                ),
                else_=[SystemProcess()],
            )
        ],
        recipients=[
            IfRequestedBy(
                requesters=[
                    PrimaryCommunityRole("curator"),
                    PrimaryCommunityRole("owner"),
                ],
                then_=[AutoApprove()],
                else_=[PrimaryCommunityRole("curator")],
            )
        ],
        transitions=WorkflowTransitions(
            submitted="retracting",
            declined="published",
            accepted="deleted",
            cancelled="published",
        ),
        events={
            "event_type": WorkflowEvent(
                submitters=[
                    # generators that can use the topic, request, request_type, event_type
                ],
            )
        }
        escalations=[
            WorkflowRequestEscalation(
                after=timedelta(days=21),
                recipients=[
                    PrimaryCommunityRole("owner"),
                ],
            )
        ],
    )

Imagine that the owner of /api/records/abc would like to delete the record. With workflows, the user directly creates a request for that:

POST /api/records/abc/requests/delete_published_record

{...potentially some payload here, such as deletion reason...}

This in turn calls Invenio Request Service's create method, passing the payload and additionally the abc record in kwargs.

We've replaced the can_create permission of the Invenio Requests Service with:

    can_create = [
        SystemProcess(),
        RequestCreatorsFromWorkflow(),
    ]

For the POST above: RequestCreatorsFromWorkflow takes the abc record from the kwargs, looks up its workflow, finds delete_published_record in the workflow's requests configuration, and uses the requesters field.

If the user has permission, it will use the recipients property from the configuration above to determine who should be placed in the recipient field of the newly created request. An AutoApprove above is a special case - the request is accepted automatically.

When the request is submitted, the transitions part will automatically change the state field on the record to retracting, causing that only the owner and curator will have access to the record at this stage.

Finally, the escalations part will check in a Celery task that the request hasn't been handled for x days, will reassign the recipient of the request to a new person, and send notifications.

For events, it works the same way, using the submitters on the WorkflowEvent. We use different event types (not just comment events) during the lifecycle of the request - for example, if we have a repository with sensitive data where access to files is controlled by a data access committee, we'll use a special event type to record the committee's decision in a structured way.

Final note: we are not the ones that write the workflow policies - it is the owners of the repositories built on our overlay. That's why we would like to pass as complete a context to permissions (and components as well) as possible to enable them writing their possibly complex permission generators.

So that's why we need to have those kwargs and extra context :) We've been using this from our own fork for the last year or so. If it gets into Invenio main, we'll be able to remove our fork. If it doesn't fit RDM (for example, for security/API compactness considerations), I understand - no harm done, we'll just keep our patch.

Copy link
Member

@zzacharo zzacharo Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @mesemus thanks a lot for the amazing explanation! I see you have some really advanced customization which might be also interesting for the core. I understand now a bit better your use case but now I am intrigued to know a bit more (maybe a demo?) of your use case examples. Have you also changed the UI to accomodate all these different deposition workflows?

I am interested because I have started investigating (we are in the phase of conducting some interviews with our CERN stakeholders) potential solutions/improvements on the publication workflow we support currently on RDM. For example, we are working now to add "Reviewers" as a first class citizen of the request and in parallel thinking on how to improve the commenting capabilities on a request.

I will be off until 22nd of April but I would be interested to see in a demo your use case and consider if there is any common pathway to integrate fully or partially the customizable publication workflows.

Copy link
Contributor

@mesemus mesemus Apr 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zzacharo sure, let's move this to Discord to find a time slot.

event_type=event_type,
notify=notify,
**kwargs,
)

# Validate data (if there are errors, .load() raises)
schema = self._wrap_schema(event_type.marshmallow_schema())
Expand Down Expand Up @@ -93,12 +102,12 @@ def create(
expand=expand,
)

def read(self, identity, id_, expand=False):
def read(self, identity, id_, expand=False, **kwargs):
"""Retrieve a record."""
event = self._get_event(id_)
request = self._get_request(event.request_id)

self.require_permission(identity, "read", request=request)
self.require_permission(identity, "read", request=request, **kwargs)

return self.result_item(
self,
Expand All @@ -111,12 +120,19 @@ def read(self, identity, id_, expand=False):
)

@unit_of_work()
def update(self, identity, id_, data, revision_id=None, uow=None, expand=False):
def update(
self, identity, id_, data, revision_id=None, uow=None, expand=False, **kwargs
):
"""Update a comment (only comments can be updated)."""
event = self._get_event(id_)
request = self._get_request(event.request.id)
self.require_permission(
identity, "update_comment", request=request, event=event
identity,
"update_comment",
request=request,
event=event,
data=data,
**kwargs,
)
self.check_revision_id(event, revision_id)

Expand All @@ -143,15 +159,15 @@ def update(self, identity, id_, data, revision_id=None, uow=None, expand=False):
)

@unit_of_work()
def delete(self, identity, id_, revision_id=None, uow=None):
def delete(self, identity, id_, revision_id=None, uow=None, **kwargs):
"""Delete a comment (only comments can be deleted)."""
event = self._get_event(id_)
request_id = event.request_id
request = self._get_request(request_id)

# Permissions
self.require_permission(
identity, "delete_comment", request=request, event=event
identity, "delete_comment", request=request, event=event, **kwargs
)
self.check_revision_id(event, revision_id)

Expand Down Expand Up @@ -188,7 +204,9 @@ def search(

# Permissions - guarded by the request's can_read.
request = self._get_request(request_id)
self.require_permission(identity, "read", request=request)
self.require_permission(
identity, "read", request=request, params=params, **kwargs
)

# Prepare and execute the search
search = self._search(
Expand Down
41 changes: 32 additions & 9 deletions invenio_requests/services/requests/service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,19 @@ def create(
expires_at=None,
uow=None,
expand=False,
**kwargs,
):
"""Create a record."""
self.require_permission(identity, "create")
self.require_permission(
identity,
"create",
request_type=request_type,
receiver=receiver,
creator=creator,
record=topic,
expires_at=expires_at,
**kwargs,
)

# we're not using "self.schema" b/c the schema may differ per
# request type!
Expand Down Expand Up @@ -134,11 +144,11 @@ def create(
expand=expand,
)

def read(self, identity, id_, expand=False):
def read(self, identity, id_, expand=False, **kwargs):
"""Retrieve a request."""
# resolve and require permission
request = self.record_cls.get_record(id_)
self.require_permission(identity, f"read", request=request)
self.require_permission(identity, f"read", request=request, **kwargs)

# run components
for component in self.components:
Expand All @@ -156,13 +166,17 @@ def read(self, identity, id_, expand=False):
)

@unit_of_work()
def update(self, identity, id_, data, revision_id=None, uow=None, expand=False):
def update(
self, identity, id_, data, revision_id=None, uow=None, expand=False, **kwargs
):
"""Update a request."""
request = self.record_cls.get_record(id_)

self.check_revision_id(request, revision_id)

self.require_permission(identity, f"update", record=request, request=request)
self.require_permission(
identity, f"update", record=request, request=request, data=data, **kwargs
)

# we're not using "self.schema" b/c the schema may differ per
# request type!
Expand Down Expand Up @@ -194,15 +208,15 @@ def update(self, identity, id_, data, revision_id=None, uow=None, expand=False):
)

@unit_of_work()
def delete(self, identity, id_, uow=None):
def delete(self, identity, id_, uow=None, **kwargs):
"""Delete a request from database and search indexes."""
request = self.record_cls.get_record(id_)

# TODO do we need revisions for requests?
# self.check_revision_id(request, revision_id)

# check permissions
self.require_permission(identity, f"action_delete", request=request)
self.require_permission(identity, f"action_delete", request=request, **kwargs)

# run components
self.run_components("delete", identity, record=request, uow=uow)
Expand Down Expand Up @@ -238,7 +252,14 @@ def execute_action(

# Check permissions - example of permission: can_cancel_submitted
permission_name = f"action_{action}"
self.require_permission(identity, permission_name, request=request)
self.require_permission(
identity,
permission_name,
request=request,
action_obj=action,
data=data,
**kwargs,
)

# Check if the action *can* be executed (i.e. a given state transition
# is allowed).
Expand Down Expand Up @@ -279,7 +300,9 @@ def search_user_requests(
The user is able to search the requests that were created by them
they are the receiver or they got access via the requests topic e.g record sharing.
"""
self.require_permission(identity, "search_user_requests")
self.require_permission(
identity, "search_user_requests", params=params, **kwargs
)

# Prepare and execute the search
params = params or {}
Expand Down
Loading