From 598dad5d7e29607e095b97c9fe81d98205c0a6d7 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Thu, 13 Feb 2025 09:38:25 +0100 Subject: [PATCH] models/team: Replace `App` arguments with `GitHubClient` We don't need the full-blown `App` struct. All we need is a way to talk to the GitHub API. --- src/controllers/crate_owner_invitation.rs | 2 +- src/controllers/krate/delete.rs | 2 +- src/controllers/krate/owners.rs | 9 ++-- src/controllers/krate/publish.rs | 2 +- src/controllers/version/update.rs | 2 +- src/models/team.rs | 50 ++++++++++++++--------- src/models/user.rs | 10 +++-- 7 files changed, 47 insertions(+), 30 deletions(-) diff --git a/src/controllers/crate_owner_invitation.rs b/src/controllers/crate_owner_invitation.rs index 8145853567c..8e83c6abe55 100644 --- a/src/controllers/crate_owner_invitation.rs +++ b/src/controllers/crate_owner_invitation.rs @@ -157,7 +157,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, &owners).await? != Rights::Full { + if user.rights(&*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/krate/delete.rs b/src/controllers/krate/delete.rs index d465ad0270e..16ebaa2c707 100644 --- a/src/controllers/krate/delete.rs +++ b/src/controllers/krate/delete.rs @@ -68,7 +68,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, &owners).await? { + match user.rights(&*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 455bfeaeeb2..854af54ef1c 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -16,6 +16,7 @@ use axum_extra::json; use axum_extra::response::ErasedJson; use chrono::Utc; use crates_io_database::schema::crate_owners; +use crates_io_github::GitHubClient; use diesel::prelude::*; use diesel_async::scoped_futures::ScopedFutureExt; use diesel_async::{AsyncConnection, AsyncPgConnection, RunQueryDsl}; @@ -176,7 +177,7 @@ async fn modify_owners( let owners = krate.owners(conn).await?; - match user.rights(&app, &owners).await? { + match user.rights(&*app.github, &owners).await? { Rights::Full => {} // Yes! Rights::Publish => { @@ -292,7 +293,7 @@ async fn add_owner( login: &str, ) -> Result { if login.contains(':') { - add_team_owner(app, conn, req_user, krate, login).await + add_team_owner(&*app.github, conn, req_user, krate, login).await } else { invite_user_owner(app, conn, req_user, krate, login).await } @@ -330,14 +331,14 @@ async fn invite_user_owner( } async fn add_team_owner( - app: &App, + gh_client: &dyn GitHubClient, conn: &mut AsyncPgConnection, req_user: &User, krate: &Crate, login: &str, ) -> Result { // Always recreate teams to get the most up-to-date GitHub ID - let team = Team::create_or_update(app, conn, login, req_user).await?; + let team = Team::create_or_update(gh_client, conn, login, req_user).await?; // Teams are added as owners immediately, since the above call ensures // the user is a team member. diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index fac50ebc932..0decc4d9b6b 100644 --- a/src/controllers/krate/publish.rs +++ b/src/controllers/krate/publish.rs @@ -356,7 +356,7 @@ pub async fn publish(app: AppState, req: Parts, body: Body) -> AppResult AppResult { + pub async fn contains_user( + &self, + gh_client: &dyn GitHubClient, + user: &User, + ) -> AppResult { match self.org_id { - Some(org_id) => team_with_gh_id_contains_user(app, org_id, self.github_id, user).await, + Some(org_id) => { + team_with_gh_id_contains_user(gh_client, org_id, self.github_id, user).await + } // This means we don't have an org_id on file for the `self` team. It much // probably was deleted from github by the time we backfilled the database. // Short-circuiting to false since a non-existent team cannot contain any @@ -189,17 +194,25 @@ impl Team { } } -async fn can_add_team(app: &App, org_id: i32, team_id: i32, user: &User) -> AppResult { +async fn can_add_team( + gh_client: &dyn GitHubClient, + org_id: i32, + team_id: i32, + user: &User, +) -> AppResult { Ok( - team_with_gh_id_contains_user(app, org_id, team_id, user).await? - || is_gh_org_owner(app, org_id, user).await?, + team_with_gh_id_contains_user(gh_client, org_id, team_id, user).await? + || is_gh_org_owner(gh_client, org_id, user).await?, ) } -async fn is_gh_org_owner(app: &App, org_id: i32, user: &User) -> AppResult { +async fn is_gh_org_owner( + gh_client: &dyn GitHubClient, + org_id: i32, + user: &User, +) -> AppResult { let token = AccessToken::new(user.gh_access_token.clone()); - match app - .github + match gh_client .org_membership(org_id, &user.gh_login, &token) .await { @@ -210,7 +223,7 @@ async fn is_gh_org_owner(app: &App, org_id: i32, user: &User) -> AppResult } async fn team_with_gh_id_contains_user( - app: &App, + gh_client: &dyn GitHubClient, github_org_id: i32, github_team_id: i32, user: &User, @@ -219,8 +232,7 @@ async fn team_with_gh_id_contains_user( // check that "state": "active" let token = AccessToken::new(user.gh_access_token.clone()); - let membership = match app - .github + let membership = match gh_client .team_membership(github_org_id, github_team_id, &user.gh_login, &token) .await { diff --git a/src/models/user.rs b/src/models/user.rs index 87f5dc097b6..4d44c2fed4f 100644 --- a/src/models/user.rs +++ b/src/models/user.rs @@ -6,12 +6,12 @@ use diesel::sql_types::Integer; use diesel::upsert::excluded; use diesel_async::{AsyncPgConnection, RunQueryDsl}; -use crate::app::App; use crate::util::errors::AppResult; use crate::models::{Crate, CrateOwner, Email, Owner, OwnerKind, Rights}; use crate::schema::{crate_owners, emails, users}; use crates_io_diesel_helpers::lower; +use crates_io_github::GitHubClient; /// The model representing a row in the `users` database table. #[derive(Clone, Debug, PartialEq, Eq, Queryable, Identifiable, AsChangeset, Selectable)] @@ -63,7 +63,11 @@ impl User { /// `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, app: &App, owners: &[Owner]) -> AppResult { + pub async fn rights( + &self, + gh_client: &dyn GitHubClient, + owners: &[Owner], + ) -> AppResult { let mut best = Rights::None; for owner in owners { match *owner { @@ -73,7 +77,7 @@ impl User { } } Owner::Team(ref team) => { - if team.contains_user(app, self).await? { + if team.contains_user(gh_client, self).await? { best = Rights::Publish; } }