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

access request: add secret_link_expiration to guest access request payload schema #1410

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,14 @@
Changes
=======

Version 4.15.0 (2023-08-24)

- access request: add secret_link_expiration to guest access request payload schema
- permissions: add create/update conditions for managing access options
- views: add error handlers to the blueprint
- access request: add permission on secret_link_expiration request field


Version 4.14.0 (2023-08-17)

- alembic: add recipe for files and media files versioning
Expand Down
2 changes: 1 addition & 1 deletion invenio_rdm_records/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,6 @@

from .ext import InvenioRDMRecords

__version__ = "4.14.0"
__version__ = "4.15.0"

__all__ = ("__version__", "InvenioRDMRecords")
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ def __init__(self, settings_dict):
self.allow_user_requests = settings_dict.get("allow_user_requests", False)
self.allow_guest_requests = settings_dict.get("allow_guest_requests", False)
self.accept_conditions_text = settings_dict.get("accept_conditions_text", None)
self.secret_link_expiration = settings_dict.get("secret_link_expiration", None)
self.secret_link_expiration = settings_dict.get("secret_link_expiration", 0)

def dump(self):
"""Dump the record as dictionary."""
Expand Down
78 changes: 71 additions & 7 deletions invenio_rdm_records/requests/access/requests.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,19 @@
# it under the terms of the MIT License; see LICENSE file for more details.

"""Access requests for records."""
from datetime import datetime, timedelta

import marshmallow as ma
from flask import current_app, g
from invenio_access.permissions import authenticated_user, system_identity
from invenio_i18n import lazy_gettext as _
from invenio_mail.tasks import send_email
from invenio_records_resources.services.uow import Operation, RecordCommitOp
from invenio_requests import current_events_service
from invenio_requests.customizations import RequestType, actions
from marshmallow import fields
from invenio_requests.customizations.event_types import CommentEventType
from marshmallow import ValidationError, fields, validates
from marshmallow_utils.permissions import FieldPermissionsMixin

from ...proxies import current_rdm_records_service as service

Expand Down Expand Up @@ -58,7 +63,9 @@ class GuestSubmitAction(actions.SubmitAction):

def execute(self, identity, uow):
"""Execute the submit action."""
self.request["title"] = self.request.topic.resolve().metadata["title"]
record = self.request.topic.resolve()
self.request["title"] = record.metadata["title"]

super().execute(identity, uow)


Expand All @@ -82,11 +89,22 @@ def execute(self, identity, uow):
"origin": f"request:{self.request.id}",
}

# secret link will never expire if secret_link_expiration is empty
days = int(payload["secret_link_expiration"])
# TODO date calculation could be done elsewhere ?
if days:
data["expires_at"] = (
Copy link
Contributor

Choose a reason for hiding this comment

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

check this - probably not needed, it refers to guest access request expiration date, not the access link

Copy link
Contributor

Choose a reason for hiding this comment

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

checked - this is accept action, it should be here

(datetime.utcnow() + timedelta(days=days)).date().isoformat()
)
link = service.access.create_secret_link(identity, record.id, data)

access_url = f"{record.links['self_html']}?token={link._link.token}"
plain_message = _("Access the record here: %(url)s", url=access_url)

plain_message = _("Access the record here: {url}".format(url=access_url))
message = _(
'Click <a href="%(url)s">here</a> to access the record.', url=access_url
'Click <a href="{url}">here</a> to access the record.'.format(
url=access_url
)
)

uow.register(RecordCommitOp(record._record.parent))
Expand All @@ -101,6 +119,22 @@ def execute(self, identity, uow):

super().execute(identity, uow)

confirmation_message = {
"payload": {
"content": 'Click <a href="{url}">here</a> to access the record.'.format(
url=access_url
)
}
}
current_events_service.create(
system_identity,
self.request.id,
confirmation_message,
CommentEventType,
uow=uow,
notify=False,
)


class UserAcceptAction(actions.AcceptAction):
"""Accept action."""
Expand Down Expand Up @@ -175,15 +209,44 @@ class GuestAccessRequest(RequestType):
allowed_receiver_ref_types = ["user", "community"]
allowed_topic_ref_types = ["record"]

@classmethod
def _create_payload_cls(cls):
zzacharo marked this conversation as resolved.
Show resolved Hide resolved
class PayloadBaseSchema(ma.Schema, FieldPermissionsMixin):
field_load_permissions = {
"secret_link_expiration": "manage_access_options",
}

class Meta:
unknown = ma.RAISE

cls.payload_schema_cls = PayloadBaseSchema

def _update_link_config(self, **context_vars):
"""Fix the prefix required for "self_html"."""
identity = context_vars.get("identity", g.identity)
prefix = "/me"
if authenticated_user not in identity.provides:
prefix = "/access-requests"

if hasattr(g, "identity"):
identity = context_vars.get("identity", g.identity)

if authenticated_user not in identity.provides:
prefix = "/access-requests"

return {"ui": context_vars["ui"] + prefix}

@validates("secret_link_expiration")
def _validate_days(self, value):
try:
if int(value) < 0:
raise ValidationError(
message="Not a valid number of days.",
Copy link
Member

Choose a reason for hiding this comment

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

translate?

field_name="secret_link_expiration",
)
except ValueError:
raise ValidationError(
message="Not a valid number of days.",
Copy link
Member

Choose a reason for hiding this comment

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

translate?

field_name="secret_link_expiration",
)

available_actions = {
"create": actions.CreateAction,
"submit": GuestSubmitAction,
Expand All @@ -200,4 +263,5 @@ def _update_link_config(self, **context_vars):
"full_name": fields.String(required=True),
"token": fields.String(required=True),
"message": fields.String(required=False),
"secret_link_expiration": fields.String(required=True),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"secret_link_expiration": fields.String(required=True),
"secret_link_expiration": fields.String(required=True),
"field_load_permissions" : {
"secret_link_expiration": "manage_access_options",
}

Copy link
Contributor

Choose a reason for hiding this comment

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

possible to be done with class PayloadBaseSchema(ma.Schema, FieldPermissionsMixin) in the core

Copy link
Contributor

Choose a reason for hiding this comment

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

mutuall decided to leave it as it is since this class attribute is cleaner if contains only fields, no other class attributes

}
7 changes: 7 additions & 0 deletions invenio_rdm_records/requests/entity_resolvers.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
ServiceResultProxy,
ServiceResultResolver,
)
from invenio_users_resources.services.schemas import SystemUserSchema
from sqlalchemy.orm.exc import NoResultFound

from invenio_rdm_records.services.config import RDMRecordServiceConfig
Expand Down Expand Up @@ -101,6 +102,11 @@ def ghost_record(self, value):
"""Return the ghost representation of the unresolved value."""
return value

def system_record(self):
"""Return the representation of system user."""
default_constant_values = {}
return SystemUserSchema().dump(default_constant_values)

def get_needs(self, ctx=None):
"""Get the needs provided by the entity."""
return []
Expand All @@ -114,6 +120,7 @@ class EmailResolver(EntityResolver):
"""Resolver for email addresses."""

type_id = "email"
type_key = "email" # TODO: hack to make this entity resolver work in notifications

def __init__(self):
"""Constructor."""
Expand Down
25 changes: 9 additions & 16 deletions invenio_rdm_records/services/access/service.py
Original file line number Diff line number Diff line change
Expand Up @@ -615,13 +615,14 @@ def create_user_access_request(
def create_guest_access_request_token(
self, identity, id_, data, expand=False, uow=None
):
"""Create an request token that can be used to create an access request."""
"""Create a request token that can be used to create an access request."""
# Permissions
if authenticated_user in identity.provides:
raise PermissionDeniedError("request_guest_access")

record = self.record_cls.pid.resolve(id_)
if current_app.config.get("MAIL_SUPPRESS_SEND", False):
# TODO should be handled globally, not here, maybe EmailOp?
current_app.logger.warn(
"Cannot proceed with guest based access request - "
"email sending has been disabled!"
Expand All @@ -636,6 +637,7 @@ def create_guest_access_request_token(
)

# Create the URL for the email verification endpoint
# TODO why replace api?
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the comment...is it still relevant?

verify_url = url_for(
"invenio_rdm_records_ext.verify_access_request_token",
_external=True,
Expand Down Expand Up @@ -680,7 +682,7 @@ def create_guest_access_request(self, identity, token, expand=False, uow=None):

access_token = AccessRequestToken.get_by_token(token)
if access_token is None:
return None
return

access_token_data = access_token.to_dict()
record = self.record_cls.pid.resolve(access_token_data["record_pid"])
Expand All @@ -703,14 +705,16 @@ def create_guest_access_request(self, identity, token, expand=False, uow=None):

if requests:
raise DuplicateAccessRequestError([str(r.id) for r in requests])

data = {
"payload": {
"permission": "view",
"email": access_token_data["email"],
"full_name": access_token_data["full_name"],
"token": access_token_data["token"],
"message": access_token_data.get("message") or "",
"secret_link_expiration": str(
record.parent.access.settings.secret_link_expiration
),
}
}

Expand All @@ -719,9 +723,6 @@ def create_guest_access_request(self, identity, token, expand=False, uow=None):
if record_owner:
receiver = record_owner

if receiver is None:
pass

access_token.delete()
request = current_requests_service.create(
system_identity,
Expand All @@ -730,21 +731,13 @@ def create_guest_access_request(self, identity, token, expand=False, uow=None):
receiver,
creator=data["payload"]["email"],
topic=record,
expires_at=None,
expires_at=None, # TODO expire request ?
expand=expand,
uow=uow,
)

if request.errors:
return request

prefix = _(
"%(full_name)s (%(email)s) commented",
full_name=access_token_data["full_name"],
email=data["payload"]["email"],
)
message = data["payload"].get("message") or ""
comment = {"payload": {"content": f"{prefix}: {message}"}}
comment = {"payload": {"content": message}}

return current_requests_service.execute_action(
system_identity,
Expand Down
8 changes: 8 additions & 0 deletions invenio_rdm_records/services/generators.py
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,14 @@ def needs(self, record=None, file_key=None, **kwargs):
return []


class IfCreate(ConditionalGenerator):
"""Check if the request is created or modified."""

def _condition(self, record=None, request=None, **kwargs):
"""Check if record is empty - meaning it is created for the first time ."""
return record is None and request is None
Copy link
Member

Choose a reason for hiding this comment

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

do we need both to be None? Maybe a more detailed comment on when is this happening?

Copy link
Contributor

@kpsherva kpsherva Aug 23, 2023

Choose a reason for hiding this comment

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

I will add a comment, I think I have described it in another place
it is because in the services and permissions we usually pass record=community, record=record, etc, assuming that the parent classes and all the dependent classes understand record as an argument. However, for some reason we decided to make request=request in the requests service, and there are some classes in permissions expecting record as an argument and all hell breaks loose.
I have to find all the places we have request instead of record as a name of the argument and normalise it



class IfRequestType(ConditionalGenerator):
"""Conditional generator for requests of a certain type."""

Expand Down
19 changes: 19 additions & 0 deletions invenio_rdm_records/services/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
SystemProcess,
)
from invenio_records_permissions.policies.records import RecordPermissionPolicy
from invenio_requests.services.generators import Creator, Receiver, Status
from invenio_requests.services.permissions import (
PermissionPolicy as RequestPermissionPolicy,
)
Expand All @@ -26,6 +27,7 @@
from .generators import (
AccessGrant,
GuestAccessRequestToken,
IfCreate,
IfExternalDOIRecord,
IfFileIsLocal,
IfNewRecord,
Expand Down Expand Up @@ -262,3 +264,20 @@ class RDMRequestsPermissionPolicy(RequestPermissionPolicy):
can_update = RequestPermissionPolicy.can_update + [guest_token]
can_action_submit = RequestPermissionPolicy.can_action_submit + [guest_token]
can_action_cancel = RequestPermissionPolicy.can_action_cancel + [guest_token]
can_create_comment = can_read
can_update_comment = RequestPermissionPolicy.can_update_comment + [guest_token]
can_delete_comment = RequestPermissionPolicy.can_delete_comment + [guest_token]

# manages GuessAccessRequest payload permissions
can_manage_access_options = [
IfCreate(
then_=[SystemProcess()],
else_=[
IfRequestType(
GuestAccessRequest,
then_=[Status(["submitted"], [Receiver()])],
else_=Disable(),
)
],
)
]
15 changes: 1 addition & 14 deletions invenio_rdm_records/services/schemas/parent/access.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,20 +71,7 @@ class AccessSettingsSchema(Schema):
allow_guest_requests = fields.Boolean()

accept_conditions_text = SanitizedHTML(allow_none=True)
secret_link_expiration = fields.Integer(
validate=validate.Range(max=365), allow_none=True
)

@pre_load
def translate_expiration_date(self, data, **kwargs):
"""Translate secret_link_expiration from ui dropdown value."""
expiration_days = data["secret_link_expiration"]
if expiration_days == 0:
data["secret_link_expiration"] = None
else:
data["secret_link_expiration"] = expiration_days

return data
secret_link_expiration = fields.Integer(validate=validate.Range(max=365, min=0))


class ParentAccessSchema(Schema, FieldPermissionsMixin):
Expand Down
1 change: 1 addition & 0 deletions invenio_rdm_records/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ def get(self, key, default=None):
def verify_token(identity):
"""Verify the token and provide identity with corresponding need."""
token = request.args.get("token", session.get("token", None))

session["token"] = token
if token:
try:
Expand Down
12 changes: 12 additions & 0 deletions invenio_rdm_records/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,25 @@
from invenio_access.permissions import system_identity
from invenio_i18n import lazy_gettext as _
from invenio_mail.tasks import send_email
from invenio_pidstore.errors import PIDDeletedError, PIDDoesNotExistError
from invenio_records_resources.services.errors import PermissionDeniedError
from invenio_requests.proxies import current_requests_service
from invenio_requests.views.decorators import pass_request
from invenio_requests.views.ui import (
not_found_error,
record_permission_denied_error,
record_tombstone_error,
)

from .proxies import current_rdm_records_service as current_service
from .requests.access.requests import GuestAcceptAction
from .services.errors import DuplicateAccessRequestError

blueprint = Blueprint("invenio_rdm_records_ext", __name__)
# Register error handlers
blueprint.register_error_handler(PermissionDeniedError, record_permission_denied_error)
blueprint.register_error_handler(PIDDeletedError, record_tombstone_error)
blueprint.register_error_handler(PIDDoesNotExistError, not_found_error)


@blueprint.record_once
Expand Down Expand Up @@ -122,6 +133,7 @@ def read_request(request, **kwargs):
f"invenio_requests/{request_type}/index.html",
user_avatar="",
record=None,
permissions={},
invenio_request=request.to_dict(),
request_is_accepted=request_is_accepted,
)
Expand Down
Loading
Loading