Make event ACL editable via Tobira #1272

owi92 · 2024-11-05T17:31:19Z

This will allow users to update event ACL in Opencast via Tobira. Doing so will trigger the republish-metadata workflow, so the changes will be propagated to all publications.

Closes #1264
_{can be reviewed commit by commit}

oas777 · 2024-11-05T17:33:53Z

It TRIGGERS the republish-metadata workflow? Automatically? No additional action needed? Thank you!

owi92 · 2024-11-05T17:42:55Z

Yes, correct. Though that might still fail, or take some time.
To reflect the changes in Tobira immediately, it will also be written to our DB, without having to wait for the workflow and the following sync... but that's getting into technicalities.

So tldr: In a perfect world, there's no additional action needed ;)

oas777 · 2024-11-07T16:25:00Z

Ole, is https://pr1272.tobira.opencast.org/ still the deployment I should be looking at? If so, I have two minor (!) remarks:

The confirmation for having saved my changes by greying out the buttons "Reset" and "Save" is very low-key because it's grey to start with - shouldn't it be blueish, then grey out? Cf.

Shouldn't the symbol for "Access policy" have a checkmark inside the shield?

owi92 · 2024-11-08T11:50:36Z

is https://pr1272.tobira.opencast.org/ still the deployment I should be looking at

Yes. Thank you for the feedback, I think these are good points. Will change accordingly.

github-actions · 2024-11-18T16:58:23Z

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

Doing this whenever I spot some.

This adds a mutation that will send a PUT request to Opencast to update the acl of a given event. When the request responds with 204, it is considered successful and the updated acl is stored in Tobira's DB, without waiting for sync with OC. That allows us to pretend that things are happening much faster than they do in actuality, as Opencast needs to run the "republish metadata" workflow in order to propagate these changes back to Tobira. There were some discussions and efforts to change how that is done in Opencast, but the proposed changes didn't make the cut yet. Once that or sth similar happens, we should also adapt the mechanism implemented in this commit.

This adds the necessary changes for the interface to commit the mutation added in the previous commit. Also fixes some minor UX issues.

So far this is only needed for republishing metadata in case of acl updates. We might need this in other places, so I made a dedicated function for it. Of course we might also remove it again, if future Opencast changes make it redundant. We'll see.

Not sure if the default should be `true` or `false` for this.

This endpoint allows us to check whether there are any workflows running in Opencast for an event. Also added are error handling and blocking of acl editing based on that check.

Previously this used the good old i18next t function, but that doesn't correctly display some html tags like `<br />` when used inside the translation strings. This changes the component to use i18next Trans component instead, with which the tags are displayed correctly.

LukasKalbertodt

Nice, this will add a big and long awaited feature! Just a bunch of notes, as far as I remember all should be pretty easy to fix.

LukasKalbertodt · 2024-11-20T08:23:02Z

backend/src/api/model/event.rs

+            .await
+            .map_err(|e| {
+                error!("Failed to send acl update request: {}", e);
+                err::opencast_unavailable!("Failed to communicate with Opencast")


Also put "Failed to send acl update request" in the second line? It's a more specific error

LukasKalbertodt · 2024-11-20T08:25:37Z

backend/src/api/model/event.rs

+                "Failed to update event acl, OC returned status: {}",
+                response.status()
+            );
+            Err(err::opencast_unavailable!("Opencast API error: {}", response.status()))


Mh opencast_unavailable is the wrong error here. Seems like we need to add another ApiErrorKind, sth generic like OpencastError, representing everything that can go wrong when communicating with Opencast (except for "unreachable" things which are already covered)? Or we can also use InternalServerError here? Its just "error outside the control of the API user"?

LukasKalbertodt · 2024-11-20T08:28:04Z

backend/src/api/model/acl.rs

+
+#[derive(Debug, GraphQLInputObject, Serialize)]
+pub(crate) struct AclInput {
+    pub allow: bool,


This allow flag does not need to be in our API, right? I know it's something Opencast expect in its API, but we never set it to false, so it doesn't make sense to send a constant between frontend and backend. This flag should be added at the latest possible state, just to make the OC API happy.

I also noticed that this "format" of ACL is not native to either the backend or frontend, meaning: the frontend has a loop to convert "its format" to the API format, and the backend has the same to convert it from API format to a format that we can pass to the DB.
We already have ACLs in our API at one other point: AclItem. And there each item is { role: string, actions: string[], info: ... }. The info part we could ignore, but what if we change AclInput to:

struct AclInputEntry { pub role: String, pub actions: Vec<String>, }

Then it's closer to what we already have in the API and the frontend conversion would be easier, as its closer to its "native" format. Might make the backend conversion more complicated. Ah I don't know, this second point is probably not too important...

LukasKalbertodt · 2024-11-20T08:31:30Z

backend/src/api/model/event.rs

+            let roles_for_action = |target_action: &str| -> Vec<String> {
+                acl.iter()
+                    .filter(|entry| entry.allow && entry.action == target_action)
+                    .map(|entry| entry.role.clone())
+                    .collect()
+            };
+            let read_roles = roles_for_action("read");
+            let write_roles = roles_for_action("write");


How about

let mut map = HashMap::new(); for e in acl { map.entry(entry.action).or_insert(vec![]).push(entry.role); } let read_roles = map.get("read").unwrap_or_default(); let write_roles = map.get("write").unwrap_or_default();

Mh ok, just 2 lines shorter, but it at least only loops through the thing once. But eh, probably whatever.

LukasKalbertodt · 2024-11-20T08:34:20Z

backend/src/api/mutation.rs

+    /// Updates the acl of a given event by sending a PUT request to Opencast. If the request is
+    /// successful (as indicated by the response code 204), the updated acl is already stored in Tobira
+    /// without waiting for an upcoming sync - however this means it might get overwritten again if
+    /// the update in Opencast failed for some reason.
+    /// This solution should be improved in the future.


I feel like the "PUT" and "204" parts are overly specific for API documentation and just implementation detail. Maybe "... by sending the changes to Opencast. If successful, the updated ACL are stored in Tobira..."

LukasKalbertodt · 2024-11-20T11:01:00Z

backend/src/sync/client.rs

+        let pq = format!("/workflow/mediaPackage/{oc_id}/hasActiveWorkflows");
+        let req = self.authed_req_builder(&self.external_api_node, &pq)


Mh so, this is not using the "external API" anymore (those paths would start with /api). So not sure if it's fine to always use the external_api_node. I just asked in the community chat and hopefully it is fine! Otherwise we would need to add another node config or.. sth.

Edit: Ok so it seems like this API is always available on the admin node. So we can't really use external_api_node I am afraid :/ We also don't have a fitting node already, so I fear we must add a new node config. Not sure if we should call it admin_node or workflow_api_node or sth... mh.

The other thing is that the stability of this API is not guaranteed too much. I'm sure lots of other apps use it, but maybe Tobira should be able to deal with the case that the API doesn't exist? Maybe in that case just don't block editing ACL? Mhhh

LukasKalbertodt · 2024-11-20T11:04:06Z

backend/src/sync/client.rs

+    pub async fn has_active_workflows(&self, oc_id: &String) -> Result<bool> {
+        let pq = format!("/workflow/mediaPackage/{oc_id}/hasActiveWorkflows");
+        let req = self.authed_req_builder(&self.external_api_node, &pq)
+            .header(http::header::CONTENT_TYPE, "application/x-www-form-urlencoded")


Is this header necessary?

LukasKalbertodt · 2024-11-20T11:07:31Z

frontend/src/i18n/locales/en.yaml

+      Changing the access policy is not possible at this time, since the video is being processed in the background.
+      <br />
+      Please try again later. To try again now, please reload the page.


The last sentence sounds a bit odd to me. I would probably remove it and then also remove the <br /> to put everything in one line. But no strong opinion.

With <br /> removed, I suppose the commit about adjusting the error display can be removed?

LukasKalbertodt · 2024-11-20T11:14:10Z

frontend/src/routes/manage/Video/Shared.tsx

@@ -31,6 +31,7 @@ export const makeManageVideoRoute = (
    page: ManageVideoSubPageType,
    path: string,
    render: (event: AuthorizedEvent, data: QueryResponse) => JSX.Element,
+    options?: { onAccessRoute?: boolean },


I would probably call this just sth like fetchWorkflowState or sth more direct?

LukasKalbertodt · 2024-11-20T11:21:52Z

frontend/src/util/err.tsx

+                // @ts-expect-error: Dynamically passed i18next keys need to be typed
+                // more strictly than just `string`. But I don't want to do that for all
+                // possible errors.
+                ? <Trans i18nKey={causes[0]} />


That's not too bad. There is one type one can simply import that represents all valid translation keys. If only I remembered the name of the type :D I'm sure we use it somewhere already

owi92 added area:api API (design and backend implementation) changelog:user User facing changes labels Nov 5, 2024

github-actions bot temporarily deployed to test-deployment-pr1272 November 5, 2024 17:39 Destroyed

owi92 marked this pull request as draft November 8, 2024 12:54

owi92 force-pushed the event-acl-editing branch from 7a3e745 to faa8d96 Compare November 11, 2024 20:56

github-actions bot temporarily deployed to test-deployment-pr1272 November 11, 2024 21:02 Destroyed

owi92 force-pushed the event-acl-editing branch from faa8d96 to 5ccb07d Compare November 11, 2024 21:04

github-actions bot temporarily deployed to test-deployment-pr1272 November 11, 2024 21:07 Destroyed

owi92 marked this pull request as ready for review November 11, 2024 21:20

github-actions bot added the status:conflicts This PR has conflicts that need to be resolved label Nov 18, 2024

Remove some leading/trailing whitespace

aeab1a3

Doing this whenever I spot some.

owi92 force-pushed the event-acl-editing branch from 5ccb07d to b6a45bd Compare November 19, 2024 11:14

github-actions bot removed the status:conflicts This PR has conflicts that need to be resolved label Nov 19, 2024

github-actions bot temporarily deployed to test-deployment-pr1272 November 19, 2024 11:19 Destroyed

owi92 added 7 commits November 19, 2024 12:28

Adapt ACL editing UI for actual functionality

86045de

This adds the necessary changes for the interface to commit the mutation added in the previous commit. Also fixes some minor UX issues.

Make the option of ACL editing configurable

79c92e3

Not sure if the default should be `true` or `false` for this.

Add endpoint to check workflow activity

beead64

This endpoint allows us to check whether there are any workflows running in Opencast for an event. Also added are error handling and blocking of acl editing based on that check.

Change acl icon and button colors

cce8143

owi92 force-pushed the event-acl-editing branch from b6a45bd to cce8143 Compare November 19, 2024 11:28

github-actions bot deployed to test-deployment-pr1272 November 19, 2024 11:31 View deployment

LukasKalbertodt requested changes Nov 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make event ACL editable via Tobira #1272

Make event ACL editable via Tobira #1272

owi92 commented Nov 5, 2024

oas777 commented Nov 5, 2024

owi92 commented Nov 5, 2024

oas777 commented Nov 7, 2024 •

edited

Loading

owi92 commented Nov 8, 2024

github-actions bot commented Nov 18, 2024

LukasKalbertodt left a comment

LukasKalbertodt Nov 20, 2024

LukasKalbertodt Nov 20, 2024

LukasKalbertodt Nov 20, 2024

LukasKalbertodt Nov 20, 2024

LukasKalbertodt Nov 20, 2024

LukasKalbertodt Nov 20, 2024

LukasKalbertodt Nov 20, 2024

LukasKalbertodt Nov 20, 2024

LukasKalbertodt Nov 20, 2024

LukasKalbertodt Nov 20, 2024

		let pq = format!("/workflow/mediaPackage/{oc_id}/hasActiveWorkflows");
		let req = self.authed_req_builder(&self.external_api_node, &pq)

Make event ACL editable via Tobira #1272

Are you sure you want to change the base?

Make event ACL editable via Tobira #1272

Conversation

owi92 commented Nov 5, 2024

oas777 commented Nov 5, 2024

owi92 commented Nov 5, 2024

oas777 commented Nov 7, 2024 • edited Loading

owi92 commented Nov 8, 2024

github-actions bot commented Nov 18, 2024

LukasKalbertodt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oas777 commented Nov 7, 2024 •

edited

Loading