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

Make event ACL editable via Tobira #1272

Open
wants to merge 8 commits into
base: next
Choose a base branch
from
Open
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
10 changes: 9 additions & 1 deletion backend/src/api/model/acl.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use juniper::GraphQLObject;
use juniper::{GraphQLInputObject, GraphQLObject};
use postgres_types::BorrowToSql;
use serde::Serialize;

use crate::{api::{util::TranslatedString, Context, err::ApiResult}, db::util::select};

Expand Down Expand Up @@ -94,3 +95,10 @@ where
}
}).await.map_err(Into::into)
}

#[derive(Debug, GraphQLInputObject, Serialize)]
pub(crate) struct AclInput {
pub allow: bool,
pub action: String,
Copy link
Member

Choose a reason for hiding this comment

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

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...

pub role: String,
}
155 changes: 146 additions & 9 deletions backend/src/api/model/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ use crate::{
prelude::*,
};

use self::{acl::AclInput, err::ApiError};

use super::playlist::VideoListEntry;


Expand Down Expand Up @@ -212,6 +214,11 @@ impl AuthorizedEvent {
&self.tobira_deletion_timestamp
}

/// Whether the event has active workflows.
async fn has_active_workflows(&self, context: &Context) -> ApiResult<bool> {
Self::has_active_workflows(&self, context).await
}

async fn series(&self, context: &Context) -> ApiResult<Option<Series>> {
if let Some(series) = self.series {
Ok(Series::load_by_key(series, context).await?)
Expand Down Expand Up @@ -337,21 +344,37 @@ impl AuthorizedEvent {
.pipe(Ok)
}

pub(crate) async fn delete(id: Id, context: &Context) -> ApiResult<RemovedEvent> {
async fn load_for_api(
id: Id,
context: &Context,
not_found_error: ApiError,
not_authorized_error: ApiError,
) -> ApiResult<AuthorizedEvent> {
let event = Self::load_by_id(id, context)
.await?
.ok_or_else(|| err::invalid_input!(
key = "event.delete.not-found",
"event not found",
))?
.await?
.ok_or_else(|| not_found_error)?
.into_result()?;

if !context.auth.overlaps_roles(&event.write_roles) {
return Err(err::not_authorized!(
return Err(not_authorized_error);
}

Ok(event)
}

pub(crate) async fn delete(id: Id, context: &Context) -> ApiResult<RemovedEvent> {
let event = Self::load_for_api(
id,
context,
err::invalid_input!(
key = "event.delete.not-found",
"event not found"
),
err::not_authorized!(
key = "event.delete.not-allowed",
"you are not allowed to delete this event",
));
}
)
).await?;

let response = context
.oc_client
Expand Down Expand Up @@ -381,6 +404,120 @@ impl AuthorizedEvent {
}
}

async fn has_active_workflows(&self, context: &Context) -> ApiResult<bool> {
if !context.auth.overlaps_roles(&self.write_roles) {
return Err(err::not_authorized!(
key = "event.workflow.not-allowed",
"you are not allowed to inquire about this event's workflow activity",
));
}

let response = context
.oc_client
.has_active_workflows(&self.opencast_id)
.await
.map_err(|e| {
error!("Failed to get workflow activity: {}", e);
err::opencast_unavailable!("Failed to communicate with Opencast")
})?;

Ok(response)
}

pub(crate) async fn update_acl(id: Id, acl: Vec<AclInput>, context: &Context) -> ApiResult<AuthorizedEvent> {
Copy link
Member

Choose a reason for hiding this comment

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

Without having looked at what the frontend is actually sending, there is a problem here:

  • If the frontend only send read and write ACLs, then the API request to Opencast (only containing those ACLs) will overwrite any other actions that might have been in the ACL. (And we don't mirror that in our own DB, but that's the smaller problem in that case.)
  • If the frontend sends the full ACL, i.e. also all preview and custom roles, then the request to OC is fine, but we only update the read and write roles in the DB, which seems wrong.

So for one, we have to decide what this API requires (just read&write or full ACL) and specify this in the API docs. As second step, this API handler needs to be adjusted to either add the other actions to the OC request, or to write all actions, not just read and write, to our DB.

let event = Self::load_for_api(
id,
context,
err::invalid_input!(
key = "event.acl.not-found",
"event not found",
),
err::not_authorized!(
key = "event.acl.not-allowed",
"you are not allowed to update this event's acl",
)
).await?;

if Self::has_active_workflows(&event, context).await? {
return Err(err::not_authorized!(
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 think not_authorized! is the right error kind. Unfortunately, it seems like we need to add yet another one... maybe OpencastError is fine, if you added that in response to the comment above.

key = "event.workflow.active",
"acl change blocked by another workflow",
));
}

let response = context
.oc_client
.update_event_acl(&event.opencast_id, &acl)
.await
.map_err(|e| {
error!("Failed to send acl update request: {}", e);
err::opencast_unavailable!("Failed to communicate with Opencast")
Copy link
Member

Choose a reason for hiding this comment

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

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

})?;

if response.status() == StatusCode::NO_CONTENT {
// 204: The access control list for the specified event is updated.
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");
Comment on lines +459 to +466
Copy link
Member

Choose a reason for hiding this comment

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

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.

info!(event_id = %id, "Requested acl update of event");

// Todo: also update preview roles
context.db.execute("\
update all_events \
set read_roles = $2, write_roles = $3 \
where id = $1 \
", &[&event.key, &read_roles, &write_roles]).await?;

Self::start_workflow(&event.opencast_id, "republish-metadata", &context).await?;
Comment on lines +475 to +476
Copy link
Member

Choose a reason for hiding this comment

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

That should probably be called before the DB update? My thinking is: if starting the workflow fails, the event ACL in Opencast is never republished and thus never actually "public" and keeping the old ACL in our DB is more correct as it would reflect the state of the harvest API. If the DB change fails after starting the workflow, thats not too big of a deal as it will be updated via sync a few minutes later anyway.

But sure, the workflow could still fail after being started, which is what we discussed a couple of times and decided to ignore that. So dealing with "starting workflow failed" is likely not too important.

Ok(event)
Copy link
Member

Choose a reason for hiding this comment

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

You return the event that was loaded before you did the DB modifications, right? So the API response will not reflect the changes to the roles yet, which is not expected. You should load the event fresh from DB here.

EDIT: ah mh, so the acl field does its own DB query, so that part of the API contains the up to date information. However, read_roles and write_roles are out of date, so I would still just load the event fresh from the DB here.

} else {
warn!(
event_id = %id,
"Failed to update event acl, OC returned status: {}",
response.status(),
);
Err(err::opencast_unavailable!("Opencast API error: {}", response.status()))
}
}

/// Starts a workflow on the event.
async fn start_workflow(oc_id: &String, workflow_id: &str, context: &Context) -> ApiResult<StatusCode> {
let response = context
.oc_client
.start_workflow(&oc_id, "republish-metadata")
Copy link
Member

Choose a reason for hiding this comment

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

Woops, you surely meant to pass workflow_id here

.await
.map_err(|e| {
error!("Failed sending request to start workflow: {}", e);
err::opencast_unavailable!("Failed to communicate with Opencast")
})?;

if response.status() == StatusCode::CREATED {
// 201: A new workflow is created.
info!(workflow = %workflow_id, "Requested creation of workflow");
Copy link
Member

Choose a reason for hiding this comment

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

The info! logs in this function should also include the OC event ID

Ok(response.status())
} else if response.status() == StatusCode::NOT_FOUND {
// 404: The specified workflow instance does not exist.
warn!(
workflow = %workflow_id,
"{}: The specified workflow instance does not exist.",
response.status(),
Comment on lines +507 to +508
Copy link
Member

Choose a reason for hiding this comment

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

In this branch response.status() is always 404, so not sure if we really need to interpolate it into the log message

);
Err(err::opencast_unavailable!("Opencast API error: {}", response.status()))
} else {
warn!(
workflow = %workflow_id,
"Failed to create workflow, OC returned status: {}",
response.status(),
);
Err(err::opencast_unavailable!("Opencast API error: {}", response.status()))
Copy link
Member

Choose a reason for hiding this comment

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

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"?

}
}

pub(crate) async fn load_writable_for_user(
context: &Context,
order: EventSortOrder,
Expand Down
2 changes: 1 addition & 1 deletion backend/src/api/model/realm/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ impl Realm {
self.owner_display_name.as_deref()
}

/// Returns the acl of this realm, combining moderator and admin roles and assigns
/// Returns the acl of this realm, combining moderator and admin roles and assigns
/// the respective actions that are necessary for UI purposes.
async fn own_acl(&self, context: &Context) -> ApiResult<Acl> {
let raw_roles_sql = "
Expand Down
2 changes: 1 addition & 1 deletion backend/src/api/model/series.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ impl Series {

return Ok(RemoveMountedSeriesOutcome::RemovedRealm(removed_realm));
}

if old_realm.name_from_block.map(Id::block) == Some(blocks[0].id()) {
// The realm has its name derived from the series block that is being removed - so the name
// shouldn't be used anymore. Ideally this would restore the previous title,
Expand Down
12 changes: 11 additions & 1 deletion backend/src/api/mutation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use super::{
err::ApiResult,
id::Id,
model::{
acl::AclInput,
series::{Series, NewSeries},
realm::{
ChildIndex,
Expand Down Expand Up @@ -58,7 +59,7 @@ impl Mutation {
/// Deletes the given event. Meaning: a deletion request is sent to Opencast, the event
/// is marked as "deletion pending" in Tobira, and fully removed once Opencast
/// finished deleting the event.
///
///
/// Returns the deletion timestamp in case of success and errors otherwise.
/// Note that "success" in this case only means the request was successfully sent
/// and accepted, not that the deletion itself succeeded, which is instead checked
Expand All @@ -67,6 +68,15 @@ impl Mutation {
AuthorizedEvent::delete(id, context).await
}

/// 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.
Comment on lines +71 to +75
Copy link
Member

Choose a reason for hiding this comment

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

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..."

async fn update_event_acl(id: Id, acl: Vec<AclInput>, context: &Context) -> ApiResult<AuthorizedEvent> {
AuthorizedEvent::update_acl(id, acl, context).await
}

/// Sets the order of all children of a specific realm.
///
/// `childIndices` must contain at least one element, i.e. do not call this
Expand Down
22 changes: 17 additions & 5 deletions backend/src/config/general.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ pub(crate) struct GeneralConfig {

/// Public URL to Tobira (without path).
/// Used for RSS feeds, as those require specifying absolute URLs to resources.
///
///
/// Example: "https://tobira.my-uni.edu".
pub(crate) tobira_url: HttpHost,

Expand All @@ -22,12 +22,12 @@ pub(crate) struct GeneralConfig {
/// These can be specified in multiple languages.
/// Consent is prompted upon first use and only if this is configured. It is
/// re-prompted when any of these values change.
///
///
/// We recommend not to configure this unless absolutely necessary,
/// in order to not degrade the user experience needlessly.
///
///
/// Example:
///
///
/// ```
/// initial_consent.title.en = "Terms & Conditions"
/// initial_consent.button.en = "Agree"
Expand All @@ -49,7 +49,7 @@ pub(crate) struct GeneralConfig {
/// add custom ones. Note that these two default links are special and can
/// be specified with only the shown string. To add custom ones, you need
/// to define a label and a link. The link is either the same for every language
/// or can be specified for each language in the same manner as the label.
/// or can be specified for each language in the same manner as the label.
/// Example:
///
/// ```
Expand Down Expand Up @@ -110,6 +110,18 @@ pub(crate) struct GeneralConfig {
/// (partial) name.
#[config(default = false)]
pub users_searchable: bool,

/// This allows users to edit the ACL of events they have write access for.
/// Doing so will update these in Opencast and start the `republish-metadata`
/// workflow to propagate the changes to other publications as well.
/// Instead of waiting for the workflow however, Tobira will also immediately
/// store the updated ACL in its database.
///
/// Note that this might lead to situations where the event ACL in Tobira is different
/// from that in other publications, mainly if the afore mentioned workflow fails
/// or takes an unusually long time to complete.
#[config(default = true)]
pub allow_acl_edit: bool,
}

const INTERNAL_RESERVED_PATHS: &[&str] = &["favicon.ico", "robots.txt", ".well-known"];
Expand Down
1 change: 1 addition & 0 deletions backend/src/http/assets.rs
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,7 @@ fn frontend_config(config: &Config) -> serde_json::Value {
"initialConsent": config.general.initial_consent,
"showDownloadButton": config.general.show_download_button,
"usersSearchable": config.general.users_searchable,
"allowAclEdit": config.general.allow_acl_edit,
"footerLinks": config.general.footer_links,
"metadataLabels": config.general.metadata,
"paellaPluginConfig": config.player.paella_plugin_config,
Expand Down
45 changes: 44 additions & 1 deletion backend/src/sync/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use serde::Deserialize;
use tap::TapFallible;

use crate::{
api::model::acl::AclInput,
config::{Config, HttpHost},
prelude::*,
sync::harvest::HarvestResponse,
Expand Down Expand Up @@ -140,7 +141,7 @@ impl OcClient {
}

pub async fn delete_event(&self, oc_id: &String) -> Result<Response<Incoming>> {
let pq = format!("/api/events/{}", oc_id);
let pq = format!("/api/events/{oc_id}");
let req = self.authed_req_builder(&self.external_api_node, &pq)
.method(http::Method::DELETE)
.body(RequestBody::empty())
Expand All @@ -149,6 +150,48 @@ impl OcClient {
self.http_client.request(req).await.map_err(Into::into)
}

pub async fn update_event_acl(&self, oc_id: &String, acl: &Vec<AclInput>) -> Result<Response<Incoming>> {
Copy link
Member

Choose a reason for hiding this comment

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

Basically always use &[T] instead of &Vec<T> in function arguments. And also &str instead of &String.

let pq = format!("/api/events/{oc_id}/acl");
let req = self.authed_req_builder(&self.external_api_node, &pq)
.method(http::Method::PUT)
.header(http::header::CONTENT_TYPE, "application/x-www-form-urlencoded")
.body(
format!("acl={}", serde_json::to_string(&acl).expect("Failed to serialize")).into()
)
Comment on lines +157 to +160
Copy link
Member

Choose a reason for hiding this comment

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

This seems like it is missing some escaping. application/x-www-form-urlencoded is just a foo=bar&baz=qux string right? But what if the ACL contains roles/actions with & or =? In JS you would use encodeURIComponent. Sure, roles and actions usually shouldn't contain these characters, but this shouldn't just fail weirdly in that case..

We already have form_urlencoded as dependency, which is likely the right tool for the job.

.expect("failed to build request");

self.http_client.request(req).await.map_err(Into::into)
Copy link
Member

Choose a reason for hiding this comment

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

I think this could use an info! log. Or put that inside the API handler. Either way, I think it would be nice knowing when mutating requests are sent to OC

}

pub async fn start_workflow(&self, oc_id: &String, workflow_id: &str) -> Result<Response<Incoming>> {
Copy link
Member

Choose a reason for hiding this comment

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

&String -> &str

let params = format!("\
event_identifier={oc_id}\
&workflow_definition_identifier={workflow_id}\
");
Comment on lines +167 to +170
Copy link
Member

Choose a reason for hiding this comment

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

Here again, the two variables might need escaping/proper encoding.

let req = self.authed_req_builder(&self.external_api_node, "/api/workflows")
.method(http::Method::POST)
.header(http::header::CONTENT_TYPE, "application/x-www-form-urlencoded")
.body(params.into())
.expect("failed to build request");

self.http_client.request(req).await.map_err(Into::into)
}

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)
Comment on lines +181 to +182
Copy link
Member

Choose a reason for hiding this comment

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

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

.header(http::header::CONTENT_TYPE, "application/x-www-form-urlencoded")
Copy link
Member

Choose a reason for hiding this comment

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

Is this header necessary?

.body(RequestBody::empty())
.expect("failed to build request");
let uri = req.uri().clone();
let response = self.http_client.request(req)
.await
.with_context(|| format!("HTTP request failed (uri: '{uri}')"))?;

let (out, _) = self.deserialize_response(response, &uri).await?;
Ok(out)
}

fn build_authed_req(&self, node: &HttpHost, path_and_query: &str) -> (Uri, Request<RequestBody>) {
let req = self.authed_req_builder(node, path_and_query)
.body(RequestBody::empty())
Expand Down
Loading
Loading