Skip to content

models/team: Avoid repeated AccessToken instantiation #10589

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 14, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 15 additions & 15 deletions src/models/team.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ impl Team {

let org_id = team.organization.id;

if !can_add_team(gh_client, org_id, team.id, req_user).await? {
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",
Expand Down Expand Up @@ -166,11 +166,13 @@ impl Team {
pub async fn contains_user(
&self,
gh_client: &dyn GitHubClient,
user: &User,
gh_login: &str,
token: &AccessToken,
) -> Result<bool, GitHubError> {
match self.org_id {
Some(org_id) => {
team_with_gh_id_contains_user(gh_client, org_id, self.github_id, user).await
team_with_gh_id_contains_user(gh_client, org_id, self.github_id, gh_login, token)
.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.
Expand Down Expand Up @@ -199,39 +201,37 @@ async fn can_add_team(
gh_client: &dyn GitHubClient,
org_id: i32,
team_id: i32,
user: &User,
gh_login: &str,
token: &AccessToken,
) -> Result<bool, GitHubError> {
Ok(
team_with_gh_id_contains_user(gh_client, org_id, team_id, user).await?
|| is_gh_org_owner(gh_client, org_id, user).await?,
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,
user: &User,
gh_login: &str,
token: &AccessToken,
) -> Result<bool, GitHubError> {
let token = AccessToken::new(user.gh_access_token.expose_secret().to_string());
let membership = gh_client
.org_membership(org_id, &user.gh_login, &token)
.await?;

let membership = gh_client.org_membership(org_id, gh_login, token).await?;
Ok(membership.is_some_and(|m| m.state == "active" && m.role == "admin"))
}

async fn team_with_gh_id_contains_user(
gh_client: &dyn GitHubClient,
github_org_id: i32,
github_team_id: i32,
user: &User,
gh_login: &str,
token: &AccessToken,
) -> Result<bool, GitHubError> {
// GET /organizations/:org_id/team/:team_id/memberships/:username
// check that "state": "active"

let token = AccessToken::new(user.gh_access_token.expose_secret().to_string());
let membership = gh_client
.team_membership(github_org_id, github_team_id, &user.gh_login, &token)
.team_membership(github_org_id, github_team_id, gh_login, token)
.await?;

// There is also `state: pending` for which we could possibly give
Expand Down
8 changes: 6 additions & 2 deletions src/models/user.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ use diesel::prelude::*;
use diesel::sql_types::Integer;
use diesel::upsert::excluded;
use diesel_async::{AsyncPgConnection, RunQueryDsl};
use secrecy::SecretString;
use oauth2::AccessToken;
use secrecy::{ExposeSecret, SecretString};

use crate::models::{Crate, CrateOwner, Email, Owner, OwnerKind, Rights};
use crate::schema::{crate_owners, emails, users};
Expand Down Expand Up @@ -68,6 +69,8 @@ impl User {
gh_client: &dyn GitHubClient,
owners: &[Owner],
) -> Result<Rights, GitHubError> {
let token = AccessToken::new(self.gh_access_token.expose_secret().to_string());

let mut best = Rights::None;
for owner in owners {
match *owner {
Expand All @@ -77,7 +80,8 @@ impl User {
}
}
Owner::Team(ref team) => {
if team.contains_user(gh_client, self).await? {
let gh_login = &self.gh_login;
if team.contains_user(gh_client, gh_login, &token).await? {
best = Rights::Publish;
}
}
Expand Down