-
Notifications
You must be signed in to change notification settings - Fork 678
Add trustpub_only flag
#12365
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
base: main
Are you sure you want to change the base?
Add trustpub_only flag
#12365
Changes from all commits
d0f2832
30e0c97
4929fb4
0b9faa7
e93afe2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| ALTER TABLE crates DROP COLUMN trustpub_only; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| ALTER TABLE crates ADD COLUMN trustpub_only BOOLEAN NOT NULL DEFAULT FALSE; | ||
| COMMENT ON COLUMN crates.trustpub_only IS 'When true, this crate can only be published via Trusted Publishing, not with API tokens'; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,213 @@ | ||
| use crate::app::AppState; | ||
| use crate::auth::AuthCheck; | ||
| use crate::controllers::krate::CratePath; | ||
| use crate::email::EmailMessage; | ||
| use crate::middleware::real_ip::RealIp; | ||
| use crate::models::token::EndpointScope; | ||
| use crate::models::{Crate, User}; | ||
| use crate::schema::*; | ||
| use crate::util::errors::{AppResult, crate_not_found, custom, forbidden}; | ||
| use crate::views::EncodableCrate; | ||
| use anyhow::Context; | ||
| use axum::{Extension, Json}; | ||
| use diesel::prelude::*; | ||
| use diesel_async::scoped_futures::ScopedFutureExt; | ||
| use diesel_async::{AsyncConnection, RunQueryDsl}; | ||
| use http::{StatusCode, request::Parts}; | ||
| use serde::{Deserialize, Serialize}; | ||
| use tracing::{info, warn}; | ||
|
|
||
| #[derive(Debug, Deserialize, utoipa::ToSchema)] | ||
| pub struct PatchRequest { | ||
| /// Whether this crate can only be published via Trusted Publishing. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub trustpub_only: Option<bool>, | ||
| } | ||
|
|
||
| #[derive(Debug, Serialize, utoipa::ToSchema)] | ||
| pub struct PatchResponse { | ||
| /// The updated crate metadata. | ||
| #[serde(rename = "crate")] | ||
| krate: EncodableCrate, | ||
| } | ||
|
|
||
| /// Update crate settings. | ||
| #[utoipa::path( | ||
| patch, | ||
| path = "/api/v1/crates/{name}", | ||
| params(CratePath), | ||
| request_body = PatchRequest, | ||
| security( | ||
| ("api_token" = []), | ||
| ("cookie" = []), | ||
| ), | ||
| tag = "crates", | ||
| responses((status = 200, description = "Successful Response", body = inline(PatchResponse))), | ||
| )] | ||
| pub async fn update_crate( | ||
| app: AppState, | ||
| path: CratePath, | ||
| req: Parts, | ||
| Extension(real_ip): Extension<RealIp>, | ||
| Json(body): Json<PatchRequest>, | ||
| ) -> AppResult<Json<PatchResponse>> { | ||
| let mut conn = app.db_write().await?; | ||
|
|
||
| // Check that the crate exists | ||
| let krate = path.load_crate(&mut conn).await?; | ||
|
|
||
| // Check that the user is authenticated with appropriate permissions | ||
| let auth = AuthCheck::default() | ||
| .with_endpoint_scope(EndpointScope::TrustedPublishing) | ||
| .for_crate(&krate.name) | ||
| .check(&req, &mut conn) | ||
| .await?; | ||
|
|
||
| if auth | ||
| .api_token() | ||
| .is_some_and(|token| token.endpoint_scopes.is_none()) | ||
| { | ||
| return Err(forbidden( | ||
| "This endpoint cannot be used with legacy API tokens. Use a scoped API token instead.", | ||
| )); | ||
| } | ||
|
|
||
| // Update crate settings in a transaction | ||
| conn.transaction(|conn| { | ||
| update_inner(conn, &app, &krate, auth.user(), &real_ip, body).scope_boxed() | ||
| }) | ||
| .await | ||
| } | ||
|
|
||
| async fn update_inner( | ||
| conn: &mut diesel_async::AsyncPgConnection, | ||
| app: &AppState, | ||
| krate: &Crate, | ||
| user: &User, | ||
| real_ip: &RealIp, | ||
| body: PatchRequest, | ||
| ) -> AppResult<Json<PatchResponse>> { | ||
| // Query user owners to check permissions and send emails | ||
| let user_owners = crate_owners::table | ||
| .inner_join(users::table) | ||
| .inner_join(emails::table.on(users::id.eq(emails::user_id))) | ||
| .filter(crate_owners::crate_id.eq(krate.id)) | ||
| .filter(crate_owners::deleted.eq(false)) | ||
| .filter(crate_owners::owner_kind.eq(crate::models::OwnerKind::User)) | ||
| .select((users::id, users::gh_login, emails::email, emails::verified)) | ||
| .load::<(i32, String, String, bool)>(conn) | ||
| .await?; | ||
|
|
||
| // Check that the authenticated user is an owner | ||
| if !user_owners.iter().any(|(id, _, _, _)| *id == user.id) { | ||
| let msg = "only owners have permission to modify crate settings"; | ||
| return Err(custom(StatusCode::FORBIDDEN, msg)); | ||
| } | ||
|
|
||
| // Update trustpub_only if provided | ||
| if let Some(trustpub_only) = body.trustpub_only { | ||
| diesel::update(crates::table) | ||
| .filter(crates::id.eq(krate.id)) | ||
| .set(crates::trustpub_only.eq(trustpub_only)) | ||
| .execute(conn) | ||
| .await?; | ||
|
|
||
| // Audit log the setting change | ||
| info!( | ||
| target: "audit", | ||
| action = "trustpub_only_change", | ||
| krate.name = %krate.name, | ||
| network.client.ip = %**real_ip, | ||
| usr.id = user.id, | ||
| usr.name = %user.gh_login, | ||
| "User {} set trustpub_only={trustpub_only} for crate {}", | ||
| user.gh_login, | ||
| krate.name | ||
| ); | ||
|
|
||
| // Send email notifications to all crate owners | ||
| for (_, gh_login, email_address, email_verified) in &user_owners { | ||
| if *email_verified { | ||
| let email = TrustpubOnlyChangedEmail { | ||
| recipient: gh_login, | ||
| auth_user: user, | ||
| krate, | ||
| trustpub_only, | ||
| }; | ||
|
|
||
| if let Err(err) = email.send(app, email_address).await { | ||
| warn!("Failed to send trustpub_only notification to {email_address}: {err}"); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+107
to
+143
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a blocker, but I'm wondering if we should check whether the given value is different from the existing value since we are sending a notification? Nevertheless, I especially love the introduced notification mechanism here. Without it, a malicious user with a leaked API token could change the setting and publish a malicious version, which the crate owner might not notice. But with this mechanism, the crate owner has a chance to rotate the token and make amends! |
||
|
|
||
| // Reload the crate to get updated data | ||
| let (krate, downloads, recent_downloads, default_version, yanked, num_versions): ( | ||
| Crate, | ||
| i64, | ||
| Option<i64>, | ||
| Option<String>, | ||
| Option<bool>, | ||
| Option<i32>, | ||
| ) = Crate::by_name(&krate.name) | ||
| .inner_join(crate_downloads::table) | ||
| .left_join(recent_crate_downloads::table) | ||
| .left_join(default_versions::table) | ||
| .left_join(versions::table.on(default_versions::version_id.eq(versions::id))) | ||
| .select(( | ||
| Crate::as_select(), | ||
| crate_downloads::downloads, | ||
| recent_crate_downloads::downloads.nullable(), | ||
| versions::num.nullable(), | ||
| versions::yanked.nullable(), | ||
| default_versions::num_versions.nullable(), | ||
| )) | ||
| .first(conn) | ||
| .await | ||
| .optional()? | ||
| .ok_or_else(|| crate_not_found(&krate.name))?; | ||
|
|
||
| let encodable_crate = EncodableCrate::from( | ||
| krate, | ||
| default_version.as_deref(), | ||
| num_versions.unwrap_or_default(), | ||
| yanked, | ||
| None, | ||
| None, | ||
| None, | ||
| None, | ||
| false, | ||
| downloads, | ||
| recent_downloads, | ||
| ); | ||
|
|
||
| Ok(Json(PatchResponse { | ||
| krate: encodable_crate, | ||
| })) | ||
| } | ||
|
|
||
| #[derive(Serialize)] | ||
| struct TrustpubOnlyChangedEmail<'a> { | ||
| /// The GitHub login of the email recipient. | ||
| recipient: &'a str, | ||
| /// The user who changed the setting. | ||
| auth_user: &'a User, | ||
| /// The crate for which the setting was changed. | ||
| krate: &'a Crate, | ||
| /// The new value of the trustpub_only flag. | ||
| trustpub_only: bool, | ||
| } | ||
|
|
||
| impl TrustpubOnlyChangedEmail<'_> { | ||
| async fn send(&self, state: &AppState, email_address: &str) -> anyhow::Result<()> { | ||
| let email = EmailMessage::from_template("trustpub_only_changed", self); | ||
| let email = email.context("Failed to render email template")?; | ||
|
|
||
| state | ||
| .emails | ||
| .send(email_address, email) | ||
| .await | ||
| .context("Failed to send email") | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -79,6 +79,7 @@ mod tests { | |
| repository: None, | ||
| max_upload_size: None, | ||
| max_features: None, | ||
| trustpub_only: false, | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think people may be confused by "API token" as a term of art, so how about:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm... I specifically wanted to include the "API token" term to make it more obvious to the user what the problem is. I'm not sure what exactly you mean by "term of art". Do you think that crate owners that try to publish something are not aware of what the term "API token" means? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, yes, especially in a scenario where this might be an error message they get some time after they configured their crate to require TP. I could see a crate owner — particularly one that's stressed because they're trying to publish a fix for a vulnerability or critical bug — throwing their hands up and wondering how a TP token differs from an API token.
I don't mind including it somewhere in there in a second sentence, but I think we need to frontload the actionable part, which is that Trusted Publishing is required.