From 3d1e43199279c8d449e368371c43cb4159443c29 Mon Sep 17 00:00:00 2001 From: Lukas Kalbertodt Date: Wed, 22 Nov 2023 10:31:34 +0100 Subject: [PATCH 01/18] Add `users` database table Users don't originate from Tobira, Tobira doesn't manage or own them. However, this table is still required for a few different things: - A known list of users with their roles for the ACL selector - Bookmarks - Some site settings stored in the account - Sudo mode timing information The first point will be implemented in the next commits. All others are just for the future, but I think adding this information will be easily be possible later. --- backend/src/db/migrations.rs | 1 + backend/src/db/migrations/27-users.sql | 25 +++++++++++++++++++++++++ 2 files changed, 26 insertions(+) create mode 100644 backend/src/db/migrations/27-users.sql diff --git a/backend/src/db/migrations.rs b/backend/src/db/migrations.rs index be72bae23..f0e5711d8 100644 --- a/backend/src/db/migrations.rs +++ b/backend/src/db/migrations.rs @@ -341,4 +341,5 @@ static MIGRATIONS: Lazy> = include_migrations![ 24: "known-groups", 25: "longer-videos", 26: "more-event-search-data", + 27: "users", ]; diff --git a/backend/src/db/migrations/27-users.sql b/backend/src/db/migrations/27-users.sql new file mode 100644 index 000000000..db1798bb9 --- /dev/null +++ b/backend/src/db/migrations/27-users.sql @@ -0,0 +1,25 @@ +select prepare_randomized_ids('users'); + +-- This table stores user information, but note that it's not used for +-- authentication or authorization. That's still handled using external systems, +-- according to 'auth.mode'. +create table users ( + -- Users already have a unique ID but it's usually a good idea using a + -- artificial primary key anyway. See https://dba.stackexchange.com/q/1910/ + id bigint primary key default randomized_id('users'), + username text not null unique, + + -- These three are just a cache. For the actual user session, the values + -- from the login system are always used instead of this. This is just for + -- the ACL selector UI. + display_name text not null, + email text, + user_role text not null, + + -- The last time this user has made an authenticated request. Is null if the + -- user was never seen (i.e. the data was imported in another way). + last_seen timestamp with time zone +); + +-- Looking up users by role is a common operation for resolving roles in ACLs. +create unique index idx_user_role on users (user_role); From 2ba0dd28eb8f60c246ff83758c13cede1e4c40a8 Mon Sep 17 00:00:00 2001 From: Lukas Kalbertodt Date: Wed, 22 Nov 2023 12:06:32 +0100 Subject: [PATCH 02/18] Require, store and expose `user_role` field for users (BREAKING) So far, we had no strict requirements on the user role, we just assumed there to be exactly one user role in the roles. To make the ACL selector work properly, we not require this explicitly. This shouldn't be a problem for anyone as this requirement is likely already fulfilled. --- backend/src/api/model/user.rs | 7 ++++ backend/src/auth/handlers.rs | 10 ++++- backend/src/auth/mod.rs | 64 +++++++++++++++++++++++++++----- docs/docs/setup/auth/in-depth.md | 6 ++- frontend/src/schema.graphql | 6 +++ 5 files changed, 80 insertions(+), 13 deletions(-) diff --git a/backend/src/api/model/user.rs b/backend/src/api/model/user.rs index 0202f4310..3f5230c94 100644 --- a/backend/src/api/model/user.rs +++ b/backend/src/api/model/user.rs @@ -28,6 +28,13 @@ impl User { self.roles.iter().map(AsRef::as_ref).collect() } + /// Returns the *user role* of this user. Each user has exactly one and this + /// role is used in ACLs to give access to a single user. This role is + /// always also contained in `roles`. + fn user_role(&self) -> &str { + &self.user_role + } + /// The name of the user intended to be read by humans. fn display_name(&self) -> &str { &self.display_name diff --git a/backend/src/auth/handlers.rs b/backend/src/auth/handlers.rs index 159bcd10c..5ff92e075 100644 --- a/backend/src/auth/handlers.rs +++ b/backend/src/auth/handlers.rs @@ -194,6 +194,7 @@ async fn check_opencast_login( struct InfoMeResponse { roles: Vec, user: InfoMeUserResponse, + user_role: String, } #[derive(Deserialize)] @@ -204,7 +205,7 @@ async fn check_opencast_login( } let body = hyper::body::to_bytes(response.into_body()).await?; - let info: InfoMeResponse = serde_json::from_slice(&body) + let mut info: InfoMeResponse = serde_json::from_slice(&body) .context("Could not deserialize `/info/me.json` response")?; // If all roles are `ROLE_ANONYMOUS`, then we assume the login was invalid. @@ -212,6 +213,12 @@ async fn check_opencast_login( return Ok(None); } + // Make sure the roles list always contains the user role. This is very + // likely always the case, but better be sure. + if !info.roles.contains(&info.user_role) { + info.roles.push(info.user_role.clone()); + } + // Otherwise the login was correct! Ok(Some(User { username: info.user.username, @@ -220,6 +227,7 @@ async fn check_opencast_login( // Sometimes, Opencast does not include `ROLE_ANONYMOUS` in the // response, so we just add it here to be sure. roles: info.roles.into_iter().chain([ROLE_ANONYMOUS.to_owned()]).collect(), + user_role: info.user_role, })) } diff --git a/backend/src/auth/mod.rs b/backend/src/auth/mod.rs index ad475bc49..2fe923683 100644 --- a/backend/src/auth/mod.rs +++ b/backend/src/auth/mod.rs @@ -157,6 +157,38 @@ impl AuthMode { } } +impl AuthConfig { + /// Finds the user role from the given roles according to + /// `user_role_prefixes`. If none can be found, `None` is returned and a + /// warning is printed. If more than one is found, a warning is printed and + /// the first one is returned. + fn find_user_role<'a>( + &self, + username: &str, + mut roles: impl Iterator, + ) -> Option<&'a str> { + let is_user_role = |role: &&str| { + self.user_role_prefixes.iter().any(|prefix| role.starts_with(prefix)) + }; + + let note = "Check 'auth.user_role_prefixes' and your auth integration."; + let Some(user_role) = roles.by_ref().find(is_user_role) else { + warn!("User '{username}' has no user role, but it needs exactly one. {note}"); + return None; + }; + + + if let Some(extra) = roles.find(is_user_role) { + warn!( + "User '{username}' has multiple user roles ({user_role} and {extra}) \ + but there should be one unique user role per user. {note}", + ); + } + + Some(user_role) + } +} + /// Information about whether or not, and if so how /// someone or something talking to Tobira is authenticated #[derive(PartialEq, Eq)] @@ -173,6 +205,7 @@ pub(crate) struct User { pub(crate) display_name: String, pub(crate) email: Option, pub(crate) roles: HashSet, + pub(crate) user_role: String, } impl AuthContext { @@ -225,7 +258,7 @@ impl User { AuthMode::None => Ok(None), AuthMode::FullAuthProxy => Ok(Self::from_auth_headers(headers, auth_config).into()), AuthMode::LoginProxy | AuthMode::Opencast => { - Self::from_session(headers, db, auth_config.session_duration) + Self::from_session(headers, db, auth_config) .await .map(Into::into) } @@ -253,13 +286,15 @@ impl User { let display_name = get_header(&auth_config.display_name_header)?; let email = get_header(&auth_config.email_header); - // Get roles from the user. If the header is not set, the user simply has no extra roles. + // Get roles from the user. let mut roles = HashSet::from([ROLE_ANONYMOUS.to_string()]); - if let Some(roles_raw) = get_header(&auth_config.roles_header) { - roles.extend(roles_raw.split(',').map(|role| role.trim().to_owned())); - }; + let roles_raw = get_header(&auth_config.roles_header)?; + roles.extend(roles_raw.split(',').map(|role| role.trim().to_owned())); + let user_role = auth_config + .find_user_role(&username, roles.iter().map(|s| s.as_str()))? + .to_owned(); - Some(Self { username, display_name, email, roles }) + Some(Self { username, display_name, email, roles, user_role }) } /// Tries to load user data from a DB session referred to in a session @@ -267,7 +302,7 @@ impl User { async fn from_session( headers: &HeaderMap, db: &Client, - session_duration: Duration, + auth_config: &AuthConfig, ) -> Result, PgError> { // Try to get a session ID from the cookie. let session_id = match SessionId::from_headers(headers) { @@ -282,16 +317,25 @@ impl User { where id = $1 \ and extract(epoch from now() - created) < $2::double precision" ); - let row = match db.query_opt(&query, &[&session_id, &session_duration.as_secs_f64()]).await? { + let session_duration = auth_config.session_duration.as_secs_f64(); + let row = match db.query_opt(&query, &[&session_id, &session_duration]).await? { None => return Ok(None), Some(row) => row, }; + let username: String = mapping.username.of(&row); + let roles = mapping.roles.of::>(&row); + let user_role = auth_config + .find_user_role(&username, roles.iter().map(|s| s.as_str())) + .expect("user session without user role") + .to_owned(); + Ok(Some(Self { - username: mapping.username.of(&row), + username, display_name: mapping.display_name.of(&row), email: mapping.email.of(&row), - roles: mapping.roles.of::>(&row).into_iter().collect(), + roles: roles.into_iter().collect(), + user_role, })) } diff --git a/docs/docs/setup/auth/in-depth.md b/docs/docs/setup/auth/in-depth.md index 488fe5ec6..42d8c8b99 100644 --- a/docs/docs/setup/auth/in-depth.md +++ b/docs/docs/setup/auth/in-depth.md @@ -16,6 +16,7 @@ Tobira requires the following information about each user: An alphabetic ID is preferred over a purely numeric one as it appears in the URL to the user's personal page. - **Display name**: the user's name in a format intended for humans; usually something like "Forename Surname". - **Roles**: a list of roles that are used for authorization (e.g. deciding whether a user is allowed to see a video or modify some data). +- **User role**: the unique user role for that user (e.g. `ROLE_USER_PETER`). In the `"opencast"` mode, this data is retrieved via `/info/me.json` from Opencast. In the to `"*-proxy"` modes, you have to pass this data explicitly to Tobira via so called *auth headers*. @@ -28,8 +29,9 @@ These are just designated HTTP headers, one for each kind of information describ - `x-tobira-username`: Username - `x-tobira-user-display-name`: Display name -- `x-tobira-user-roles`: List of roles, comma separated -- `x-tobira-user-email`: User email address +- `x-tobira-user-roles`: List of roles, comma separated. + Needs to contain exactly one role starting with any of the configured `auth.user_role_prefixes`. +- `x-tobira-user-email`: User email address (optional) All these header values have to be a **base64-encoded UTF-8** string! diff --git a/frontend/src/schema.graphql b/frontend/src/schema.graphql index bb8ed7d7a..a472cf11c 100644 --- a/frontend/src/schema.graphql +++ b/frontend/src/schema.graphql @@ -553,6 +553,12 @@ type User { This endpoint is only for debugging and for special cases like the ACL selector. """ roles: [String!]! + """ + Returns the *user role* of this user. Each user has exactly one and this + role is used in ACLs to give access to a single user. This role is + always also contained in `roles`. + """ + userRole: String! "The name of the user intended to be read by humans." displayName: String! "`True` if the user has the permission to upload videos." From f5f14ea0a63d34c4d0b25388370618ea84b5ccf7 Mon Sep 17 00:00:00 2001 From: Lukas Kalbertodt Date: Wed, 22 Nov 2023 16:02:54 +0100 Subject: [PATCH 03/18] Automatically write user information to DB I included an in-memory cache to avoid excessive writes to the DB. We remember this user info in order for the ACL selector to work well. --- backend/Cargo.lock | 14 ++++++ backend/Cargo.toml | 1 + backend/src/auth/cache.rs | 97 ++++++++++++++++++++++++++++++++++++ backend/src/auth/mod.rs | 25 +++++++--- backend/src/http/handlers.rs | 8 ++- backend/src/http/mod.rs | 4 +- 6 files changed, 139 insertions(+), 10 deletions(-) create mode 100644 backend/src/auth/cache.rs diff --git a/backend/Cargo.lock b/backend/Cargo.lock index 2a81667e8..059953fea 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -531,6 +531,19 @@ dependencies = [ "windows-sys 0.48.0", ] +[[package]] +name = "dashmap" +version = "5.5.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "978747c1d849a7d2ee5e8adc0159961c48fb7e5db2f06af6723b80123bb53856" +dependencies = [ + "cfg-if", + "hashbrown 0.14.2", + "lock_api", + "once_cell", + "parking_lot_core", +] + [[package]] name = "deadpool" version = "0.9.5" @@ -2514,6 +2527,7 @@ dependencies = [ "clap", "confique", "cookie", + "dashmap", "deadpool", "deadpool-postgres", "elliptic-curve", diff --git a/backend/Cargo.toml b/backend/Cargo.toml index c51c9aa17..6b7309818 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -27,6 +27,7 @@ chrono = { version = "0.4", default-features = false, features = ["serde", "std" clap = { version = "4.2.2", features = ["derive", "string"] } confique = { version = "0.2.0", default-features = false, features = ["toml"] } cookie = "0.17.0" +dashmap = "5.5.3" deadpool = { version = "0.9.0", default-features = false, features = ["managed", "rt_tokio_1"] } deadpool-postgres = { version = "0.10", default-features = false, features = ["rt_tokio_1"] } elliptic-curve = { version = "0.13.4", features = ["jwk", "sec1"] } diff --git a/backend/src/auth/cache.rs b/backend/src/auth/cache.rs new file mode 100644 index 000000000..7ede677f7 --- /dev/null +++ b/backend/src/auth/cache.rs @@ -0,0 +1,97 @@ +use std::{time::{Instant, Duration}, collections::HashSet}; + +use dashmap::{DashMap, mapref::entry::Entry}; +use deadpool_postgres::Client; + +use crate::prelude::*; + + +const CACHE_DURATION: Duration = Duration::from_secs(60 * 10); + +struct CacheEntry { + display_name: String, + email: Option, + roles: HashSet, + user_role: String, + last_written_to_db: Instant, +} + +/// Cache to remember what users we have seen. Its only purpose is to make +/// writes to the `users` table less frequent. The data from this cache must +/// not be used in any other way. +/// +/// This works fine in multi-node setups: each node just has its local cache and +/// prevents some DB writes. But as this data is never used otherwise, we don't +/// run into data inconsistency problems. +pub(crate) struct UserCache(DashMap); + +impl UserCache { + pub(crate) fn new() -> Self { + Self(DashMap::new()) + } + + pub(crate) async fn upsert_user_info(&self, user: &super::User, db: &Client) { + match self.0.entry(user.username.clone()) { + Entry::Occupied(mut occupied) => { + let entry = occupied.get(); + let needs_update = entry.last_written_to_db.elapsed() > CACHE_DURATION + || entry.display_name != user.display_name + || entry.email != user.email + || entry.roles != user.roles + || entry.user_role != user.user_role; + + if needs_update { + let res = Self::write_to_db(user, db).await; + if res.is_ok() { + occupied.get_mut().last_written_to_db = Instant::now(); + } + } + }, + Entry::Vacant(vacant) => { + let res = Self::write_to_db(user, db).await; + if res.is_ok() { + vacant.insert(CacheEntry { + display_name: user.display_name.clone(), + email: user.email.clone(), + roles: user.roles.clone(), + user_role: user.user_role.clone(), + last_written_to_db: Instant::now(), + }); + } + }, + } + } + + async fn write_to_db(user: &super::User, db: &Client) -> Result<(), ()> { + let sql = "\ + insert into users (username, display_name, email, user_role, last_seen) \ + values ($1, $2, $3, $4, now()) \ + on conflict (username) do update set \ + display_name = excluded.display_name, \ + email = excluded.email, \ + user_role = excluded.user_role, \ + last_seen = excluded.last_seen \ + "; + let res = db.execute(sql, &[ + &user.username, + &user.display_name, + &user.email, + &user.user_role, + ]).await; + + // We mostly just ignore errors. That's because saving the user data is + // not THAT important. Logging a warning should make sure that this + // doesn't fail all the time. + // + // It's important to note that this user saving makes every API request + // potentially writing to the DB. If the DB is in an emergency read + // only state, that would mean that all API requests would fail. + if let Err(e) = res { + warn!("Updating user data for '{}' failed: {e}", user.username); + return Err(()); + } + + Ok(()) + } +} + diff --git a/backend/src/auth/mod.rs b/backend/src/auth/mod.rs index 2fe923683..b55d2e633 100644 --- a/backend/src/auth/mod.rs +++ b/backend/src/auth/mod.rs @@ -10,11 +10,13 @@ use tokio_postgres::Error as PgError; use crate::{config::TranslatedString, prelude::*, db::util::select}; +mod cache; mod handlers; mod session_id; mod jwt; pub(crate) use self::{ + cache::UserCache, session_id::SessionId, jwt::{JwtConfig, JwtContext}, handlers::{handle_post_session, handle_delete_session, handle_post_login}, @@ -213,6 +215,7 @@ impl AuthContext { headers: &HeaderMap, auth_config: &AuthConfig, db: &Client, + user_cache: &UserCache, ) -> Result { if let Some(given_key) = headers.get("x-tobira-trusted-external-key") { @@ -223,7 +226,7 @@ impl AuthContext { } } - User::new(headers, auth_config, db) + User::new(headers, auth_config, db, user_cache) .await? .map_or(Self::Anonymous, Self::User) .pipe(Ok) @@ -248,21 +251,27 @@ impl AuthContext { impl User { /// Obtains the current user from the given request headers. This is done /// either via auth headers and/or a session cookie, depending on the - /// configuration. + /// configuration. The `users` table is updated if appropriate. pub(crate) async fn new( headers: &HeaderMap, auth_config: &AuthConfig, db: &Client, + user_cache: &UserCache, ) -> Result, PgError> { - match auth_config.mode { - AuthMode::None => Ok(None), - AuthMode::FullAuthProxy => Ok(Self::from_auth_headers(headers, auth_config).into()), + let out = match auth_config.mode { + AuthMode::None => None, + AuthMode::FullAuthProxy => Self::from_auth_headers(headers, auth_config), AuthMode::LoginProxy | AuthMode::Opencast => { - Self::from_session(headers, db, auth_config) - .await - .map(Into::into) + Self::from_session(headers, db, auth_config).await? } + }; + + if let Some(user) = &out { + user_cache.upsert_user_info(user, db).await; } + + + Ok(out) } /// Tries to read user data auth headers (`x-tobira-username`, ...). If the diff --git a/backend/src/http/handlers.rs b/backend/src/http/handlers.rs index 4489e7ed5..96b016bba 100644 --- a/backend/src/http/handlers.rs +++ b/backend/src/http/handlers.rs @@ -264,7 +264,13 @@ async fn handle_api(req: Request, ctx: &Context) -> Result auth, Err(e) => { error!("DB error when checking user session: {}", e); diff --git a/backend/src/http/mod.rs b/backend/src/http/mod.rs index b650f01f0..f660d6628 100644 --- a/backend/src/http/mod.rs +++ b/backend/src/http/mod.rs @@ -21,7 +21,7 @@ use std::{ }; use crate::{ - api, auth::JwtContext, config::Config, metrics, prelude::*, search, default_enable_backtraces, + api, auth::{JwtContext, UserCache}, config::Config, metrics, prelude::*, search, default_enable_backtraces, }; use self::{ assets::Assets, @@ -69,6 +69,7 @@ pub(crate) struct Context { pub(crate) jwt: Arc, pub(crate) search: Arc, pub(crate) metrics: Arc, + pub(crate) user_cache: UserCache, } @@ -90,6 +91,7 @@ pub(crate) async fn serve( config: Arc::new(config), search: Arc::new(search), metrics: Arc::new(metrics::Metrics::new()), + user_cache: UserCache::new(), }); // This sets up all the hyper server stuff. It's a bit of magic and touching From 84b617e6203167ed3036c5aa012b7d1ceac9a5c4 Mon Sep 17 00:00:00 2001 From: Lukas Kalbertodt Date: Wed, 22 Nov 2023 16:13:09 +0100 Subject: [PATCH 04/18] Use `user.userRole` from API instead of finding it manually in frontend --- frontend/src/User.tsx | 2 ++ frontend/src/relay/boundary.tsx | 2 ++ frontend/src/routes/Upload.tsx | 11 +++++++---- frontend/src/ui/Access.tsx | 15 ++++----------- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/frontend/src/User.tsx b/frontend/src/User.tsx index 54fc0a9d8..7345fd943 100644 --- a/frontend/src/User.tsx +++ b/frontend/src/User.tsx @@ -25,6 +25,7 @@ export type User = { canUseEditor: boolean; canCreateUserRealm: boolean; roles: readonly string[]; + userRole: string; }; export const isRealUser = (state: UserState): state is User => ( @@ -59,6 +60,7 @@ export const userDataFragment = graphql` canUseEditor canCreateUserRealm roles + userRole } } `; diff --git a/frontend/src/relay/boundary.tsx b/frontend/src/relay/boundary.tsx index fa6f49b52..42ba134ba 100644 --- a/frontend/src/relay/boundary.tsx +++ b/frontend/src/relay/boundary.tsx @@ -73,6 +73,7 @@ class GraphQLErrorBoundaryImpl extends React.Component { && "canUseEditor" in user && typeof user.canUseEditor === "boolean" && "canCreateUserRealm" in user && typeof user.canCreateUserRealm === "boolean" && "roles" in user && isStringArray(user.roles) + && "userRole" in user && typeof user.userRole === "string" ) { // `userData = user` unfortunately doesn't work here as the type // of the `user` object is not sufficiently narrowed. Relevant @@ -85,6 +86,7 @@ class GraphQLErrorBoundaryImpl extends React.Component { canUseEditor: user.canUseEditor, canCreateUserRealm: user.canCreateUserRealm, roles: user.roles, + userRole: user.userRole, }; } } diff --git a/frontend/src/routes/Upload.tsx b/frontend/src/routes/Upload.tsx index d6cc9ce17..b18340fea 100644 --- a/frontend/src/routes/Upload.tsx +++ b/frontend/src/routes/Upload.tsx @@ -30,7 +30,7 @@ import { Breadcrumbs } from "../ui/Breadcrumbs"; import { ManageNav } from "./manage"; import { COLORS } from "../color"; import { COMMON_ROLES } from "../util/roles"; -import { Acl, getUserRole, AclSelector, knownRolesFragement } from "../ui/Access"; +import { Acl, AclSelector, knownRolesFragement } from "../ui/Access"; import { AccessKnownRolesData$data, AccessKnownRolesData$key, @@ -673,14 +673,17 @@ type MetaDataEditProps = { const MetaDataEdit: React.FC = ({ onSave, disabled, knownRoles }) => { const { t } = useTranslation(); const user = useUser(); - const userRole = getUserRole(user); + if (!isRealUser(user)) { + return unreachable(); + } + const titleFieldId = useId(); const descriptionFieldId = useId(); const seriesFieldId = useId(); const defaultAcl: Acl = { - readRoles: new Set([COMMON_ROLES.ANONYMOUS, userRole]), - writeRoles: new Set([userRole]), + readRoles: new Set([COMMON_ROLES.ANONYMOUS, user.userRole]), + writeRoles: new Set([user.userRole]), }; const { register, handleSubmit, control, formState: { errors } } = useForm({ diff --git a/frontend/src/ui/Access.tsx b/frontend/src/ui/Access.tsx index 710d3f1b0..cb79e1c10 100644 --- a/frontend/src/ui/Access.tsx +++ b/frontend/src/ui/Access.tsx @@ -18,7 +18,7 @@ import { graphql } from "react-relay"; import { i18n } from "i18next"; import { focusStyle } from "."; -import { useUser, isRealUser, UserState } from "../User"; +import { useUser, isRealUser } from "../User"; import { COLORS } from "../color"; import { COMMON_ROLES } from "../util/roles"; import { SelectProps } from "./Input"; @@ -158,11 +158,10 @@ const AclSelect: React.FC = ({ acl, kind }) => { } else { // Always show the current user first, if included. Then show all known // users, then all unknown ones, both in alphabetical order. - const currentUserRole = getUserRole(user); const collator = new Intl.Collator(i18n.resolvedLanguage, { sensitivity: "base" }); selection.sort((a, b) => { const section = (x: Option) => { - if (x.value === currentUserRole) { + if (isRealUser(user) && x.value === user.userRole) { return 0; } if (x.label.startsWith("ROLE_")) { @@ -339,7 +338,7 @@ const ListEntry: React.FC = ({ remove, item, kind }) => { .filter(role => acl.readRoles.has(role) && (!canWrite || acl.writeRoles.has(role))) .map(role => getLabel(role, knownGroups, i18n)); const isSubset = supersets.length > 0; - const isUser = item.value === getUserRole(user); + const isUser = isRealUser(user) && item.value === user.userRole; let label: JSX.Element; if (isUser) { @@ -458,7 +457,7 @@ const ActionsMenu: React.FC = ({ item, kind }) => { return [COMMON_ROLES.ADMIN, COMMON_ROLES.USER_ADMIN].includes(item.value) - || userIsRequired && item.value === getUserRole(user) + || userIsRequired && isRealUser(user) && item.value === user.userRole ? {t("manage.access.table.actions.write")} : { }; -export const getUserRole = (user: UserState) => { - const userRole = isRealUser(user) && user.roles.find(isUserRole); - return typeof userRole !== "string" ? "Unknown" : userRole; -}; - - // ============================================================================================== // ===== Known groups & users From 1fc73ebe1ad291104fc4988e8c99d34acee9e985 Mon Sep 17 00:00:00 2001 From: Lukas Kalbertodt Date: Wed, 29 Nov 2023 12:18:10 +0100 Subject: [PATCH 05/18] Add `acl` endpoint to `event` API returning roles with labels Quite a bit of discussion went into how this API should work. There are various ways to represent ACLs: 1. { read: [{ role: "ROLE_ANONYMOUS", label: "..." }, ...], write: [...], } 2. { read: { "ROLE_ANONYMOUS": "label..." }, ... }, write: { ... }, } 3. { "ROLE_ANONYMOUS": { label: "...", actions: ["read", ...] }, ... } 4. [ { role: "ROLE_ANONYMOUS", label: "...", actions: ["read", ...] }, ... ] 5. [ { role: "ROLE_ANONYMOUS", label: "...", action: "read" }, { role: "ROLE_ANONYMOUS", label: "...", action: "annotate" }, ... ] The last one is the way Opencast represents ACLs internally. I dislike that as it repeats the role and label for each action. Options 1, 2 and 3 are problematic because of dynamic keys. The actions (read, write, ...) and the roles itself are used as object keys there. That doesn't work well with GraphQL which is statically typed. We can make it work by having a custom scalar for this data structure. But scalars are "atoms" for GraphQL, i.e. one cannot select a subset of fields then. That's likely not necessary for us but still maybe not optimal, also because the individual fields are not present in GraphQL's schema/docs then. This leaves us with option 4. Only annoying thing here is that we use an array where each role must only appear once. A map (JSON object) would be fitting to represent that constraint but oh well. --- backend/src/api/model/acl.rs | 118 +++++++++++++++++++++++++++++++++ backend/src/api/model/event.rs | 11 ++- backend/src/api/model/mod.rs | 1 + frontend/src/schema.graphql | 51 ++++++++++++-- 4 files changed, 174 insertions(+), 7 deletions(-) create mode 100644 backend/src/api/model/acl.rs diff --git a/backend/src/api/model/acl.rs b/backend/src/api/model/acl.rs new file mode 100644 index 000000000..00f2bacb3 --- /dev/null +++ b/backend/src/api/model/acl.rs @@ -0,0 +1,118 @@ +use juniper::GraphQLObject; +use postgres_types::BorrowToSql; + +use crate::{api::{util::TranslatedString, Context, err::ApiResult}, db::util::select}; + + + + +pub(crate) type Acl = Vec; + +/// A role being granted permission to perform certain actions. +#[derive(Debug, GraphQLObject)] +#[graphql(context = Context)] +pub(crate) struct AclItem { + /// Role. In arrays of AclItems, no two items have the same `role`. + pub role: String, + + /// List of actions this role can perform (e.g. `read`, `write`, + /// `annotate`). This is a set, i.e. no duplicate elements. + pub actions: Vec, + + /// Additional info we have about the role. Is `null` if the role is unknown + /// or is `ROLE_ANONYMOUS`, `ROLE_ADMIN` or `ROLE_USER`, as those are + /// handled in a special way in the frontend. + pub info: Option, +} + +/// Some extra information we know about a role. +#[derive(Debug, GraphQLObject)] +#[graphql(context = Context)] +pub(crate) struct RoleInfo { + /// A user-facing label for this role (group or person). If the label does + /// not depend on the language (e.g. a name), `{ "_": "Peter" }` is + /// returned. + pub label: TranslatedString, + + /// For user roles this is `null`. For groups, it defines a list of other + /// group roles that this role implies. I.e. a user with this role always + /// also has these other roles. + pub implies: Option>, + + /// Is `true` if this role represents a large group. Used to warn users + /// accidentally giving write access to large groups. + pub large: bool, +} + +pub(crate) async fn load_for( + context: &Context, + raw_roles: &str, + params: I, +) -> ApiResult +where + P: BorrowToSql, + I: IntoIterator + std::fmt::Debug, + I::IntoIter: ExactSizeIterator, +{ + // First: load labels for roles from the DB. For that we use the `users` + // and `known_groups` table. + let (selection, mapping) = select!( + role: "roles.role", + actions, + implies, + large: "coalesce(known_groups.large, false)", + label: "coalesce( + known_groups.label, + case when users.display_name is null + then null + else hstore('_', users.display_name) + end + )", + ); + let sql = format!("\ + with raw_roles as ({raw_roles}), + roles as ( + select role, array_agg(action) as actions + from raw_roles + group by role + ) + select {selection} + from roles + left join users on users.user_role = role + left join known_groups on known_groups.role = roles.role\ + "); + + context.db.query_mapped(&sql, params, |row| { + AclItem { + role: mapping.role.of(&row), + actions: mapping.actions.of(&row), + info: mapping.label.of::>(&row).map(|label| RoleInfo { + label, + implies: mapping.implies.of(&row), + large: mapping.large.of(&row), + }), + } + }).await.map_err(Into::into) + + // let mut labels = context.db.query_raw(&sql, params) + // .await? + // .map_ok(|row| (row.get::<_, String>(0), row.get::<_, Option>>(1))) + // .try_collect::>() + // .await?; + + // // Assemble everything. This will likely change in the future once we + // // allow arbitrary actions. + // let mut map = >>::new(); + // for (list, action) in [(&self.read_roles, "read"), (&self.write_roles, "write")] { + // for role in list { + // map.entry(role).or_default().push(action.into()); + // } + // } + + // Ok(map.into_iter().map(|(role, actions)| AclItem { + // role: role.into(), + // // Roles are unique so we can `remove` her to avoid cloning. + // label: labels.remove(role).flatten(), + // actions, + // }).collect()) +} diff --git a/backend/src/api/model/event.rs b/backend/src/api/model/event.rs index 997cf04c8..da424b30a 100644 --- a/backend/src/api/model/event.rs +++ b/backend/src/api/model/event.rs @@ -11,7 +11,7 @@ use crate::{ Context, Cursor, Id, Node, NodeValue, common::NotAllowed, err::{self, ApiResult, invalid_input}, - model::{series::Series, realm::Realm}, + model::{series::Series, realm::Realm, acl::{Acl, self}}, }, db::{ types::{EventTrack, EventState, Key, ExtraMetadata, EventCaption}, @@ -221,6 +221,15 @@ impl AuthorizedEvent { .await? .pipe(Ok) } + + async fn acl(&self, context: &Context) -> ApiResult { + let raw_roles_sql = "\ + select unnest(read_roles) as role, 'read' as action from events where id = $1 + union + select unnest(write_roles) as role, 'write' as action from events where id = $1 + "; + acl::load_for(context, raw_roles_sql, dbargs![&self.key]).await + } } #[derive(juniper::GraphQLUnion)] diff --git a/backend/src/api/model/mod.rs b/backend/src/api/model/mod.rs index e39c84080..589068aef 100644 --- a/backend/src/api/model/mod.rs +++ b/backend/src/api/model/mod.rs @@ -1,6 +1,7 @@ //! This module and its children define most of the application logic of the //! API. +pub(crate) mod acl; pub(crate) mod block; pub(crate) mod event; pub(crate) mod known_roles; diff --git a/frontend/src/schema.graphql b/frontend/src/schema.graphql index a472cf11c..7b809f462 100644 --- a/frontend/src/schema.graphql +++ b/frontend/src/schema.graphql @@ -119,14 +119,29 @@ type Series { events(order: EventSortOrder = {column: "CREATED", direction: "DESCENDING"}): [AuthorizedEvent!]! } -union EventSearchOutcome = SearchUnavailable | EventSearchResults - -input NewVideoBlock { - event: ID! - showTitle: Boolean! - showLink: Boolean! +"Some extra information we know about a role." +type RoleInfo { + """ + A user-facing label for this role (group or person). If the label does + not depend on the language (e.g. a name), `{ "_": "Peter" }` is + returned. + """ + label: TranslatedString! + """ + For user roles this is `null`. For groups, it defines a list of other + group roles that this role implies. I.e. a user with this role always + also has these other roles. + """ + implies: [String!] + """ + Is `true` if this role represents a large group. Used to warn users + accidentally giving write access to large groups. + """ + large: Boolean! } +union EventSearchOutcome = SearchUnavailable | EventSearchResults + """ A group selectable in the ACL UI. Basically a mapping from role to a nice label and info about the relationship to other roles/groups. @@ -138,6 +153,12 @@ type KnownGroup { large: Boolean! } +input NewVideoBlock { + event: ID! + showTitle: Boolean! + showLink: Boolean! +} + type SyncedEventData implements Node { updated: DateTimeUtc! startTime: DateTimeUtc @@ -207,6 +228,7 @@ type AuthorizedEvent implements Node { series: Series "Returns a list of realms where this event is referenced (via some kind of block)." hostRealms: [Realm!]! + acl: [AclItem!]! } "A block just showing some title." @@ -327,6 +349,23 @@ type Realm implements Node { references(id: ID!): Boolean! } +"A role being granted permission to perform certain actions." +type AclItem { + "Role. In arrays of AclItems, no two items have the same `role`." + role: String! + """ + List of actions this role can perform (e.g. `read`, `write`, + `annotate`). This is a set, i.e. no duplicate elements. + """ + actions: [String!]! + """ + Additional info we have about the role. Is `null` if the role is unknown + or is `ROLE_ANONYMOUS`, `ROLE_ADMIN` or `ROLE_USER`, as those are + handled in a special way in the frontend. + """ + info: RoleInfo +} + union SeriesSearchOutcome = SearchUnavailable | SeriesSearchResults "Exactly one of `plain` or `block` has to be non-null." From 85bda6a7370647b5e89e1f10a96584bfae118f91 Mon Sep 17 00:00:00 2001 From: Lukas Kalbertodt Date: Mon, 4 Dec 2023 19:00:44 +0100 Subject: [PATCH 06/18] Make ACL UI use `event.acl` for role info and refactor some things This commit should ideally be multiple ones, but it was too hard to disentangle the changes. More or less independent changes: - Remove the `onPaste` handler. It currently works by matching names, but those are not unique. We have to find a better way to make that work. In the meantime, to not disturb development of the next commit, remove it. - Factor out some stuff into `TableRow`. This also allows to model the special ROLE_ADMIN entry a bit nicer. - Change the `Acl` data structure to use the `event.acl` endpoint. This entails some refactorings. --- frontend/src/routes/Upload.tsx | 31 +- frontend/src/routes/manage/Video/Access.tsx | 31 +- frontend/src/routes/manage/Video/Shared.tsx | 3 +- frontend/src/ui/Access.tsx | 503 +++++++++++--------- 4 files changed, 324 insertions(+), 244 deletions(-) diff --git a/frontend/src/routes/Upload.tsx b/frontend/src/routes/Upload.tsx index b18340fea..82f2b828a 100644 --- a/frontend/src/routes/Upload.tsx +++ b/frontend/src/routes/Upload.tsx @@ -681,10 +681,20 @@ const MetaDataEdit: React.FC = ({ onSave, disabled, knownRole const descriptionFieldId = useId(); const seriesFieldId = useId(); - const defaultAcl: Acl = { - readRoles: new Set([COMMON_ROLES.ANONYMOUS, user.userRole]), - writeRoles: new Set([user.userRole]), - }; + const defaultAcl: Acl = new Map([ + [user.userRole, { + actions: new Set(["read", "write"]), + info: { + label: { "_": user.displayName }, + implies: null, + large: false, + }, + }], + [COMMON_ROLES.ANONYMOUS, { + actions: new Set(["read"]), + info: null, + }], + ]); const { register, handleSubmit, control, formState: { errors } } = useForm({ mode: "onChange", @@ -1036,8 +1046,7 @@ const finishUpload = async ( throw `Field \`userRole\` from 'info/me.json' is not valid: ${userRole}`; } - const { readRoles, writeRoles } = metadata.acl; - const acl = constructAcl([...readRoles], [...writeRoles]); + const acl = constructAcl(metadata.acl); const body = new FormData(); body.append("flavor", "security/xacml+episode"); body.append("mediaPackage", mediaPackage); @@ -1097,7 +1106,7 @@ const constructDcc = (metadata: Metadata, user: User): string => { }; /** Constructs an ACL XML description from the given roles that are allowd to read/write */ -const constructAcl = (readRoles: string[], writeRoles: string[]): string => { +const constructAcl = (acl: Acl): string => { // TODO: maybe we should escape the role somehow? const makeRule = (action: string, role: string): string => ` @@ -1120,9 +1129,8 @@ const constructAcl = (readRoles: string[], writeRoles: string[]): string => { `; - - const readRules = readRoles.map(role => makeRule("read", role)); - const writeRules = writeRoles.map(role => makeRule("write", role)); + const rules = [...acl.entries()] + .flatMap(([role, info]) => [...info.actions].map(action => makeRule(action, role))); return ` @@ -1131,8 +1139,7 @@ const constructAcl = (readRoles: string[], writeRoles: string[]): string => { "urn:oasis:names:tc:xacml:1.0:rule-combining-algorithm:permit-overrides" Version="2.0" xmlns="urn:oasis:names:tc:xacml:2.0:policy:schema:os"> - ${readRules.join("\n")} - ${writeRules.join("\n")} + ${rules.join("\n")} `.trim(); }; diff --git a/frontend/src/routes/manage/Video/Access.tsx b/frontend/src/routes/manage/Video/Access.tsx index 1aba4a935..5ab88e74f 100644 --- a/frontend/src/routes/manage/Video/Access.tsx +++ b/frontend/src/routes/manage/Video/Access.tsx @@ -1,20 +1,21 @@ import { useTranslation } from "react-i18next"; +import { WithTooltip } from "@opencast/appkit"; +import { Dispatch, RefObject, SetStateAction, useRef, useState } from "react"; +import { LuInfo } from "react-icons/lu"; +import { useFragment } from "react-relay"; + import { Breadcrumbs } from "../../../ui/Breadcrumbs"; import { AuthorizedEvent, makeManageVideoRoute } from "./Shared"; import { PageTitle } from "../../../layout/header/ui"; -import { Dispatch, RefObject, SetStateAction, useRef, useState } from "react"; import { COLORS } from "../../../color"; -import { LuInfo } from "react-icons/lu"; import { Button, Kind as ButtonKind } from "../../../ui/Button"; import { isRealUser, useUser } from "../../../User"; import { NotAuthorized } from "../../../ui/error"; -import { WithTooltip } from "@opencast/appkit"; import { Modal, ModalHandle } from "../../../ui/Modal"; import { currentRef, keyOfId } from "../../../util"; import { COMMON_ROLES } from "../../../util/roles"; import { Acl, AclSelector, knownRolesFragement } from "../../../ui/Access"; import { useNavBlocker } from "../../util"; -import { useFragment } from "react-relay"; import { AccessKnownRolesData$data, AccessKnownRolesData$key, @@ -89,10 +90,12 @@ type AccessUIProps = { const AccessUI: React.FC = ({ event, knownRoles }) => { - const initialAcl: Acl = { - readRoles: new Set(event.readRoles), - writeRoles: new Set(event.writeRoles), - }; + const initialAcl: Acl = new Map( + event.acl.map(item => [item.role, { + actions: new Set(item.actions), + info: item.info, + }]) + ); const [selections, setSelections] = useState(initialAcl); @@ -123,13 +126,15 @@ const ButtonWrapper: React.FC = ({ selections, setSelections const resetModalRef = useRef(null); const containsUser = (acl: Acl) => isRealUser(user) - && user.roles.some(r => r === COMMON_ROLES.ADMIN || acl.writeRoles.has(r)); + && user.roles.some(r => r === COMMON_ROLES.ADMIN || acl.get(r)?.actions.has("write")); - const compareSets = (a: Set, b: Set) => + const areSetsEqual = (a: Set, b: Set) => a.size === b.size && [...a].every((str => b.has(str))); - - const selectionIsInitial = compareSets(selections.readRoles, initialAcl.readRoles) - && compareSets(selections.writeRoles, initialAcl.writeRoles); + const selectionIsInitial = selections.size === initialAcl.size + && [...selections].every(([role, info]) => { + const other = initialAcl.get(role); + return other && areSetsEqual(other.actions, info.actions); + }); const submit = async (acl: Acl) => { // TODO: Actually save new ACL. diff --git a/frontend/src/routes/manage/Video/Shared.tsx b/frontend/src/routes/manage/Video/Shared.tsx index 1901a965d..a790e609f 100644 --- a/frontend/src/routes/manage/Video/Shared.tsx +++ b/frontend/src/routes/manage/Video/Shared.tsx @@ -82,8 +82,7 @@ const query = graphql` created canWrite isLive - readRoles - writeRoles + acl { role actions info { label implies large } } syncedData { duration thumbnail diff --git a/frontend/src/ui/Access.tsx b/frontend/src/ui/Access.tsx index cb79e1c10..5e19f7611 100644 --- a/frontend/src/ui/Access.tsx +++ b/frontend/src/ui/Access.tsx @@ -15,7 +15,7 @@ import { LuX, LuAlertTriangle } from "react-icons/lu"; import { MultiValue } from "react-select"; import CreatableSelect from "react-select/creatable"; import { graphql } from "react-relay"; -import { i18n } from "i18next"; +import { i18n, ParseKeys } from "i18next"; import { focusStyle } from "."; import { useUser, isRealUser } from "../User"; @@ -29,15 +29,22 @@ import CONFIG from "../config"; -export type Acl = { - readRoles: Set; - writeRoles: Set; + +export type Acl = Map; + info: RoleInfo | null; +}>; + +export type RoleInfo = { + label: TranslatedLabel; + implies: readonly string[] | null; + large: boolean; }; type Action = "read" | "write"; -type Option = { - value: string; +type SelectOption = { + role: string; label: string; } @@ -45,9 +52,12 @@ type AclContext = { userIsRequired: boolean; acl: Acl; change: (f: (acl: Acl) => void) => void; - knownGroups: KnownRoles; + knownGroups: Map; + large: boolean; + }>; groupDag: GroupDag; - largeGroups: Set; } const AclContext = createContext(null); @@ -68,27 +78,34 @@ type AclSelectorProps = { export const AclSelector: React.FC = ( { acl, userIsRequired = false, onChange, knownRoles } ) => { + const { i18n } = useTranslation(); + const knownGroups = [...knownRoles.knownGroups]; + insertBuiltinRoleInfo(acl, knownGroups, i18n); const [groupAcl, userAcl] = splitAcl(acl); const change: AclContext["change"] = f => { - const copy = { - readRoles: new Set(acl.readRoles), - writeRoles: new Set(acl.writeRoles), - }; + const copy = new Map([...acl.entries()].map(([role, value]) => [role, { + actions: new Set(value.actions), + info: value.info == null ? null : { + label: value.info.label, + implies: value.info.implies, + large: value.info.large, + }, + }])); f(copy); onChange(copy); }; - const customGroups: KnownRoles = Object.fromEntries(knownRoles.knownGroups.map(group => [ - group.role, - i18n => group.label[i18n.language] ?? group.label.en ?? Object.values(group.label)[0], - ])); - const knownGroups = { ...BUILTIN_GROUPS, ...customGroups }; - const groupDag = buildDag(knownRoles.knownGroups); - const largeGroups = new Set([ - ...BUILTIN_LARGE_GROUPS, - ...knownRoles.knownGroups.filter(g => g.large).map(g => g.role), - ]); - - const context = { userIsRequired, acl, change, knownGroups, groupDag, largeGroups }; + + const context = { + userIsRequired, + acl, + change, + groupDag: buildDag(knownGroups), + knownGroups: new Map(knownGroups.map(g => [g.role, { + label: g.label, + implies: new Set(g.implies), + large: g.large, + }])), + }; return
; + + /** + * Resolved label. The value from the API, but also built-in groups + * evaulated and fallback to just the role. + */ + label: string; + + /** Whether this is a large group. `false` for unknown roles. */ + large: boolean; +}; type AclSelectProps = SelectProps & { acl: Acl; @@ -125,13 +159,9 @@ const AclSelect: React.FC = ({ acl, kind }) => { const [menuIsOpen, setMenuIsOpen] = useState(false); const userIsAdmin = isRealUser(user) && user.roles.includes(COMMON_ROLES.ADMIN); - // Turn known roles into selectable options that react-select understands. - const knownRoles = kind === "Group" ? knownGroups : KNOWN_USERS; - const options = Object.entries(knownRoles) - .map(([role, label]) => ({ - value: role, - label: label(i18n), - })); + const options = kind === "Group" + ? Object.entries(knownGroups).map(([role, { label }]) => ({ role, label: label(i18n) })) + : Object.entries(KNOWN_USERS).map(([role, label]) => ({ role, label: label(i18n) })); const translations = match(kind, { "Group": () => ({ @@ -146,22 +176,23 @@ const AclSelect: React.FC = ({ acl, kind }) => { }), }); - - let selection: Option[] = [...new Set([...acl.readRoles, ...acl.writeRoles])] - .map(role => ({ - value: role, - label: getLabel(role, knownRoles, i18n), - })); + // Sort the active ACL entries (and put them into a new variable for that). + let entries: Entry[] = [...acl.entries()].map(([role, { actions, info }]) => ({ + role, + actions, + label: getLabel(role, info?.label, i18n), + large: info?.large ?? false, + })); if (kind === "Group") { // Sort large groups to the top. - selection = groupDag.sort(selection); + entries = groupDag.sort(entries); } else { // Always show the current user first, if included. Then show all known // users, then all unknown ones, both in alphabetical order. const collator = new Intl.Collator(i18n.resolvedLanguage, { sensitivity: "base" }); - selection.sort((a, b) => { - const section = (x: Option) => { - if (isRealUser(user) && x.value === user.userRole) { + entries.sort((a, b) => { + const section = (x: Entry) => { + if (isRealUser(user) && x.role === user.userRole) { return 0; } if (x.label.startsWith("ROLE_")) { @@ -179,38 +210,33 @@ const AclSelect: React.FC = ({ acl, kind }) => { }); } - const remove = (item: Option) => change(prev => { - prev.readRoles.delete(item.value); - prev.writeRoles.delete(item.value); - }); + const remove = (item: Entry) => change(prev => prev.delete(item.role)); const handleCreate = (inputValue: string) => change(prev => { - prev.readRoles.add(inputValue); + prev.set(inputValue, { + // If "create" is called, a new option is created, meaning that we + // don't know the role. + info: null, + actions: new Set(["read"]), + }); }); - const handleChange = (choice: MultiValue