Merge pull request #10596 from Turbo87/models-deps

models: Remove remaining `GitHubClient` usage

Author: Turbo87

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));
                 }

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;

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<Self, GitHubError> {
+        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)
+    }
+}

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

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<NewOwnerInvite, OwnerAddError> {
+    // 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<Team> {
+    // 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<bool, GitHubError> {
+    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 {

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<Json<Go
         };
 
         let owners = krate.owners(conn).await?;
-        if user.rights(&*app.github, &owners).await? < Rights::Publish {
+        if Rights::get(user, &*app.github, &owners).await? < Rights::Publish {
             return Err(custom(StatusCode::FORBIDDEN, MISSING_RIGHTS_ERROR_MESSAGE));
         }
 

src/controllers/version/update.rs

@@ -1,10 +1,9 @@
 use super::CrateVersionPath;
 use crate::app::AppState;
 use crate::auth::{AuthCheck, Authentication};
+use crate::controllers::helpers::authorization::Rights;
 use crate::models::token::EndpointScope;
-use crate::models::{
-    Crate, NewVersionOwnerAction, Rights, Version, VersionAction, VersionOwnerAction,
-};
+use crate::models::{Crate, NewVersionOwnerAction, Version, VersionAction, VersionOwnerAction};
 use crate::rate_limiter::LimitedAction;
 use crate::schema::versions;
 use crate::util::errors::{bad_request, custom, AppResult};
@@ -122,7 +121,7 @@ pub async fn perform_version_yank_update(
 
     let yanked = yanked.unwrap_or(version.yanked);
 
-    if user.rights(&*state.github, &owners).await? < Rights::Publish {
+    if Rights::get(user, &*state.github, &owners).await? < Rights::Publish {
         if user.is_admin {
             let action = if yanked { "yanking" } else { "unyanking" };
             warn!(

src/models.rs

@@ -12,7 +12,6 @@ pub use self::follow::Follow;
 pub use self::keyword::{CrateKeyword, Keyword};
 pub use self::krate::{Crate, CrateName, NewCrate, RecentCrateDownloads};
 pub use self::owner::{CrateOwner, Owner, OwnerKind};
-pub use self::rights::Rights;
 pub use self::team::{NewTeam, Team};
 pub use self::token::ApiToken;
 pub use self::user::{NewUser, User};
@@ -32,8 +31,7 @@ mod follow;
 mod keyword;
 pub mod krate;
 mod owner;
-mod rights;
-mod team;
+pub mod team;
 pub mod token;
 pub mod user;
 pub mod version;