diff --git a/src/controllers/crate_owner_invitation.rs b/src/controllers/crate_owner_invitation.rs index 8e83c6abe55..cade05e51ac 100644 --- a/src/controllers/crate_owner_invitation.rs +++ b/src/controllers/crate_owner_invitation.rs @@ -1,9 +1,10 @@ use crate::app::AppState; use crate::auth::AuthCheck; use crate::auth::Authentication; +use crate::controllers::helpers::authorization::Rights; use crate::controllers::helpers::pagination::{Page, PaginationOptions, PaginationQueryParams}; use crate::models::crate_owner_invitation::AcceptError; -use crate::models::{Crate, CrateOwnerInvitation, Rights, User}; +use crate::models::{Crate, CrateOwnerInvitation, User}; use crate::schema::{crate_owner_invitations, crates, users}; use crate::util::errors::{bad_request, custom, forbidden, internal, AppResult, BoxedAppError}; use crate::util::RequestUtils; @@ -157,7 +158,7 @@ async fn prepare_list( // Only allow crate owners to query pending invitations for their crate. let krate: Crate = Crate::by_name(&crate_name).first(conn).await?; let owners = krate.owners(conn).await?; - if user.rights(&*state.github, &owners).await? != Rights::Full { + if Rights::get(user, &*state.github, &owners).await? != Rights::Full { let detail = "only crate owners can query pending invitations for their crate"; return Err(forbidden(detail)); } diff --git a/src/controllers/helpers.rs b/src/controllers/helpers.rs index 0263695e2b0..dff8702d5d6 100644 --- a/src/controllers/helpers.rs +++ b/src/controllers/helpers.rs @@ -2,6 +2,7 @@ use crate::util::errors::AppResult; use axum::response::{IntoResponse, Response}; use axum_extra::json; +pub mod authorization; pub(crate) mod pagination; pub(crate) use self::pagination::Paginate; diff --git a/src/controllers/helpers/authorization.rs b/src/controllers/helpers/authorization.rs new file mode 100644 index 00000000000..e33f4a94f4b --- /dev/null +++ b/src/controllers/helpers/authorization.rs @@ -0,0 +1,57 @@ +use crate::models::{Owner, User}; +use crates_io_github::{GitHubClient, GitHubError}; +use oauth2::AccessToken; +use secrecy::ExposeSecret; + +/// Access rights to the crate (publishing and ownership management) +/// NOTE: The order of these variants matters! +#[derive(PartialEq, Eq, PartialOrd, Ord, Debug, Clone, Copy)] +pub enum Rights { + None, + Publish, + Full, +} + +impl Rights { + /// Given this set of owners, determines the strongest rights the + /// user has. + /// + /// Short-circuits on `Full` because you can't beat it. In practice, we'll always + /// see `[user, user, user, ..., team, team, team]`, so we could shortcircuit on + /// `Publish` as well, but this is a non-obvious invariant so we don't bother. + /// Sweet free optimization if teams are proving burdensome to check. + /// More than one team isn't really expected, though. + pub async fn get( + user: &User, + gh_client: &dyn GitHubClient, + owners: &[Owner], + ) -> Result { + let token = AccessToken::new(user.gh_access_token.expose_secret().to_string()); + + let mut best = Self::None; + for owner in owners { + match *owner { + Owner::User(ref other_user) => { + if other_user.id == user.id { + return Ok(Self::Full); + } + } + Owner::Team(ref team) => { + // Phones home to GitHub to ask if this User is a member of the given team. + // Note that we're assuming that the given user is the one interested in + // the answer. If this is not the case, then we could accidentally leak + // private membership information here. + let is_team_member = gh_client + .team_membership(team.org_id, team.github_id, &user.gh_login, &token) + .await? + .is_some_and(|m| m.is_active()); + + if is_team_member { + best = Self::Publish; + } + } + } + } + Ok(best) + } +} diff --git a/src/controllers/krate/delete.rs b/src/controllers/krate/delete.rs index 6dfb4d7b156..56297279117 100644 --- a/src/controllers/krate/delete.rs +++ b/src/controllers/krate/delete.rs @@ -1,8 +1,9 @@ use crate::app::AppState; use crate::auth::AuthCheck; +use crate::controllers::helpers::authorization::Rights; use crate::controllers::krate::CratePath; use crate::email::Email; -use crate::models::{NewDeletedCrate, Rights}; +use crate::models::NewDeletedCrate; use crate::schema::{crate_downloads, crates, dependencies}; use crate::util::errors::{custom, AppResult, BoxedAppError}; use crate::worker::jobs; @@ -68,7 +69,7 @@ pub async fn delete_crate( // Check that the user is an owner of the crate (team owners are not allowed to delete crates) let user = auth.user(); let owners = krate.owners(&mut conn).await?; - match user.rights(&*app.github, &owners).await? { + match Rights::get(user, &*app.github, &owners).await? { Rights::Full => {} Rights::Publish => { let msg = "team members don't have permission to delete crates"; diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index 74e92b5c919..7ae56e2a7db 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -1,12 +1,13 @@ //! All routes related to managing owners of a crate +use crate::controllers::helpers::authorization::Rights; use crate::controllers::krate::CratePath; use crate::models::krate::OwnerRemoveError; use crate::models::{ krate::NewOwnerInvite, token::EndpointScope, CrateOwner, NewCrateOwnerInvitation, - NewCrateOwnerInvitationOutcome, + NewCrateOwnerInvitationOutcome, NewTeam, }; -use crate::models::{Crate, Owner, Rights, Team, User}; +use crate::models::{Crate, Owner, Team, User}; use crate::util::errors::{bad_request, crate_not_found, custom, AppResult, BoxedAppError}; use crate::views::EncodableOwner; use crate::{app::AppState, App}; @@ -15,12 +16,13 @@ use axum::Json; use axum_extra::json; use axum_extra::response::ErasedJson; use chrono::Utc; -use crates_io_github::GitHubClient; +use crates_io_github::{GitHubClient, GitHubError}; use diesel::prelude::*; use diesel_async::scoped_futures::ScopedFutureExt; use diesel_async::{AsyncConnection, AsyncPgConnection, RunQueryDsl}; use http::request::Parts; use http::StatusCode; +use oauth2::AccessToken; use secrecy::{ExposeSecret, SecretString}; use thiserror::Error; @@ -176,7 +178,7 @@ async fn modify_owners( let owners = krate.owners(conn).await?; - match user.rights(&*app.github, &owners).await? { + match Rights::get(user, &*app.github, &owners).await? { Rights::Full => {} // Yes! Rights::Publish => { @@ -336,8 +338,26 @@ async fn add_team_owner( krate: &Crate, login: &str, ) -> Result { + // github:rust-lang:owners + let mut chunks = login.split(':'); + + let team_system = chunks.next().unwrap(); + if team_system != "github" { + let error = "unknown organization handler, only 'github:org:team' is supported"; + return Err(bad_request(error).into()); + } + + // unwrap is documented above as part of the calling contract + let org = chunks.next().unwrap(); + let team = chunks.next().ok_or_else(|| { + let error = "missing github team argument; format is github:org:team"; + bad_request(error) + })?; + // Always recreate teams to get the most up-to-date GitHub ID - let team = Team::create_or_update(gh_client, conn, login, req_user).await?; + let team = + create_or_update_github_team(gh_client, conn, &login.to_lowercase(), org, team, req_user) + .await?; // Teams are added as owners immediately, since the above call ensures // the user is a team member. @@ -352,6 +372,84 @@ async fn add_team_owner( Ok(NewOwnerInvite::Team(team)) } +/// Tries to create or update a Github Team. Assumes `org` and `team` are +/// correctly parsed out of the full `name`. `name` is passed as a +/// convenience to avoid rebuilding it. +pub async fn create_or_update_github_team( + gh_client: &dyn GitHubClient, + conn: &mut AsyncPgConnection, + login: &str, + org_name: &str, + team_name: &str, + req_user: &User, +) -> AppResult { + // GET orgs/:org/teams + // check that `team` is the `slug` in results, and grab its data + + // "sanitization" + fn is_allowed_char(c: char) -> bool { + matches!(c, 'a'..='z' | 'A'..='Z' | '0'..='9' | '-' | '_') + } + + if let Some(c) = org_name.chars().find(|c| !is_allowed_char(*c)) { + return Err(bad_request(format_args!( + "organization cannot contain special \ + characters like {c}" + ))); + } + + let token = AccessToken::new(req_user.gh_access_token.expose_secret().to_string()); + let team = gh_client.team_by_name(org_name, team_name, &token).await + .map_err(|_| { + bad_request(format_args!( + "could not find the github team {org_name}/{team_name}. \ + Make sure that you have the right permissions in GitHub. \ + See https://doc.rust-lang.org/cargo/reference/publishing.html#github-permissions" + )) + })?; + + let org_id = team.organization.id; + let gh_login = &req_user.gh_login; + + let is_team_member = gh_client + .team_membership(org_id, team.id, gh_login, &token) + .await? + .is_some_and(|m| m.is_active()); + + let can_add_team = + is_team_member || is_gh_org_owner(gh_client, org_id, gh_login, &token).await?; + + if !can_add_team { + return Err(custom( + StatusCode::FORBIDDEN, + "only members of a team or organization owners can add it as an owner", + )); + } + + let org = gh_client.org_by_name(org_name, &token).await?; + + NewTeam::builder() + .login(&login.to_lowercase()) + .org_id(org_id) + .github_id(team.id) + .maybe_name(team.name.as_deref()) + .maybe_avatar(org.avatar_url.as_deref()) + .build() + .create_or_update(conn) + .await + .map_err(Into::into) +} + +async fn is_gh_org_owner( + gh_client: &dyn GitHubClient, + org_id: i32, + gh_login: &str, + token: &AccessToken, +) -> Result { + let membership = gh_client.org_membership(org_id, gh_login, token).await?; + Ok(membership.is_some_and(|m| m.is_active_admin())) +} + /// Error results from a [`add_owner()`] model call. #[derive(Debug, Error)] enum OwnerAddError { diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index 26f4f7a9df2..aa35c6df530 100644 --- a/src/controllers/krate/publish.rs +++ b/src/controllers/krate/publish.rs @@ -28,9 +28,10 @@ use url::Url; use crate::models::{ default_versions::Version as DefaultVersion, Category, Crate, DependencyKind, Keyword, - NewCrate, NewVersion, NewVersionOwnerAction, Rights, VersionAction, + NewCrate, NewVersion, NewVersionOwnerAction, VersionAction, }; +use crate::controllers::helpers::authorization::Rights; use crate::licenses::parse_license_expr; use crate::middleware::log_request::RequestLogExt; use crate::models::token::EndpointScope; @@ -356,7 +357,7 @@ pub async fn publish(app: AppState, req: Parts, body: Body) -> AppResult { } impl Team { - /// Tries to create the Team in the DB (assumes a `:` has already been found). - /// - /// # Panics - /// - /// This function will panic if login contains less than 2 `:` characters. - pub async fn create_or_update( - gh_client: &dyn GitHubClient, - conn: &mut AsyncPgConnection, - login: &str, - req_user: &User, - ) -> AppResult { - // must look like system:xxxxxxx - let mut chunks = login.split(':'); - // unwrap is okay, split on an empty string still has 1 chunk - match chunks.next().unwrap() { - // github:rust-lang:owners - "github" => { - // unwrap is documented above as part of the calling contract - let org = chunks.next().unwrap(); - let team = chunks.next().ok_or_else(|| { - bad_request( - "missing github team argument; \ - format is github:org:team", - ) - })?; - Team::create_or_update_github_team( - gh_client, - conn, - &login.to_lowercase(), - org, - team, - req_user, - ) - .await - } - _ => Err(bad_request( - "unknown organization handler, \ - only 'github:org:team' is supported", - )), - } - } - - /// Tries to create or update a Github Team. Assumes `org` and `team` are - /// correctly parsed out of the full `name`. `name` is passed as a - /// convenience to avoid rebuilding it. - async fn create_or_update_github_team( - gh_client: &dyn GitHubClient, - conn: &mut AsyncPgConnection, - login: &str, - org_name: &str, - team_name: &str, - req_user: &User, - ) -> AppResult { - // GET orgs/:org/teams - // check that `team` is the `slug` in results, and grab its data - - // "sanitization" - fn is_allowed_char(c: char) -> bool { - matches!(c, 'a'..='z' | 'A'..='Z' | '0'..='9' | '-' | '_') - } - - if let Some(c) = org_name.chars().find(|c| !is_allowed_char(*c)) { - return Err(bad_request(format_args!( - "organization cannot contain special \ - characters like {c}" - ))); - } - - let token = AccessToken::new(req_user.gh_access_token.expose_secret().to_string()); - let team = gh_client.team_by_name(org_name, team_name, &token).await - .map_err(|_| { - bad_request(format_args!( - "could not find the github team {org_name}/{team_name}. \ - Make sure that you have the right permissions in GitHub. \ - See https://doc.rust-lang.org/cargo/reference/publishing.html#github-permissions" - )) - })?; - - let org_id = team.organization.id; - - if !can_add_team(gh_client, org_id, team.id, &req_user.gh_login, &token).await? { - return Err(custom( - StatusCode::FORBIDDEN, - "only members of a team or organization owners can add it as an owner", - )); - } - - let org = gh_client.org_by_name(org_name, &token).await?; - - NewTeam::builder() - .login(&login.to_lowercase()) - .org_id(org_id) - .github_id(team.id) - .maybe_name(team.name.as_deref()) - .maybe_avatar(org.avatar_url.as_deref()) - .build() - .create_or_update(conn) - .await - .map_err(Into::into) - } - - /// Phones home to Github to ask if this User is a member of the given team. - /// Note that we're assuming that the given user is the one interested in - /// the answer. If this is not the case, then we could accidentally leak - /// private membership information here. - pub async fn contains_user( - &self, - gh_client: &dyn GitHubClient, - gh_login: &str, - token: &AccessToken, - ) -> Result { - team_with_gh_id_contains_user(gh_client, self.org_id, self.github_id, gh_login, token).await - } - pub async fn owning(krate: &Crate, conn: &mut AsyncPgConnection) -> QueryResult> { let base_query = CrateOwner::belonging_to(krate).filter(crate_owners::deleted.eq(false)); let teams = base_query @@ -186,45 +65,3 @@ impl Team { Ok(teams.collect()) } } - -async fn can_add_team( - gh_client: &dyn GitHubClient, - org_id: i32, - team_id: i32, - gh_login: &str, - token: &AccessToken, -) -> Result { - Ok( - team_with_gh_id_contains_user(gh_client, org_id, team_id, gh_login, token).await? - || is_gh_org_owner(gh_client, org_id, gh_login, token).await?, - ) -} - -async fn is_gh_org_owner( - gh_client: &dyn GitHubClient, - org_id: i32, - gh_login: &str, - token: &AccessToken, -) -> Result { - let membership = gh_client.org_membership(org_id, gh_login, token).await?; - Ok(membership.is_some_and(|m| m.is_active_admin())) -} - -async fn team_with_gh_id_contains_user( - gh_client: &dyn GitHubClient, - github_org_id: i32, - github_team_id: i32, - gh_login: &str, - token: &AccessToken, -) -> Result { - // GET /organizations/:org_id/team/:team_id/memberships/:username - // check that "state": "active" - - let membership = gh_client - .team_membership(github_org_id, github_team_id, gh_login, token) - .await?; - - // There is also `state: pending` for which we could possibly give - // some feedback, but it's not obvious how that should work. - Ok(membership.is_some_and(|m| m.is_active())) -} diff --git a/src/models/user.rs b/src/models/user.rs index 1d3932b6a63..bfb844d6d5d 100644 --- a/src/models/user.rs +++ b/src/models/user.rs @@ -5,13 +5,11 @@ use diesel::prelude::*; use diesel::sql_types::Integer; use diesel::upsert::excluded; use diesel_async::{AsyncPgConnection, RunQueryDsl}; -use oauth2::AccessToken; -use secrecy::{ExposeSecret, SecretString}; +use secrecy::SecretString; -use crate::models::{Crate, CrateOwner, Email, Owner, OwnerKind, Rights}; +use crate::models::{Crate, CrateOwner, Email, Owner, OwnerKind}; use crate::schema::{crate_owners, emails, users}; use crates_io_diesel_helpers::lower; -use crates_io_github::{GitHubClient, GitHubError}; /// The model representing a row in the `users` database table. #[derive(Clone, Debug, Queryable, Identifiable, Selectable)] @@ -56,40 +54,6 @@ impl User { Ok(users.collect()) } - /// Given this set of owners, determines the strongest rights the - /// user has. - /// - /// Shortcircuits on `Full` because you can't beat it. In practice we'll always - /// see `[user, user, user, ..., team, team, team]`, so we could shortcircuit on - /// `Publish` as well, but this is a non-obvious invariant so we don't bother. - /// Sweet free optimization if teams are proving burdensome to check. - /// More than one team isn't really expected, though. - pub async fn rights( - &self, - gh_client: &dyn GitHubClient, - owners: &[Owner], - ) -> Result { - let token = AccessToken::new(self.gh_access_token.expose_secret().to_string()); - - let mut best = Rights::None; - for owner in owners { - match *owner { - Owner::User(ref other_user) => { - if other_user.id == self.id { - return Ok(Rights::Full); - } - } - Owner::Team(ref team) => { - let gh_login = &self.gh_login; - if team.contains_user(gh_client, gh_login, &token).await? { - best = Rights::Publish; - } - } - } - } - Ok(best) - } - /// Queries the database for the verified emails /// belonging to a given user pub async fn verified_email(