From 176fa0b3962e7708ee941b2e9378185a62a1fce5 Mon Sep 17 00:00:00 2001 From: Lukas Kalbertodt Date: Tue, 12 Nov 2024 11:33:39 +0100 Subject: [PATCH 01/10] Stop merging `read_roles` into `preview_roles` See https://github.com/elan-ev/tobira/pull/1244#discussion_r1783044285 I thought quite a bit about this and I think both options would have been fine, but I decided to change this for one reason in particular: currently, `read_roles` and `write_roles` reflect exactly what Opencast tells us. We of course also have columns with calculated values, but we should keep it fairly clear what columns are straight from OC and which ones are "our columns". Most importantly: if we ever would need to distinguish between read and preview, this would require a resync. This would be unfortunate, so I'd rather store the exact OC information so that we have it, just in case. I then decided to just adjust the few checks to check both columns, but we could also have added another column that holds the combination of the two. That's an implementation detail and we can still change it later. But yes, the more complicated checks don't seem like a problem to me. --- backend/src/api/model/event.rs | 12 ++++--- backend/src/api/model/search/mod.rs | 34 ++++++++++++------- .../39-event-preview-roles-and-password.sql | 23 ++----------- frontend/src/schema.graphql | 5 +-- 4 files changed, 32 insertions(+), 42 deletions(-) diff --git a/backend/src/api/model/event.rs b/backend/src/api/model/event.rs index 1bae88e02..0c6a4a9cc 100644 --- a/backend/src/api/model/event.rs +++ b/backend/src/api/model/event.rs @@ -219,8 +219,7 @@ impl AuthorizedEvent { fn write_roles(&self) -> &[String] { &self.write_roles } - /// This includes all read roles (and by extension write roles, - /// as they are a subset of read roles). + /// This doesn't contain `ROLE_ADMIN` as that is included implicitly. fn preview_roles(&self) -> &[String] { &self.preview_roles } @@ -365,7 +364,7 @@ impl AuthorizedEvent { .await? .map(|row| { let event = Self::from_row_start(&row); - if context.auth.overlaps_roles(&event.preview_roles) { + if event.can_be_previewed(context) { Event::Event(event) } else { Event::NotAllowed(NotAllowed) @@ -386,7 +385,7 @@ impl AuthorizedEvent { context.db .query_mapped(&query, dbargs![&series_key], |row| { let event = Self::from_row_start(&row); - if !context.auth.overlaps_roles(&event.preview_roles) { + if !event.can_be_previewed(context) { return VideoListEntry::NotAllowed(NotAllowed); } @@ -396,6 +395,11 @@ impl AuthorizedEvent { .pipe(Ok) } + fn can_be_previewed(&self, context: &Context) -> bool { + context.auth.overlaps_roles(&self.preview_roles) + || context.auth.overlaps_roles(&self.read_roles) + } + pub(crate) async fn delete(id: Id, context: &Context) -> ApiResult { let event = Self::load_by_id(id, context) .await? diff --git a/backend/src/api/model/search/mod.rs b/backend/src/api/model/search/mod.rs index ace73503a..a7fdf41f2 100644 --- a/backend/src/api/model/search/mod.rs +++ b/backend/src/api/model/search/mod.rs @@ -166,7 +166,7 @@ pub(crate) async fn perform( let selection = search::Event::select(); let query = format!("select {selection} from search_events \ where id = (select id from events where opencast_id = $1) \ - and (preview_roles || 'ROLE_ADMIN'::text) && $2"); + and (preview_roles || read_roles || 'ROLE_ADMIN'::text) && $2"); let items: Vec = context.db .query_opt(&query, &[&uuid_query, &context.auth.roles_vec()]) .await? @@ -188,7 +188,10 @@ pub(crate) async fn perform( // Prepare the event search let filter = Filter::And( std::iter::once(Filter::Leaf("listed = true".into())) - .chain(acl_filter("preview_roles", context)) + .chain([Filter::Or([ + acl_filter("preview_roles", context), + acl_filter("read_roles", context), + ].into_iter().flatten().collect())]) // Filter out live events that are already over. .chain([Filter::Or([ Filter::Leaf("is_live = false ".into()), @@ -332,7 +335,13 @@ pub(crate) async fn all_events( if writable_only { writable } else { - Filter::or([Filter::listed_and_readable("preview_roles", context), writable]) + Filter::or([ + Filter::or([ + Filter::acl_access("read_roles", context), + Filter::acl_access("preview_roles", context), + ]).and_listed(context), + writable, + ]) } }).to_string(); @@ -429,7 +438,10 @@ pub(crate) async fn all_playlists( if writable_only { writable } else { - Filter::or([Filter::listed_and_readable("read_roles", context), writable]) + Filter::or([ + Filter::acl_access("read_roles", context).and_listed(context), + writable, + ]) } }).to_string(); @@ -489,17 +501,13 @@ impl Filter { Self::Leaf("listed = true".into()) } - /// Returns a filter checking that the current user has read access and that - /// the item is listed. If the user has the privilege to find unlisted - /// item, the second check is not performed. - /// "Readable" in this context can mean either "preview-able" in case of events - /// or actual "readable" in case of playlists, as they do not have preview roles. - fn listed_and_readable(roles_field: &str, context: &Context) -> Self { - let readable = Self::acl_access(roles_field, context); + /// If the user can find unlisted items, just returns `self`. Otherweise, + /// `self` is ANDed with `Self::listed()`. + fn and_listed(self, context: &Context) -> Self { if context.auth.can_find_unlisted_items(&context.config.auth) { - readable + self } else { - Self::and([readable, Self::listed()]) + Self::and([self, Self::listed()]) } } diff --git a/backend/src/db/migrations/39-event-preview-roles-and-password.sql b/backend/src/db/migrations/39-event-preview-roles-and-password.sql index e19141cbc..0e3b3754d 100644 --- a/backend/src/db/migrations/39-event-preview-roles-and-password.sql +++ b/backend/src/db/migrations/39-event-preview-roles-and-password.sql @@ -14,30 +14,11 @@ alter table all_events add column credentials credentials, add column preview_roles text[] not null default '{}'; --- For convenience, all read roles are also copied over to preview roles. --- Removing any roles from read will however _not_ remove them from preview, as they --- might also have been added separately and we can't really account for that. -update all_events set preview_roles = read_roles; -create function sync_preview_roles() -returns trigger language plpgsql as $$ -begin - new.preview_roles := ( - select array_agg(distinct role) from unnest(new.preview_roles || new.read_roles) as role - ); - return new; -end; -$$; - -create trigger sync_preview_roles_on_change -before insert or update of read_roles, preview_roles on all_events -for each row -execute function sync_preview_roles(); - --- replace outdated view to include preview_roles +-- replace outdated view to include new columnes create or replace view events as select * from all_events where tobira_deletion_timestamp is null; --- add `preview_roles` to `search_events` view as well +-- add `preview_roles` and `has_password` to `search_events` view drop view search_events; create view search_events as select diff --git a/frontend/src/schema.graphql b/frontend/src/schema.graphql index 9868e8177..c5527ca67 100644 --- a/frontend/src/schema.graphql +++ b/frontend/src/schema.graphql @@ -315,10 +315,7 @@ type AuthorizedEvent implements Node { readRoles: [String!]! "This doesn't contain `ROLE_ADMIN` as that is included implicitly." writeRoles: [String!]! - """ - This includes all read roles (and by extension write roles, - as they are a subset of read roles). - """ + "This doesn't contain `ROLE_ADMIN` as that is included implicitly." previewRoles: [String!]! syncedData: SyncedEventData "Returns the authorized event data if the user has read access or is authenticated for the event." From 34b61d7be9b300bb9c07bad895dfc593ea949892 Mon Sep 17 00:00:00 2001 From: Lukas Kalbertodt Date: Wed, 13 Nov 2024 13:58:51 +0100 Subject: [PATCH 02/10] Rework `Filter` helper in search The `perform` method still used lots of manual filter creation, which is now replaced. I also replaced `Filter::None` with `Filter::True`, which more precisely describes its role. Before, one could easily run into a bug where `None`s from `or` rules are just filtered out, but in all cases of `None` usage, it should rather make the whole `or` evaluate to `true`. Finally, three more helper functions were added to avoid repeating `*_roles` a bunch of times. --- backend/src/api/model/search/mod.rs | 120 ++++++++++++++++------------ 1 file changed, 69 insertions(+), 51 deletions(-) diff --git a/backend/src/api/model/search/mod.rs b/backend/src/api/model/search/mod.rs index a7fdf41f2..f92c58ffb 100644 --- a/backend/src/api/model/search/mod.rs +++ b/backend/src/api/model/search/mod.rs @@ -186,21 +186,22 @@ pub(crate) async fn perform( // Prepare the event search - let filter = Filter::And( - std::iter::once(Filter::Leaf("listed = true".into())) - .chain([Filter::Or([ - acl_filter("preview_roles", context), - acl_filter("read_roles", context), - ].into_iter().flatten().collect())]) - // Filter out live events that are already over. - .chain([Filter::Or([ - Filter::Leaf("is_live = false ".into()), - Filter::Leaf(format!("end_time_timestamp >= {}", Utc::now().timestamp()).into()), - ].into())]) - .chain(filters.start.map(|start| Filter::Leaf(format!("created_timestamp >= {}", start.timestamp()).into()))) - .chain(filters.end.map(|end| Filter::Leaf(format!("created_timestamp <= {}", end.timestamp()).into()))) - .collect() - ).to_string(); + let filter = Filter::and([ + Filter::listed(), + Filter::preview_or_read_access(context), + // Filter out live events that already ended + Filter::or([ + Filter::Leaf("is_live = false ".into()), + Filter::Leaf(format!("end_time_timestamp >= {}", Utc::now().timestamp()).into()), + ]), + // Apply user selected date filters + filters.start + .map(|start| Filter::Leaf(format!("created_timestamp >= {}", start.timestamp()).into())) + .unwrap_or(Filter::True), + filters.end + .map(|end| Filter::Leaf(format!("created_timestamp <= {}", end.timestamp()).into())) + .unwrap_or(Filter::True), + ]).to_string(); let event_query = context.search.event_index.search() .with_query(user_query) .with_limit(15) @@ -327,19 +328,16 @@ pub(crate) async fn all_events( return Err(context.not_logged_in_error()); } - let filter = Filter::make_or_none_for_admins(context, || { + let filter = Filter::make_or_true_for_admins(context, || { // All users can always find all events they have write access to. If // `writable_only` is false, this API also returns events that are // listed and that the user can read. - let writable = Filter::acl_access("write_roles", context); + let writable = Filter::write_access(context); if writable_only { writable } else { Filter::or([ - Filter::or([ - Filter::acl_access("read_roles", context), - Filter::acl_access("preview_roles", context), - ]).and_listed(context), + Filter::preview_or_read_access(context).and_listed(context), writable, ]) } @@ -379,8 +377,8 @@ pub(crate) async fn all_series( return Err(context.not_logged_in_error()); } - let filter = Filter::make_or_none_for_admins(context, || { - let writable = Filter::acl_access("write_roles", context); + let filter = Filter::make_or_true_for_admins(context, || { + let writable = Filter::write_access(context); // All users can always find all items they have write access to, // regardless whether they are listed or not. @@ -391,7 +389,7 @@ pub(crate) async fn all_series( // Since series read_roles are not used for access control, we only need // to check whether we can return unlisted videos. if context.auth.can_find_unlisted_items(&context.config.auth) { - Filter::None + Filter::True } else { Filter::or([writable, Filter::listed()]) } @@ -430,16 +428,16 @@ pub(crate) async fn all_playlists( return Err(context.not_logged_in_error()); } - let filter = Filter::make_or_none_for_admins(context, || { + let filter = Filter::make_or_true_for_admins(context, || { // All users can always find all playlists they have write access to. If // `writable_only` is false, this API also returns playlists that are // listed and that the user can read. - let writable = Filter::acl_access("write_roles", context); + let writable = Filter::write_access(context); if writable_only { writable } else { Filter::or([ - Filter::acl_access("read_roles", context).and_listed(context), + Filter::read_access(context).and_listed(context), writable, ]) } @@ -461,39 +459,46 @@ pub(crate) async fn all_playlists( Ok(PlaylistSearchOutcome::Results(SearchResults { items, total_hits, duration: elapsed_time() })) } -// TODO: replace usages of this and remove this. -fn acl_filter(action: &str, context: &Context) -> Option { - // If the user is admin, we just skip the filter alltogether as the admin - // can see anything anyway. - if context.auth.is_admin() { - return None; - } - - Some(Filter::acl_access(action, context)) -} enum Filter { // TODO: try to avoid Vec if not necessary. Oftentimes there are only two operands. + + /// Must not contain `Filter::None`, which is handled by `Filter::and`. And(Vec), + + /// Must not contain `Filter::None`, which is handled by `Filter::or`. Or(Vec), Leaf(Cow<'static, str>), - /// No filter. Formats to empty string and is filtered out if inside the - /// `And` or `Or` operands. - None, + /// A constant `true`. Inside `Or`, results in the whole `Or` expression + /// being replaced by `True`. Inside `And`, this is just filtered out and + /// the remaining operands are evaluated. If formated on its own, empty + /// string is emitted. + True, } impl Filter { - fn make_or_none_for_admins(context: &Context, f: impl FnOnce() -> Self) -> Self { - if context.auth.is_admin() { Self::None } else { f() } + fn make_or_true_for_admins(context: &Context, f: impl FnOnce() -> Self) -> Self { + if context.auth.is_admin() { Self::True } else { f() } } fn or(operands: impl IntoIterator) -> Self { - Self::Or(operands.into_iter().collect()) + let mut v = Vec::new(); + for op in operands { + if matches!(op, Self::True) { + return Self::True; + } + v.push(op); + } + Self::Or(v) } fn and(operands: impl IntoIterator) -> Self { - Self::And(operands.into_iter().collect()) + Self::And( + operands.into_iter() + .filter(|op| !matches!(op, Self::True)) + .collect(), + ) } /// Returns the filter "listed = true". @@ -511,10 +516,25 @@ impl Filter { } } + fn read_access(context: &Context) -> Self { + Self::make_or_true_for_admins(context, || Self::acl_access_raw("read_roles", context)) + } + + fn write_access(context: &Context) -> Self { + Self::make_or_true_for_admins(context, || Self::acl_access_raw("write_roles", context)) + } + + fn preview_or_read_access(context: &Context) -> Self { + Self::make_or_true_for_admins(context, || Self::or([ + Self::acl_access_raw("read_roles", context), + Self::acl_access_raw("preview_roles", context), + ])) + } + /// Returns a filter checking if `roles_field` has any overlap with the /// current user roles. Encodes all roles as hex to work around Meili's - /// lack of case-sensitive comparison. - fn acl_access(roles_field: &str, context: &Context) -> Self { + /// lack of case-sensitive comparison. Does not handle the ROLE_ADMIN case. + fn acl_access_raw(roles_field: &str, context: &Context) -> Self { use std::io::Write; const HEX_DIGITS: &[u8; 16] = b"0123456789abcdef"; @@ -541,10 +561,8 @@ impl Filter { impl fmt::Display for Filter { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fn join(f: &mut fmt::Formatter, operands: &[Filter], sep: &str) -> fmt::Result { - if operands.iter().all(|op| matches!(op, Filter::None)) { - return Ok(()); - } - + // We are guaranteed by `and` and `or` methods that there are no + // `Self::True`s in here. write!(f, "(")?; for (i, operand) in operands.iter().enumerate() { if i > 0 { @@ -559,7 +577,7 @@ impl fmt::Display for Filter { Self::And(operands) => join(f, operands, "AND"), Self::Or(operands) => join(f, operands, "OR"), Self::Leaf(s) => write!(f, "{s}"), - Self::None => Ok(()), + Self::True => Ok(()), } } } From a0d82b3d766a03e25330592c20054f97dc172ee4 Mon Sep 17 00:00:00 2001 From: Lukas Kalbertodt Date: Thu, 14 Nov 2024 19:01:32 +0100 Subject: [PATCH 03/10] Remove DB trigger to transfer series credentials to event From a recent meeting, we concluded that we should just do what the event ACL tell us. The admins need to make sure the effective (i.e. after applying merge mode) event ACLs contain the password roles, if that video should be password protected. Even if that requirement still changes in the future, I think it should be done in the harvest code, not as a DB trigger. Further, just always transferring series credentials to events is also wrong in our more flexible model (that should cover more use cases than the ETH). Finally, the trigger as written was not complete as it was missing the "read roles become preview roles" logic, which is already present in code. I decided to still store series credentials, as that's useful for a few reasons, e.g. implementing this feature later on without resyncing. But this tiny SQL command didn't warrant its own migration, so I combined both. --- backend/src/db/migrations.rs | 3 +- ...l => 39-preview-roles-and-credentials.sql} | 3 ++ .../migrations/40-eth-series-credentials.sql | 37 ------------------- 3 files changed, 4 insertions(+), 39 deletions(-) rename backend/src/db/migrations/{39-event-preview-roles-and-password.sql => 39-preview-roles-and-credentials.sql} (97%) delete mode 100644 backend/src/db/migrations/40-eth-series-credentials.sql diff --git a/backend/src/db/migrations.rs b/backend/src/db/migrations.rs index 4f6328413..74b73c47d 100644 --- a/backend/src/db/migrations.rs +++ b/backend/src/db/migrations.rs @@ -371,6 +371,5 @@ static MIGRATIONS: Lazy> = include_migrations![ 36: "playlist-blocks", 37: "redo-search-triggers-and-listed", 38: "event-texts", - 39: "event-preview-roles-and-password", - 40: "eth-series-credentials", + 39: "preview-roles-and-credentials", ]; diff --git a/backend/src/db/migrations/39-event-preview-roles-and-password.sql b/backend/src/db/migrations/39-preview-roles-and-credentials.sql similarity index 97% rename from backend/src/db/migrations/39-event-preview-roles-and-password.sql rename to backend/src/db/migrations/39-preview-roles-and-credentials.sql index 0e3b3754d..565f53b39 100644 --- a/backend/src/db/migrations/39-event-preview-roles-and-password.sql +++ b/backend/src/db/migrations/39-preview-roles-and-credentials.sql @@ -14,6 +14,9 @@ alter table all_events add column credentials credentials, add column preview_roles text[] not null default '{}'; +alter table series + add column credentials credentials; + -- replace outdated view to include new columnes create or replace view events as select * from all_events where tobira_deletion_timestamp is null; diff --git a/backend/src/db/migrations/40-eth-series-credentials.sql b/backend/src/db/migrations/40-eth-series-credentials.sql deleted file mode 100644 index 9719ccc29..000000000 --- a/backend/src/db/migrations/40-eth-series-credentials.sql +++ /dev/null @@ -1,37 +0,0 @@ --- Adds a credentials column to series that hold series specific passwords and usernames. --- These need to be synced with events that are part of the series. --- This is specific for authentication requirements of the ETH and is only useful when the --- `interpret_eth_passwords` configuration is enabled. - -alter table series add column credentials credentials; - --- When the credentials of a series change, each event that is part of it also needs to be updated. -create function sync_series_credentials() returns trigger language plpgsql as $$ -begin - update all_events set credentials = series.credentials - from series where all_events.series = series.id and series.id = new.id; - return new; -end; -$$; - -create trigger sync_series_credentials_on_change -after update on series -for each row -when (old.credentials is distinct from new.credentials) -execute function sync_series_credentials(); - --- Tobira uploads do not automatically get the credentials of their assigned series, so this needs --- to be done with an additional function and a trigger. -create function sync_credentials_before_event_insert() returns trigger language plpgsql as $$ -begin - select series.credentials into new.credentials - from series - where series.id = new.series; - return new; -end; -$$; - -create trigger sync_series_credentials_before_event_insert -before insert on all_events -for each row -execute function sync_credentials_before_event_insert(); From 8e7d50e5ee79eda0ffcb67c7f22d67ac0bc5077f Mon Sep 17 00:00:00 2001 From: Lukas Kalbertodt Date: Thu, 14 Nov 2024 11:57:57 +0100 Subject: [PATCH 04/10] Stop sending search text matches for videos with only preview access --- backend/src/api/model/search/event.rs | 46 ++++++++++++--------------- frontend/src/routes/Search.tsx | 10 +----- 2 files changed, 22 insertions(+), 34 deletions(-) diff --git a/backend/src/api/model/search/event.rs b/backend/src/api/model/search/event.rs index c3a4211b0..c92214000 100644 --- a/backend/src/api/model/search/event.rs +++ b/backend/src/api/model/search/event.rs @@ -69,7 +69,9 @@ impl Node for SearchEvent { impl SearchEvent { pub(crate) fn without_matches(src: search::Event, context: &Context) -> Self { - Self::new_inner(src, vec![], SearchEventMatches::default(), context) + let read_roles = decode_acl(&src.read_roles); + let user_can_read = context.auth.overlaps_roles(read_roles); + Self::new_inner(src, vec![], SearchEventMatches::default(), user_can_read) } pub(crate) fn new(hit: meilisearch_sdk::SearchResult, context: &Context) -> Self { @@ -77,16 +79,20 @@ impl SearchEvent { let src = hit.result; let mut text_matches = Vec::new(); - src.slide_texts.resolve_matches( - match_ranges_for(match_positions, "slide_texts.texts"), - &mut text_matches, - TextAssetType::SlideText, - ); - src.caption_texts.resolve_matches( - match_ranges_for(match_positions, "caption_texts.texts"), - &mut text_matches, - TextAssetType::Caption, - ); + let read_roles = decode_acl(&src.read_roles); + let user_can_read = context.auth.overlaps_roles(read_roles); + if user_can_read { + src.slide_texts.resolve_matches( + match_ranges_for(match_positions, "slide_texts.texts"), + &mut text_matches, + TextAssetType::SlideText, + ); + src.caption_texts.resolve_matches( + match_ranges_for(match_positions, "caption_texts.texts"), + &mut text_matches, + TextAssetType::Caption, + ); + } let matches = SearchEventMatches { title: field_matches_for(match_positions, "title"), @@ -94,25 +100,15 @@ impl SearchEvent { series_title: field_matches_for(match_positions, "series_title"), }; - Self::new_inner(src, text_matches, matches, context) + Self::new_inner(src, text_matches, matches, user_can_read) } fn new_inner( src: search::Event, text_matches: Vec, matches: SearchEventMatches, - context: &Context, + user_can_read: bool, ) -> Self { - let read_roles = decode_acl(&src.read_roles); - let user_is_authorized = context.auth.overlaps_roles(read_roles); - let thumbnail = { - if user_is_authorized { - src.thumbnail - } else { - None - } - }; - Self { id: Id::search_event(src.id.0), series_id: src.series_id.map(|id| Id::search_series(id.0)), @@ -120,7 +116,7 @@ impl SearchEvent { title: src.title, description: src.description, creators: src.creators, - thumbnail, + thumbnail: if user_can_read { src.thumbnail } else { None }, duration: src.duration as f64, created: src.created, start_time: src.start_time, @@ -133,7 +129,7 @@ impl SearchEvent { text_matches, matches, has_password: src.has_password, - user_is_authorized, + user_is_authorized: user_can_read, } } } diff --git a/frontend/src/routes/Search.tsx b/frontend/src/routes/Search.tsx index 70bd086ee..d3efeed09 100644 --- a/frontend/src/routes/Search.tsx +++ b/frontend/src/routes/Search.tsx @@ -51,7 +51,6 @@ import { COLORS } from "../color"; import { BREAKPOINT_MEDIUM } from "../GlobalStyle"; import { eventId, - getCredentials, isExperimentalFlagSet, keyOfId, secondsToTimeString, @@ -162,7 +161,6 @@ const query = graphql` startTime endTime created - hasPassword userIsAuthorized hostRealms { path ancestorNames } textMatches { @@ -523,7 +521,6 @@ const SearchEvent: React.FC = ({ hostRealms, textMatches, matches, - hasPassword, userIsAuthorized, }) => { // TODO: decide what to do in the case of more than two host realms. Direct @@ -532,11 +529,6 @@ const SearchEvent: React.FC = ({ ? DirectVideoRoute.url({ videoId: id }) : VideoRoute.url({ realmPath: hostRealms[0].path, videoID: id }); - // TODO: This check should be done in backend. - const showMatches = userIsAuthorized || ( - hasPassword && getCredentials("event", eventId(keyOfId(id))) - ); - return ( {{ image: @@ -635,7 +627,7 @@ const SearchEvent: React.FC = ({ {...{ seriesId }} />} {/* Show timeline with matches if there are any */} - {textMatches.length > 0 && showMatches && ( + {textMatches.length > 0 && ( )} , From 64ecc2cf9a646d181e82914428718356c48c736f Mon Sep 17 00:00:00 2001 From: Lukas Kalbertodt Date: Thu, 14 Nov 2024 19:11:50 +0100 Subject: [PATCH 05/10] Remove superfluous custom operation type --- frontend/src/util/index.ts | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/frontend/src/util/index.ts b/frontend/src/util/index.ts index 633161a87..e276fc702 100644 --- a/frontend/src/util/index.ts +++ b/frontend/src/util/index.ts @@ -2,14 +2,13 @@ import { i18n } from "i18next"; import { MutableRefObject, useEffect, useRef, useState } from "react"; import { useTranslation } from "react-i18next"; import { bug, match } from "@opencast/appkit"; -import { OperationType } from "relay-runtime"; import { useLazyLoadQuery } from "react-relay"; import CONFIG, { TranslatedString } from "../config"; import { TimeUnit } from "../ui/Input"; import { AuthorizedData, authorizedDataQuery, CREDENTIALS_STORAGE_KEY } from "../routes/Video"; import { - VideoAuthorizedDataQuery$data, + VideoAuthorizedDataQuery, } from "../routes/__generated__/VideoAuthorizedDataQuery.graphql"; @@ -227,9 +226,6 @@ export type Credentials = { password: string; } | null; -interface AuthenticatedData extends OperationType { - response: VideoAuthorizedDataQuery$data; -} /** * Returns `authorizedData` of password protected events by fetching it from the API, @@ -247,7 +243,7 @@ export const useAuthenticatedDataQuery = ( const credentials = getCredentials("event", eventId(keyOfId(eventID))) ?? ( seriesID && getCredentials("series", seriesId(keyOfId(seriesID))) ); - const authenticatedData = useLazyLoadQuery( + const authenticatedData = useLazyLoadQuery( authorizedDataQuery, { eventId: eventId(keyOfId(eventID)), ...credentials }, // This will only query the data for events with stored credentials and/or yet unknown From 571c25b09ef8b3f6f1ace4629542096f764840d2 Mon Sep 17 00:00:00 2001 From: Lukas Kalbertodt Date: Thu, 14 Nov 2024 19:09:04 +0100 Subject: [PATCH 06/10] Remove password query requests from `` Instead, the locked icon is shown for all password-protected videos, regardless of whether they are unlocked. --- frontend/src/ui/Video.tsx | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/frontend/src/ui/Video.tsx b/frontend/src/ui/Video.tsx index 467ee6970..0a80fe5ac 100644 --- a/frontend/src/ui/Video.tsx +++ b/frontend/src/ui/Video.tsx @@ -12,7 +12,6 @@ import { import { useColorScheme } from "@opencast/appkit"; import { COLORS } from "../color"; -import { useAuthenticatedDataQuery } from "../util"; type ThumbnailProps = JSX.IntrinsicElements["div"] & { @@ -56,9 +55,6 @@ export const Thumbnail: React.FC = ({ }) => { const { t } = useTranslation(); const isDark = useColorScheme().scheme === "dark"; - const authenticatedData = useAuthenticatedDataQuery(event.id, event.series?.id); - const authorizedThumbnail = event.authorizedData?.thumbnail - ?? authenticatedData?.thumbnail; const isUpcoming = isUpcomingLiveEvent(event.syncedData?.startTime ?? null, event.isLive); const audioOnly = event.authorizedData ? ( @@ -69,10 +65,10 @@ export const Thumbnail: React.FC = ({ : false; let inner; - if (authorizedThumbnail && !deletionIsPending) { + if (event.authorizedData?.thumbnail && !deletionIsPending) { // We have a proper thumbnail. inner = ; } else { From 40594309b3bf71ac9e4dbd9afaacd6d9ca1cdfe9 Mon Sep 17 00:00:00 2001 From: Lukas Kalbertodt Date: Mon, 18 Nov 2024 16:06:56 +0100 Subject: [PATCH 07/10] Get rid of `AuthenticatedDataContext` by using relay store So, quick recap of what was done before this commit: - Initial GQL query can include credentials to immediately unlock `authorizedData`, but lets just consider the route where it doesn't. - `useAuthenticatedDataQuery` was called which sent a request based on whether the `authorizedData` was already present and on whether credentials did exist (by cleverly using `fetchPolicy`). In our case it first did nothing as no credentials are there. - The `authenticatedData` as stored inside a React context, and not in `event`. - The password form, when submitted would send a GQL request. On success the `setAuthData` setter from the context was called, meaning that all components using the contexts were rerendered. Generally nothing terrible going on here, but: `authorizedData` is really a part of the event and having a separate store, instead of just saving and retrieving it from the Relay store feels weird. There was also a bug where only one context was used per realm, which lead to all video blocks showing the same video if one was unlocked. This commit changes this. The context is completely removed and the data is stored in the relay store, making this more idiomatic. But oh boy, it was not easy to get here. Relay docs are just terrible for me to dig through. So one thing seemed clear fairly quickly: the idiomatic way is `useRefetchableFragment` for this. With that, one can only refetch a small part of the initial query, but with potentially different variables. Perfect! Problem is that the `refetch` function's callback function does not get the fetched data. So it is hard to check if the credentials were correct (and to distinguish other network errors). So my solution at the end is to keep the manual `fetchQuery` as that allows us to check the result in a callback. Then, on success, the `refetch` is called with "store-only" to just rerender the components. One of the big time sinks for me was to understand that the manual query we use with `fetchQuery` needs to be exactly the same as what `refetch` will execute. Specifically: use `node()` and not `eventById`. Refetching only works with fragments, so another layer of fragments needed to be introduced. I added a helper function to combine the event with its `authorizedData` for that. This commit is best viewed with whitespace-diff disabled. --- frontend/src/i18n/locales/en.yaml | 1 + frontend/src/routes/Embed.tsx | 32 +-- frontend/src/routes/Realm.tsx | 10 +- frontend/src/routes/Video.tsx | 390 ++++++++++++++++-------------- frontend/src/ui/Blocks/Video.tsx | 26 +- frontend/src/util/index.ts | 33 +-- 6 files changed, 232 insertions(+), 260 deletions(-) diff --git a/frontend/src/i18n/locales/en.yaml b/frontend/src/i18n/locales/en.yaml index 38691a73a..9b0757e6e 100644 --- a/frontend/src/i18n/locales/en.yaml +++ b/frontend/src/i18n/locales/en.yaml @@ -180,6 +180,7 @@ video: password: heading: Protected Video sub-heading: Access to this video is restricted. + no-preview-permission: $t(api-remote-errors.view.event) body: > Please enter the username and the password you have received to access this video; please note that these are not your login credentials. diff --git a/frontend/src/routes/Embed.tsx b/frontend/src/routes/Embed.tsx index b389664a5..93f232d2e 100644 --- a/frontend/src/routes/Embed.tsx +++ b/frontend/src/routes/Embed.tsx @@ -1,4 +1,4 @@ -import { ReactNode, Suspense, useState } from "react"; +import { ReactNode, Suspense } from "react"; import { LuFrown, LuAlertTriangle } from "react-icons/lu"; import { Translation, useTranslation } from "react-i18next"; import { @@ -7,7 +7,7 @@ import { } from "react-relay"; import { unreachable } from "@opencast/appkit"; -import { eventId, getCredentials, isSynced, keyOfId, useAuthenticatedDataQuery } from "../util"; +import { eventId, getCredentials, isSynced, keyOfId } from "../util"; import { GlobalErrorBoundary } from "../util/err"; import { loadQuery } from "../relay"; import { makeRoute, MatchedRoute } from "../rauta"; @@ -19,7 +19,7 @@ import { EmbedQuery } from "./__generated__/EmbedQuery.graphql"; import { EmbedDirectOpencastQuery } from "./__generated__/EmbedDirectOpencastQuery.graphql"; import { EmbedEventData$key } from "./__generated__/EmbedEventData.graphql"; import { PlayerContextProvider } from "../ui/player/PlayerContext"; -import { AuthenticatedDataContext, AuthorizedData, PreviewPlaceholder } from "./Video"; +import { PreviewPlaceholder, useEventWithAuthData } from "./Video"; export const EmbedVideoRoute = makeRoute({ url: ({ videoId }: { videoId: string }) => `/~embed/!v/${keyOfId(videoId)}`, @@ -120,12 +120,8 @@ const embedEventFragment = graphql` endTime duration } - authorizedData(user: $eventUser, password: $eventPassword) { - thumbnail - tracks { uri flavor mimetype resolution isMaster } - captions { uri lang } - segments { uri startTime } - } + ... VideoPageAuthorizedData + @arguments(eventUser: $eventUser, eventPassword: $eventPassword) } } `; @@ -138,12 +134,12 @@ type EmbedProps = { const Embed: React.FC = ({ query, queryRef }) => { const fragmentRef = usePreloadedQuery(query, queryRef); - const event = useFragment( + const protoEvent = useFragment( embedEventFragment, fragmentRef.event, ); + const [event, refetch] = useEventWithAuthData(protoEvent); const { t } = useTranslation(); - const [authenticatedData, setAuthenticatedData] = useState(null); if (!event) { return @@ -170,17 +166,9 @@ const Embed: React.FC = ({ query, queryRef }) => { ; } - const authorizedData = useAuthenticatedDataQuery( - event.id, - event.series?.id, - { authorizedData: event.authorizedData }, - ); - - return authorizedData - ? - : - ; - ; + return event.authorizedData + ? + : ; }; export const BlockEmbedRoute = makeRoute({ diff --git a/frontend/src/routes/Realm.tsx b/frontend/src/routes/Realm.tsx index 306447f2f..d0faed771 100644 --- a/frontend/src/routes/Realm.tsx +++ b/frontend/src/routes/Realm.tsx @@ -27,7 +27,6 @@ import { COLORS } from "../color"; import { useMenu } from "../layout/MenuState"; import { ManageNav } from "./manage"; import { BREAKPOINT as NAV_BREAKPOINT } from "../layout/Navigation"; -import { AuthorizedData, AuthenticatedDataContext } from "./Video"; // eslint-disable-next-line @typescript-eslint/quotes @@ -146,7 +145,6 @@ const RealmPage: React.FC = ({ realm }) => { const { t } = useTranslation(); const siteTitle = useTranslatedConfig(CONFIG.siteTitle); const breadcrumbs = realmBreadcrumbs(t, realm.ancestors); - const [authenticatedData, setAuthenticatedData] = useState(null); const title = realm.isMainRoot ? siteTitle : realm.name; useTitle(title, realm.isMainRoot); @@ -168,11 +166,9 @@ const RealmPage: React.FC = ({ realm }) => { {realm.isUserRealm && } )} - - {realm.blocks.length === 0 && realm.isMainRoot - ? - : } - + {realm.blocks.length === 0 && realm.isMainRoot + ? + : } ; }; diff --git a/frontend/src/routes/Video.tsx b/frontend/src/routes/Video.tsx index 5fb9e06d7..cb99303db 100644 --- a/frontend/src/routes/Video.tsx +++ b/frontend/src/routes/Video.tsx @@ -1,15 +1,14 @@ import React, { - createContext, - Dispatch, ReactElement, ReactNode, - SetStateAction, - useContext, useEffect, useRef, useState, } from "react"; -import { graphql, GraphQLTaggedNode, PreloadedQuery, useFragment } from "react-relay/hooks"; +import { + graphql, GraphQLTaggedNode, PreloadedQuery, RefetchFnDynamic, useFragment, + useRefetchableFragment, +} from "react-relay/hooks"; import { useTranslation } from "react-i18next"; import { fetchQuery, OperationType } from "relay-runtime"; import { @@ -19,7 +18,7 @@ import { QRCodeCanvas } from "qrcode.react"; import { match, unreachable, screenWidthAtMost, screenWidthAbove, useColorScheme, Floating, FloatingContainer, FloatingTrigger, WithTooltip, Card, Button, ProtoButton, - bug, + notNullish, } from "@opencast/appkit"; import { VideoObject, WithContext } from "schema-dts"; @@ -46,7 +45,6 @@ import { keyOfId, playlistId, getCredentials, - useAuthenticatedDataQuery, credentialsStorageKey, } from "../util"; import { BREAKPOINT_SMALL, BREAKPOINT_MEDIUM } from "../GlobalStyle"; @@ -90,9 +88,11 @@ import { PlaylistBlockFromPlaylist } from "../ui/Blocks/Playlist"; import { AuthenticationFormState, FormData, AuthenticationForm } from "./Login"; import { VideoAuthorizedDataQuery, - VideoAuthorizedDataQuery$data, } from "./__generated__/VideoAuthorizedDataQuery.graphql"; import { AuthorizedBlockEvent } from "../ui/Blocks/Video"; +import { + VideoPageAuthorizedData$data, VideoPageAuthorizedData$key, +} from "./__generated__/VideoPageAuthorizedData.graphql"; // =========================================================================================== @@ -122,6 +122,7 @@ export const VideoRoute = makeRoute({ ... UserData event: eventById(id: $id) { ... VideoPageEventData + @arguments(eventUser: $eventUser, eventPassword: $eventPassword) ... on AuthorizedEvent { isReferencedByRealm(path: $realmPath) } @@ -146,11 +147,7 @@ export const VideoRoute = makeRoute({ {... { query, queryRef }} nav={data => data.realm ?