diff --git a/crates/crates_io_github/src/lib.rs b/crates/crates_io_github/src/lib.rs index 56ad244fc26..eb7c05a5b4d 100644 --- a/crates/crates_io_github/src/lib.rs +++ b/crates/crates_io_github/src/lib.rs @@ -33,13 +33,13 @@ pub trait GitHubClient: Send + Sync { team_id: i32, username: &str, auth: &AccessToken, - ) -> Result; + ) -> Result>; async fn org_membership( &self, org_id: i32, username: &str, auth: &AccessToken, - ) -> Result; + ) -> Result>; async fn public_keys(&self, username: &str, password: &str) -> Result>; } @@ -120,9 +120,14 @@ impl GitHubClient for RealGitHubClient { team_id: i32, username: &str, auth: &AccessToken, - ) -> Result { + ) -> Result> { let url = format!("/organizations/{org_id}/team/{team_id}/memberships/{username}"); - self.request(&url, auth).await + match self.request(&url, auth).await { + Ok(membership) => Ok(Some(membership)), + // Officially how `false` is returned + Err(GitHubError::NotFound(_)) => Ok(None), + Err(err) => Err(err), + } } async fn org_membership( @@ -130,12 +135,13 @@ impl GitHubClient for RealGitHubClient { org_id: i32, username: &str, auth: &AccessToken, - ) -> Result { - self.request( - &format!("/organizations/{org_id}/memberships/{username}"), - auth, - ) - .await + ) -> Result> { + let url = format!("/organizations/{org_id}/memberships/{username}"); + match self.request(&url, auth).await { + Ok(membership) => Ok(Some(membership)), + Err(GitHubError::NotFound(_)) => Ok(None), + Err(err) => Err(err), + } } /// Returns the list of public keys that can be used to verify GitHub secret alert signatures diff --git a/src/models/team.rs b/src/models/team.rs index af72806a9e2..c843c4e189a 100644 --- a/src/models/team.rs +++ b/src/models/team.rs @@ -213,14 +213,11 @@ async fn is_gh_org_owner( user: &User, ) -> Result { let token = AccessToken::new(user.gh_access_token.expose_secret().to_string()); - match gh_client + let membership = gh_client .org_membership(org_id, &user.gh_login, &token) - .await - { - Ok(membership) => Ok(membership.state == "active" && membership.role == "admin"), - Err(GitHubError::NotFound(_)) => Ok(false), - Err(e) => Err(e), - } + .await?; + + Ok(membership.is_some_and(|m| m.state == "active" && m.role == "admin")) } async fn team_with_gh_id_contains_user( @@ -233,16 +230,11 @@ async fn team_with_gh_id_contains_user( // check that "state": "active" let token = AccessToken::new(user.gh_access_token.expose_secret().to_string()); - let membership = match gh_client + let membership = gh_client .team_membership(github_org_id, github_team_id, &user.gh_login, &token) - .await - { - // Officially how `false` is returned - Err(GitHubError::NotFound(_)) => return Ok(false), - x => x?, - }; + .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.state == "active") + Ok(membership.is_some_and(|m| m.state == "active")) } diff --git a/src/tests/issues/issue1205.rs b/src/tests/issues/issue1205.rs index 6038ac1fffb..1229746dc0e 100644 --- a/src/tests/issues/issue1205.rs +++ b/src/tests/issues/issue1205.rs @@ -74,7 +74,7 @@ fn github_mock() -> MockGitHubClient { github_mock .expect_team_membership() .with(eq(1), eq(2), eq("foo"), always()) - .returning(|_, _, _, _| Ok(active_membership())); + .returning(|_, _, _, _| Ok(Some(active_membership()))); github_mock } diff --git a/src/tests/routes/crates/owners/remove.rs b/src/tests/routes/crates/owners/remove.rs index f667a704d68..669248aef07 100644 --- a/src/tests/routes/crates/owners/remove.rs +++ b/src/tests/routes/crates/owners/remove.rs @@ -138,9 +138,9 @@ async fn test_remove_uppercase_team() { .expect_team_membership() .with(eq(1), eq(2), eq("foo"), always()) .returning(|_, _, _, _| { - Ok(GitHubTeamMembership { + Ok(Some(GitHubTeamMembership { state: "active".to_string(), - }) + })) }); let (app, _, cookie) = TestApp::full().with_github(github_mock).with_user().await; diff --git a/src/tests/util/github.rs b/src/tests/util/github.rs index 58d3d45a1a7..6f6667f68e8 100644 --- a/src/tests/util/github.rs +++ b/src/tests/util/github.rs @@ -120,7 +120,7 @@ impl MockData { org_id: i32, team_id: i32, username: &str, - ) -> Result { + ) -> Result, GitHubError> { let team = self .orgs .iter() @@ -131,11 +131,11 @@ impl MockData { .find(|team| team.id == team_id) .ok_or_else(not_found)?; if team.members.contains(&username) { - Ok(GitHubTeamMembership { + Ok(Some(GitHubTeamMembership { state: "active".into(), - }) + })) } else { - Err(not_found()) + Ok(None) } } @@ -143,28 +143,28 @@ impl MockData { &self, org_id: i32, username: &str, - ) -> Result { + ) -> Result, GitHubError> { let org = self .orgs .iter() .find(|org| org.id == org_id) .ok_or_else(not_found)?; if org.owners.contains(&username) { - Ok(GitHubOrgMembership { + Ok(Some(GitHubOrgMembership { state: "active".into(), role: "admin".into(), - }) + })) } else if org .teams .iter() .any(|team| team.members.contains(&username)) { - Ok(GitHubOrgMembership { + Ok(Some(GitHubOrgMembership { state: "active".into(), role: "member".into(), - }) + })) } else { - Err(not_found()) + Ok(None) } } }