-
Notifications
You must be signed in to change notification settings - Fork 147
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
Permissions for access requests #2242
Conversation
@@ -252,6 +252,21 @@ <h2 id="files-heading">{{ _('Files') }}</h2> | |||
<p>{{ _("Reason") }}: {{ record.access.embargo.reason }}</p> | |||
{% endif %} | |||
|
|||
{%- if not current_user.is_anonymous %} | |||
{# TODO allow a similar workflow for guests #} | |||
<hr style="border-color: darkred;"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
styling: in general we consider the inline styling as a bad practice (fe. some stricter content security policies will throw an error), we use the semantic-ui based theme for this, pure html: https://semantic-ui.com/
and react: https://react.semantic-ui.com/,
it will help with removing the multiple paragraphs if you use semantic ui headers and containers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, this was just a very quick & dirty hack to make something work
invenio_app_rdm/records_ui/templates/semantic-ui/invenio_app_rdm/records/detail.html
Outdated
Show resolved
Hide resolved
) | ||
}} | ||
|
||
<div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above, semantic ui classes are better fit here :)
@@ -137,6 +137,19 @@ def user_dashboard_request_view(request, **kwargs): | |||
request_is_accepted=request_is_accepted, | |||
) | |||
|
|||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @anikachurilova I think this will be modified already in your PR, could you confirm ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to me that render template of is_draft_submission
and is_record_inclusion
can be used here as well as I used it for Upgrade Legacy Record
request (https://github.com/zenodo/zenodo-rdm/pull/354/files#diff-4d09ad616277a10edaf2a5b5a11eef4e1aebd90e1ab43f95e2b7114c8eeacf88R14).
No need to add a new one
@max-moser check here how adjusted the renders by topics (feel free to review and comment if something seems wrong :)) https://github.com/inveniosoftware/invenio-app-rdm/pull/2225/files#diff-4a94629a771abc41b540cb59d363d886634610dd06924aad8305ccae9250ced7R104
invenio_app_rdm/theme/assets/semantic-ui/js/invenio_app_rdm/landing_page/index.js
Outdated
Show resolved
Hide resolved
|
||
const axiosWithConfig = axios.create(apiConfig); | ||
const recid = requestAccessButton.dataset.recid; | ||
const result = await axiosWithConfig.post(`/api/records/${recid}/access/request`, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a new endpoint? because we already have api/records/<recid>/requests
reachable inside the record link, ie. record.links.requests, should we reuse that one ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, POST /api/records/<recid>/access/requests
and POST /api/records/<recid>/access/requests/guest
are the new endpoints for creating access requests.
invenio_app_rdm/theme/assets/semantic-ui/js/invenio_app_rdm/landing_page/index.js
Outdated
Show resolved
Hide resolved
|
||
// TODO it would be nicer to have the backend report the proper `links.self_html` | ||
// instead of constructing the URL here | ||
const url = `/me/requests/${id}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, we need rest API to give us the link :)
invenio_app_rdm/theme/assets/semantic-ui/js/invenio_app_rdm/landing_page/index.js
Outdated
Show resolved
Hide resolved
c3f750c
to
8654b02
Compare
8654b02
to
6630bf5
Compare
invenio_app_rdm/config.py
Outdated
@@ -1177,3 +1178,7 @@ class NotificationsUserSchema(UserSchema): | |||
|
|||
NOTIFICATIONS_SETTINGS_VIEW_FUNCTION = notification_settings | |||
"""View function for notification settings.""" | |||
|
|||
from invenio_rdm_records.services.permissions import RDMRequestsPermissionPolicy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is required because we need to tweak the requests permission policy slightly for guest-based access requests
426d971
to
3160f3b
Compare
This PR includes:
Some functionality is still missing:
|
d3b67c7
to
0d15e57
Compare
UpdateThis PR has been split into three separate PRs, dealing with...
|
b13a9a5
to
314fe5a
Compare
* set requests permission policy defined in rdm-records * this is necessary to allow guests to access the guest request access details via the access request tokens * also, schedule cleanup of expired access request tokens
314fe5a
to
820548e
Compare
Update regarding the structure of this PR
This PR has been split into three different PRs, more details are available in the comment below.
Overview
This PR requires inveniosoftware/invenio-rdm-records#1306 to work.
It provides a UI for the access requests detail pages, as well as a button for requesting access from restricted records' landing pages.
To do
Some thoughts
Users should be able to remove access grants that give themselves access to the record (i.e. where the current user is the subject), but the UI should warn them beforehand ("are you really sure? this might lock you out").