From de0b0c7d7fb5a2764dad2e5ee19181c3c749e139 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Thu, 13 Feb 2025 11:40:11 +0100 Subject: [PATCH 1/8] models/team: Inline `Team::create_or_update()` fn This fn is not doing anything database-related, so it shouldn't live in the `models` module. --- src/controllers/krate/owners.rs | 31 ++++++++++++++++++++++- src/models/team.rs | 44 +-------------------------------- 2 files changed, 31 insertions(+), 44 deletions(-) diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index 74e92b5c919..3cbb2c0e172 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -337,7 +337,36 @@ async fn add_team_owner( login: &str, ) -> Result { // 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 mut chunks = login.split(':'); + let team = 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? + } + _ => { + return Err(bad_request( + "unknown organization handler, \ + only 'github:org:team' is supported", + ) + .into()); + } + }; // Teams are added as owners immediately, since the above call ensures // the user is a team member. diff --git a/src/models/team.rs b/src/models/team.rs index 0a25e666fbe..1e4a1127cef 100644 --- a/src/models/team.rs +++ b/src/models/team.rs @@ -58,52 +58,10 @@ impl NewTeam<'_> { } 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( + pub async fn create_or_update_github_team( gh_client: &dyn GitHubClient, conn: &mut AsyncPgConnection, login: &str, From 8f7c4c5e450822a849297aaca9d0ce9a43c09328 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Thu, 13 Feb 2025 11:43:39 +0100 Subject: [PATCH 2/8] controllers/krate/owners: Simplify `add_team_owner()` fn` --- src/controllers/krate/owners.rs | 55 +++++++++++++++------------------ 1 file changed, 25 insertions(+), 30 deletions(-) diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index 3cbb2c0e172..d364f2e3c67 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -336,37 +336,32 @@ async fn add_team_owner( krate: &Crate, login: &str, ) -> Result { - // Always recreate teams to get the most up-to-date GitHub ID + // github:rust-lang:owners let mut chunks = login.split(':'); - let team = 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? - } - _ => { - return Err(bad_request( - "unknown organization handler, \ - only 'github:org:team' is supported", - ) - .into()); - } - }; + + 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_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. From 100bcd9f47786701bb32afc2ec8281962415bdf3 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Thu, 13 Feb 2025 11:47:38 +0100 Subject: [PATCH 3/8] models/team: Move `Team::create_or_update_github_team()` fn into controller This fn is doing way to much non-database-related stuff, so it shouldn't live in the `models` module. --- src/controllers/krate/owners.rs | 76 ++++++++++++++++++++++++++++----- src/models.rs | 2 +- src/models/team.rs | 67 +---------------------------- 3 files changed, 69 insertions(+), 76 deletions(-) diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index d364f2e3c67..0be658a0879 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -2,9 +2,10 @@ use crate::controllers::krate::CratePath; use crate::models::krate::OwnerRemoveError; +use crate::models::team::can_add_team; use crate::models::{ krate::NewOwnerInvite, token::EndpointScope, CrateOwner, NewCrateOwnerInvitation, - NewCrateOwnerInvitationOutcome, + NewCrateOwnerInvitationOutcome, NewTeam, }; use crate::models::{Crate, Owner, Rights, Team, User}; use crate::util::errors::{bad_request, crate_not_found, custom, AppResult, BoxedAppError}; @@ -21,6 +22,7 @@ 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; @@ -353,15 +355,9 @@ async fn add_team_owner( })?; // Always recreate teams to get the most up-to-date GitHub ID - let team = Team::create_or_update_github_team( - gh_client, - conn, - &login.to_lowercase(), - org, - team, - 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. @@ -376,6 +372,66 @@ 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; + + if !can_add_team(gh_client, org_id, team.id, 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) +} + /// Error results from a [`add_owner()`] model call. #[derive(Debug, Error)] enum OwnerAddError { diff --git a/src/models.rs b/src/models.rs index fdb718e8ce8..5740ed8932f 100644 --- a/src/models.rs +++ b/src/models.rs @@ -33,7 +33,7 @@ mod keyword; pub mod krate; mod owner; mod rights; -mod team; +pub mod team; pub mod token; pub mod user; pub mod version; diff --git a/src/models/team.rs b/src/models/team.rs index 1e4a1127cef..0ee9ab21f37 100644 --- a/src/models/team.rs +++ b/src/models/team.rs @@ -1,15 +1,11 @@ use bon::Builder; use diesel::prelude::*; use diesel_async::{AsyncPgConnection, RunQueryDsl}; -use http::StatusCode; - -use crate::util::errors::{bad_request, custom, AppResult}; use crates_io_github::{GitHubClient, GitHubError}; use oauth2::AccessToken; -use secrecy::ExposeSecret; -use crate::models::{Crate, CrateOwner, Owner, OwnerKind, User}; +use crate::models::{Crate, CrateOwner, Owner, OwnerKind}; use crate::schema::{crate_owners, teams}; /// For now, just a Github Team. Can be upgraded to other teams @@ -58,65 +54,6 @@ impl NewTeam<'_> { } impl 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; - - 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 @@ -145,7 +82,7 @@ impl Team { } } -async fn can_add_team( +pub async fn can_add_team( gh_client: &dyn GitHubClient, org_id: i32, team_id: i32, From eea1e465e63a6614f594f6b6b787151362a82551 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Thu, 13 Feb 2025 11:52:58 +0100 Subject: [PATCH 4/8] models/team: Inline `can_add_team()` fn --- src/controllers/krate/owners.rs | 8 ++++++-- src/models/team.rs | 17 ++--------------- 2 files changed, 8 insertions(+), 17 deletions(-) diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index 0be658a0879..3bc040965f2 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -2,7 +2,7 @@ use crate::controllers::krate::CratePath; use crate::models::krate::OwnerRemoveError; -use crate::models::team::can_add_team; +use crate::models::team::{is_gh_org_owner, team_with_gh_id_contains_user}; use crate::models::{ krate::NewOwnerInvite, token::EndpointScope, CrateOwner, NewCrateOwnerInvitation, NewCrateOwnerInvitationOutcome, NewTeam, @@ -411,7 +411,11 @@ pub async fn create_or_update_github_team( let org_id = team.organization.id; let gh_login = &req_user.gh_login; - if !can_add_team(gh_client, org_id, team.id, gh_login, &token).await? { + let can_add_team = 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?; + + if !can_add_team { return Err(custom( StatusCode::FORBIDDEN, "only members of a team or organization owners can add it as an owner", diff --git a/src/models/team.rs b/src/models/team.rs index 0ee9ab21f37..4165016c2c9 100644 --- a/src/models/team.rs +++ b/src/models/team.rs @@ -82,20 +82,7 @@ impl Team { } } -pub 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( +pub async fn is_gh_org_owner( gh_client: &dyn GitHubClient, org_id: i32, gh_login: &str, @@ -105,7 +92,7 @@ async fn is_gh_org_owner( Ok(membership.is_some_and(|m| m.is_active_admin())) } -async fn team_with_gh_id_contains_user( +pub async fn team_with_gh_id_contains_user( gh_client: &dyn GitHubClient, github_org_id: i32, github_team_id: i32, From d6fa6da5813736c4342aacdf20696f8f08ccd217 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Sat, 15 Feb 2025 17:07:23 +0100 Subject: [PATCH 5/8] models/team: Move `is_gh_org_owner()` fn into controller This fn is unrelated to the database, so it should not live in the `models` module. --- src/controllers/krate/owners.rs | 14 ++++++++++++-- src/models/team.rs | 10 ---------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index 3bc040965f2..c708697d4dc 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -2,7 +2,7 @@ use crate::controllers::krate::CratePath; use crate::models::krate::OwnerRemoveError; -use crate::models::team::{is_gh_org_owner, team_with_gh_id_contains_user}; +use crate::models::team::team_with_gh_id_contains_user; use crate::models::{ krate::NewOwnerInvite, token::EndpointScope, CrateOwner, NewCrateOwnerInvitation, NewCrateOwnerInvitationOutcome, NewTeam, @@ -16,7 +16,7 @@ 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}; @@ -436,6 +436,16 @@ pub async fn create_or_update_github_team( .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/models/team.rs b/src/models/team.rs index 4165016c2c9..7aedbec2688 100644 --- a/src/models/team.rs +++ b/src/models/team.rs @@ -82,16 +82,6 @@ impl Team { } } -pub 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())) -} - pub async fn team_with_gh_id_contains_user( gh_client: &dyn GitHubClient, github_org_id: i32, From 67d44ed87382c4bfabbbb6e9c28387ea94d03094 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Sat, 15 Feb 2025 17:09:22 +0100 Subject: [PATCH 6/8] models/team: Inline `team_with_gh_id_contains_user()` fn --- src/controllers/krate/owners.rs | 9 ++++++--- src/models/team.rs | 24 ++++-------------------- 2 files changed, 10 insertions(+), 23 deletions(-) diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index c708697d4dc..316de88ff48 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -2,7 +2,6 @@ use crate::controllers::krate::CratePath; use crate::models::krate::OwnerRemoveError; -use crate::models::team::team_with_gh_id_contains_user; use crate::models::{ krate::NewOwnerInvite, token::EndpointScope, CrateOwner, NewCrateOwnerInvitation, NewCrateOwnerInvitationOutcome, NewTeam, @@ -411,9 +410,13 @@ pub async fn create_or_update_github_team( let org_id = team.organization.id; let gh_login = &req_user.gh_login; - let can_add_team = team_with_gh_id_contains_user(gh_client, org_id, team.id, gh_login, &token) + let is_team_member = gh_client + .team_membership(org_id, team.id, gh_login, &token) .await? - || is_gh_org_owner(gh_client, org_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( diff --git a/src/models/team.rs b/src/models/team.rs index 7aedbec2688..54823285172 100644 --- a/src/models/team.rs +++ b/src/models/team.rs @@ -64,7 +64,10 @@ impl Team { gh_login: &str, token: &AccessToken, ) -> Result { - team_with_gh_id_contains_user(gh_client, self.org_id, self.github_id, gh_login, token).await + Ok(gh_client + .team_membership(self.org_id, self.github_id, gh_login, token) + .await? + .is_some_and(|m| m.is_active())) } pub async fn owning(krate: &Crate, conn: &mut AsyncPgConnection) -> QueryResult> { @@ -81,22 +84,3 @@ impl Team { Ok(teams.collect()) } } - -pub 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())) -} From 09656a3a5292b1c73ce9d4f20213feb7b8cca271 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Sat, 15 Feb 2025 17:11:00 +0100 Subject: [PATCH 7/8] models/team: Inline `Team::contains_user()` fn --- src/models/team.rs | 19 ------------------- src/models/user.rs | 12 ++++++++++-- 2 files changed, 10 insertions(+), 21 deletions(-) diff --git a/src/models/team.rs b/src/models/team.rs index 54823285172..a04d3e65c02 100644 --- a/src/models/team.rs +++ b/src/models/team.rs @@ -2,9 +2,6 @@ use bon::Builder; use diesel::prelude::*; use diesel_async::{AsyncPgConnection, RunQueryDsl}; -use crates_io_github::{GitHubClient, GitHubError}; -use oauth2::AccessToken; - use crate::models::{Crate, CrateOwner, Owner, OwnerKind}; use crate::schema::{crate_owners, teams}; @@ -54,22 +51,6 @@ impl NewTeam<'_> { } impl 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. - pub async fn contains_user( - &self, - gh_client: &dyn GitHubClient, - gh_login: &str, - token: &AccessToken, - ) -> Result { - Ok(gh_client - .team_membership(self.org_id, self.github_id, gh_login, token) - .await? - .is_some_and(|m| m.is_active())) - } - 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 diff --git a/src/models/user.rs b/src/models/user.rs index 1d3932b6a63..0283c333474 100644 --- a/src/models/user.rs +++ b/src/models/user.rs @@ -80,8 +80,16 @@ impl User { } } Owner::Team(ref team) => { - let gh_login = &self.gh_login; - if team.contains_user(gh_client, gh_login, &token).await? { + // 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, &self.gh_login, &token) + .await? + .is_some_and(|m| m.is_active()); + + if is_team_member { best = Rights::Publish; } } From bf4dca8e7197260d47bebd74c096c34eda9a13d4 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Sat, 15 Feb 2025 17:20:00 +0100 Subject: [PATCH 8/8] models/user: Move `User::rights()` fn into `controllers::helpers` module --- src/controllers/crate_owner_invitation.rs | 5 +- src/controllers/helpers.rs | 1 + src/controllers/helpers/authorization.rs | 57 +++++++++++++++++++++++ src/controllers/krate/delete.rs | 5 +- src/controllers/krate/owners.rs | 5 +- src/controllers/krate/publish.rs | 5 +- src/controllers/version/update.rs | 7 ++- src/models.rs | 2 - src/models/rights.rs | 8 ---- src/models/user.rs | 48 +------------------ 10 files changed, 75 insertions(+), 68 deletions(-) create mode 100644 src/controllers/helpers/authorization.rs delete mode 100644 src/models/rights.rs 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 316de88ff48..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, 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}; @@ -177,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 => { 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 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) => { - // 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, &self.gh_login, &token) - .await? - .is_some_and(|m| m.is_active()); - - if is_team_member { - best = Rights::Publish; - } - } - } - } - Ok(best) - } - /// Queries the database for the verified emails /// belonging to a given user pub async fn verified_email(