Skip to content
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

Add more aggressive rate limiting for publishing new crates #1681

Merged
merged 2 commits into from
May 30, 2019
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
2 changes: 1 addition & 1 deletion RustConfig
Original file line number Diff line number Diff line change
@@ -1 +1 @@
VERSION=1.32.0
VERSION=1.33.0
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
DROP TABLE publish_limit_buckets;
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
CREATE TABLE publish_limit_buckets(
user_id INTEGER PRIMARY KEY NOT NULL REFERENCES users,
tokens INTEGER NOT NULL,
last_refill TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP
);
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
DROP TABLE publish_rate_overrides;
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
CREATE TABLE publish_rate_overrides (
user_id INTEGER PRIMARY KEY REFERENCES users,
burst INTEGER NOT NULL
);
2 changes: 1 addition & 1 deletion src/bin/update-downloads.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ mod test {
name: "foo",
..Default::default()
}
.create_or_update(conn, None, user_id)
.create_or_update(conn, None, user_id, None)
.unwrap();
let version = NewVersion::new(
krate.id,
Expand Down
3 changes: 3 additions & 0 deletions src/config.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use crate::publish_rate_limit::PublishRateLimit;
use crate::{env, uploaders::Uploader, Env, Replica};
use std::path::PathBuf;
use url::Url;
Expand All @@ -16,6 +17,7 @@ pub struct Config {
pub max_unpack_size: u64,
pub mirror: Replica,
pub api_protocol: String,
pub publish_rate_limit: PublishRateLimit,
}

impl Default for Config {
Expand Down Expand Up @@ -132,6 +134,7 @@ impl Default for Config {
max_unpack_size: 512 * 1024 * 1024, // 512 MB max when decompressed
mirror,
api_protocol,
publish_rate_limit: Default::default(),
}
}
}
7 changes: 6 additions & 1 deletion src/controllers/krate/publish.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,12 @@ pub fn publish(req: &mut dyn Request) -> CargoResult<Response> {
};

let license_file = new_crate.license_file.as_ref().map(|s| &**s);
let krate = persist.create_or_update(&conn, license_file, user.id)?;
let krate = persist.create_or_update(
&conn,
license_file,
user.id,
Some(&app.config.publish_rate_limit),
)?;

let owners = krate.owners(&conn)?;
if user.rights(req.app(), &owners)? < Rights::Publish {
Expand Down
2 changes: 2 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,10 @@ pub mod email;
pub mod git;
pub mod github;
pub mod middleware;
mod publish_rate_limit;
pub mod render;
pub mod schema;
mod test_util;
pub mod uploaders;
pub mod util;

Expand Down
5 changes: 2 additions & 3 deletions src/models/category.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,12 +183,11 @@ impl<'a> NewCategory<'a> {
#[cfg(test)]
mod tests {
use super::*;
use crate::test_util::pg_connection_no_transaction;
use diesel::connection::SimpleConnection;

fn pg_connection() -> PgConnection {
let database_url =
dotenv::var("TEST_DATABASE_URL").expect("TEST_DATABASE_URL must be set to run tests");
let conn = PgConnection::establish(&database_url).unwrap();
let conn = pg_connection_no_transaction();
// These tests deadlock if run concurrently
conn.batch_execute("BEGIN; LOCK categories IN ACCESS EXCLUSIVE MODE")
.unwrap();
Expand Down
5 changes: 5 additions & 0 deletions src/models/krate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use crate::models::{
use crate::views::{EncodableCrate, EncodableCrateLinks};

use crate::models::helpers::with_count::*;
use crate::publish_rate_limit::PublishRateLimit;
use crate::schema::*;

/// Hosts in this list are known to not be hosting documentation,
Expand Down Expand Up @@ -105,6 +106,7 @@ impl<'a> NewCrate<'a> {
conn: &PgConnection,
license_file: Option<&'a str>,
uploader: i32,
rate_limit: Option<&PublishRateLimit>,
) -> CargoResult<Crate> {
use diesel::update;

Expand All @@ -115,6 +117,9 @@ impl<'a> NewCrate<'a> {
// To avoid race conditions, we try to insert
// first so we know whether to add an owner
if let Some(krate) = self.save_new_crate(conn, uploader)? {
if let Some(rate_limit) = rate_limit {
rate_limit.check_rate_limit(uploader, conn)?;
}
return Ok(krate);
}

Expand Down
2 changes: 1 addition & 1 deletion src/models/user.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ pub struct User {
pub gh_id: i32,
}

#[derive(Insertable, Debug)]
#[derive(Insertable, Debug, Default)]
#[table_name = "users"]
pub struct NewUser<'a> {
pub gh_id: i32,
Expand Down
Loading