From 41ec48c12a66701b68dd9a79bfe9047b23875579 Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Thu, 20 Oct 2022 21:10:40 -0400 Subject: [PATCH 1/3] Create AdminUsers that are hardcoded to be me, Toby, or Justin --- src/controllers/util.rs | 6 ++++- src/models.rs | 2 +- src/models/user.rs | 50 ++++++++++++++++++++++++++++++++++++++++- src/tests/user.rs | 6 +++++ src/views.rs | 3 +++ 5 files changed, 64 insertions(+), 3 deletions(-) diff --git a/src/controllers/util.rs b/src/controllers/util.rs index ca0074c0bb1..447689c1a5f 100644 --- a/src/controllers/util.rs +++ b/src/controllers/util.rs @@ -4,7 +4,7 @@ use conduit_cookie::RequestSession; use super::prelude::*; use crate::middleware::log_request; -use crate::models::{ApiToken, User}; +use crate::models::{AdminUser, ApiToken, User}; use crate::util::errors::{ account_locked, forbidden, internal, AppError, AppResult, InsecurelyGeneratedTokenRevoked, }; @@ -28,6 +28,10 @@ impl AuthenticatedUser { self.user } + pub fn admin_user(self) -> AppResult { + AdminUser::new(&self.user) + } + /// Disallows token authenticated users pub fn forbid_api_token_auth(self) -> AppResult { if self.token_id.is_none() { diff --git a/src/models.rs b/src/models.rs index 261427a1465..d51d55f5578 100644 --- a/src/models.rs +++ b/src/models.rs @@ -12,7 +12,7 @@ pub use self::owner::{CrateOwner, Owner, OwnerKind}; pub use self::rights::Rights; pub use self::team::{NewTeam, Team}; pub use self::token::{ApiToken, CreatedApiToken}; -pub use self::user::{NewUser, User}; +pub use self::user::{AdminUser, NewUser, User}; pub use self::version::{NewVersion, TopVersions, Version}; pub mod helpers; diff --git a/src/models/user.rs b/src/models/user.rs index 56a6bc55a4c..0d8b06e5a0f 100644 --- a/src/models/user.rs +++ b/src/models/user.rs @@ -4,7 +4,7 @@ use std::borrow::Cow; use crate::app::App; use crate::email::Emails; -use crate::util::errors::AppResult; +use crate::util::errors::{forbidden, AppResult}; use crate::models::{ApiToken, Crate, CrateOwner, Email, NewEmail, Owner, OwnerKind, Rights}; use crate::schema::{crate_owners, emails, users}; @@ -177,4 +177,52 @@ impl User { .first(conn) .optional()?) } + + /// Attempt to turn this user into an AdminUser + pub fn admin(&self) -> AppResult { + AdminUser::new(self) + } +} + +pub struct AdminUser(User); + +impl AdminUser { + pub fn new(user: &User) -> AppResult { + match user.gh_login.as_str() { + "carols10cents" | "jtgeibel" | "Turbo87" => Ok(Self(user.clone())), + _ => Err(forbidden()), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn hardcoded_admins() { + let user = User { + id: 3, + gh_access_token: "arbitrary".into(), + gh_login: "literally_anything".into(), + name: None, + gh_avatar: None, + gh_id: 7, + account_lock_reason: None, + account_lock_until: None, + }; + assert!(user.admin().is_err()); + + let sneaky_user = User { + gh_login: "carols10cents_plus_extra_stuff".into(), + ..user + }; + assert!(sneaky_user.admin().is_err()); + + let real_real_real = User { + gh_login: "carols10cents".into(), + ..sneaky_user + }; + assert!(real_real_real.admin().is_ok()); + } } diff --git a/src/tests/user.rs b/src/tests/user.rs index 03c761501b8..259a753b848 100644 --- a/src/tests/user.rs +++ b/src/tests/user.rs @@ -117,6 +117,7 @@ fn me() { let user = app.db_new_user("foo"); let json = user.show_me(); + assert!(!json.user.admin); assert_eq!(json.owned_crates.len(), 0); app.db(|conn| { @@ -126,6 +127,11 @@ fn me() { let updated_json = user.show_me(); assert_eq!(updated_json.owned_crates.len(), 1); + + let admin_user = app.db_new_user("carols10cents"); + let admin_json = admin_user.show_me(); + + assert!(admin_json.user.admin); } #[test] diff --git a/src/views.rs b/src/views.rs index f8edce8f5ac..2ec8e6629d7 100644 --- a/src/views.rs +++ b/src/views.rs @@ -503,6 +503,7 @@ pub struct EncodablePrivateUser { pub email: Option, pub avatar: Option, pub url: Option, + pub admin: bool, } impl EncodablePrivateUser { @@ -513,6 +514,7 @@ impl EncodablePrivateUser { email_verified: bool, email_verification_sent: bool, ) -> Self { + let admin = user.admin().is_ok(); let User { id, name, @@ -531,6 +533,7 @@ impl EncodablePrivateUser { login: gh_login, name, url: Some(url), + admin, } } } From 2dccaa7d839c1995a734488ffc74e75e9baa8b1a Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Thu, 20 Oct 2022 21:22:25 -0400 Subject: [PATCH 2/3] Make an API for increasing a user's rate limit --- src/controllers.rs | 1 + src/controllers/admin.rs | 48 +++++++++++++ src/lib.rs | 2 +- src/models/user.rs | 10 +++ src/router.rs | 6 ++ src/tests/admin_actions.rs | 141 +++++++++++++++++++++++++++++++++++++ src/tests/all.rs | 1 + 7 files changed, 208 insertions(+), 1 deletion(-) create mode 100644 src/controllers/admin.rs create mode 100644 src/tests/admin_actions.rs diff --git a/src/controllers.rs b/src/controllers.rs index 316f4ba19cc..ee9f80bb3e6 100644 --- a/src/controllers.rs +++ b/src/controllers.rs @@ -76,6 +76,7 @@ mod prelude { pub mod helpers; mod util; +pub mod admin; pub mod category; pub mod crate_owner_invitation; pub mod keyword; diff --git a/src/controllers/admin.rs b/src/controllers/admin.rs new file mode 100644 index 00000000000..2f550a483ab --- /dev/null +++ b/src/controllers/admin.rs @@ -0,0 +1,48 @@ +use super::frontend_prelude::*; +use crate::{ + models::{AdminUser, User}, + schema::{publish_limit_buckets, publish_rate_overrides}, +}; +use diesel::dsl::*; + +#[derive(Deserialize)] +struct RateLimitIncrease { + email: String, + rate_limit: i32, +} + +/// Increases the rate limit for the user with the specified verified email address. +pub fn publish_rate_override(req: &mut dyn RequestExt) -> EndpointResult { + let admin = req.authenticate()?.forbid_api_token_auth()?.admin_user()?; + increase_rate_limit(admin, req) +} + +/// Increasing the rate limit requires that you are an admin user, but no information from the +/// admin user is currently needed. Someday having an audit log of which admin user took the action +/// would be nice. +fn increase_rate_limit(_admin: AdminUser, req: &mut dyn RequestExt) -> EndpointResult { + let mut body = String::new(); + req.body().read_to_string(&mut body)?; + + let rate_limit_increase: RateLimitIncrease = serde_json::from_str(&body) + .map_err(|e| bad_request(&format!("invalid json request: {e}")))?; + + let conn = req.db_write()?; + let user = User::find_by_verified_email(&conn, &rate_limit_increase.email)?; + + conn.transaction(|| { + diesel::insert_into(publish_rate_overrides::table) + .values(( + publish_rate_overrides::user_id.eq(user.id), + publish_rate_overrides::burst.eq(rate_limit_increase.rate_limit), + publish_rate_overrides::expires_at.eq((now + 30.days()).nullable()), + )) + .execute(&*conn)?; + + diesel::delete(publish_limit_buckets::table) + .filter(publish_limit_buckets::user_id.eq(user.id)) + .execute(&*conn) + })?; + + ok_true() +} diff --git a/src/lib.rs b/src/lib.rs index 7912ed4b221..825c2aa5790 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -46,7 +46,7 @@ pub mod email; pub mod github; pub mod metrics; pub mod middleware; -mod publish_rate_limit; +pub mod publish_rate_limit; pub mod schema; pub mod sql; mod test_util; diff --git a/src/models/user.rs b/src/models/user.rs index 0d8b06e5a0f..ab4342575f3 100644 --- a/src/models/user.rs +++ b/src/models/user.rs @@ -121,6 +121,16 @@ impl User { Ok(Self::find(conn, api_token.user_id)?) } + /// Queries the database for a user with the specified verified email address. + pub fn find_by_verified_email(conn: &PgConnection, email: &str) -> AppResult { + let email: Email = emails::table + .filter(emails::email.eq(email)) + .filter(emails::verified.eq(true)) + .first(conn)?; + + Ok(Self::find(conn, email.user_id)?) + } + pub fn owning(krate: &Crate, conn: &PgConnection) -> QueryResult> { let users = CrateOwner::by_owner_kind(OwnerKind::User) .inner_join(users::table) diff --git a/src/router.rs b/src/router.rs index a18eaf604fb..a3302c7aa33 100644 --- a/src/router.rs +++ b/src/router.rs @@ -157,6 +157,12 @@ pub fn build_router(app: &App) -> RouteBuilder { C(crate_owner_invitation::private_list), ); + // Admin actions + router.put( + "/api/private/admin/rate-limits", + C(admin::publish_rate_override), + ); + // Only serve the local checkout of the git index in development mode. // In production, for crates.io, cargo gets the index from // https://github.com/rust-lang/crates.io-index directly. diff --git a/src/tests/admin_actions.rs b/src/tests/admin_actions.rs new file mode 100644 index 00000000000..e8e8a97f440 --- /dev/null +++ b/src/tests/admin_actions.rs @@ -0,0 +1,141 @@ +use crate::{util::RequestHelper, TestApp}; +use chrono::{NaiveDateTime, Utc}; +use conduit::StatusCode; +use diesel::prelude::*; + +mod rate_limits { + use super::*; + + const RATE_LIMITS_URL: &str = "/api/private/admin/rate-limits"; + + #[test] + fn anon_sending_rate_limit_changes_returns_unauthorized() { + let (_app, anon) = TestApp::init().empty(); + anon.put(RATE_LIMITS_URL, &[]).assert_forbidden(); + } + + #[test] + fn non_admin_sending_rate_limit_changes_returns_unauthorized() { + let (app, _anon) = TestApp::init().empty(); + let user = app.db_new_user("foo"); + user.put(RATE_LIMITS_URL, &[]).assert_forbidden(); + } + + #[test] + fn no_body_content_returns_400() { + let (app, _anon) = TestApp::init().empty(); + let admin_user = app.db_new_user("carols10cents"); + let response = admin_user.put::<()>(RATE_LIMITS_URL, &[]); + + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + assert_eq!( + response.into_json(), + json!({ "errors": [{ + "detail": "invalid json request: EOF while parsing a value at line 1 column 0" + }] }) + ); + } + + #[test] + fn rate_limit_nan_returns_400() { + let (app, _anon) = TestApp::init().empty(); + let admin_user = app.db_new_user("carols10cents"); + + let body = json!({ + "email": "foo@example.com", + "rate_limit": "-34g", + }); + + let response = admin_user.put::<()>(RATE_LIMITS_URL, body.to_string().as_bytes()); + + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + assert_eq!( + response.into_json(), + json!({ "errors": [{ + "detail": "invalid json request: invalid type: string \"-34g\", expected i32 at \ + line 1 column 46" + }] }) + ); + } + + #[test] + fn email_address_lookup_failure_returns_not_found() { + let (app, _anon) = TestApp::init().empty(); + let admin_user = app.db_new_user("carols10cents"); + + let body = json!({ + "email": "foo@example.com", + "rate_limit": 88, + }); + + let response = admin_user.put::<()>(RATE_LIMITS_URL, body.to_string().as_bytes()); + + assert_eq!(response.status(), StatusCode::NOT_FOUND); + assert_eq!( + response.into_json(), + json!({ "errors": [{ + "detail": "Not Found" + }] }) + ); + } + + #[test] + fn email_address_lookup_success_updates_rate_limit() { + use cargo_registry::{ + publish_rate_limit::PublishRateLimit, + schema::{publish_limit_buckets, publish_rate_overrides}, + }; + use std::time::Duration; + + let (app, _, user) = TestApp::init().with_user(); + let user_model = user.as_model(); + + // Check the rate limit for a user, which inserts a record for the user into the + // `publish_limit_buckets` table so that we can test it gets deleted by the rate limit + // override. + app.db(|conn| { + let rate = PublishRateLimit { + rate: Duration::from_secs(1), + burst: 10, + }; + rate.check_rate_limit(user_model.id, conn).unwrap(); + }); + + let admin_user = app.db_new_user("carols10cents"); + + let email = app.db(|conn| user_model.email(conn).unwrap()); + let new_rate_limit = 88; + let body = json!({ + "email": email, + "rate_limit": new_rate_limit, + }); + let response = admin_user.put::<()>(RATE_LIMITS_URL, body.to_string().as_bytes()); + + assert_eq!(response.status(), StatusCode::OK); + + let (rate_limit, expires_at): (i32, Option) = app + .db(|conn| { + publish_rate_overrides::table + .select(( + publish_rate_overrides::burst, + publish_rate_overrides::expires_at, + )) + .filter(publish_rate_overrides::user_id.eq(user_model.id)) + .first(conn) + }) + .unwrap(); + assert_eq!(rate_limit, new_rate_limit); + assert_eq!( + expires_at.unwrap().date(), + (Utc::now() + chrono::Duration::days(30)).naive_utc().date() + ); + app.db(|conn| { + assert!(!diesel::select(diesel::dsl::exists( + publish_limit_buckets::table + .filter(publish_limit_buckets::user_id.eq(user_model.id)) + )) + .get_result::(conn) + .unwrap()); + }); + } +} diff --git a/src/tests/all.rs b/src/tests/all.rs index afe9928bed5..f08c28ef073 100644 --- a/src/tests/all.rs +++ b/src/tests/all.rs @@ -28,6 +28,7 @@ use std::{ use diesel::prelude::*; mod account_lock; +mod admin_actions; mod authentication; mod badge; mod blocked_routes; From c0bc75db089880697452ff4e9eb3056d50f64763 Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Thu, 20 Oct 2022 21:23:42 -0400 Subject: [PATCH 3/3] frontend work --- app/models/user.js | 1 + app/router.js | 3 + app/routes/admin/index.js | 34 +++++++++ app/routes/admin/rate-limits.js | 30 ++++++++ app/styles/admin/rate-limits.module.css | 18 +++++ app/templates/admin/rate-limits.hbs | 18 +++++ tests/acceptance/admin-test.js | 95 +++++++++++++++++++++++++ 7 files changed, 199 insertions(+) create mode 100644 app/routes/admin/index.js create mode 100644 app/routes/admin/rate-limits.js create mode 100644 app/styles/admin/rate-limits.module.css create mode 100644 app/templates/admin/rate-limits.hbs create mode 100644 tests/acceptance/admin-test.js diff --git a/app/models/user.js b/app/models/user.js index d6ad38ddaa6..bf20432f80f 100644 --- a/app/models/user.js +++ b/app/models/user.js @@ -14,6 +14,7 @@ export default class User extends Model { @attr avatar; @attr url; @attr kind; + @attr admin; async stats() { return await customAction(this, { method: 'GET', path: 'stats' }); diff --git a/app/router.js b/app/router.js index 8936900e89b..81aadd96430 100644 --- a/app/router.js +++ b/app/router.js @@ -30,6 +30,9 @@ Router.map(function () { this.route('following'); this.route('pending-invites'); }); + this.route('admin', function () { + this.route('rate-limits'); + }); this.route('settings', function () { this.route('appearance'); this.route('email-notifications'); diff --git a/app/routes/admin/index.js b/app/routes/admin/index.js new file mode 100644 index 00000000000..b399e457dbc --- /dev/null +++ b/app/routes/admin/index.js @@ -0,0 +1,34 @@ +import { inject as service } from '@ember/service'; + +import AuthenticatedRoute from './../-authenticated-route'; + +export default class AdminRoute extends AuthenticatedRoute { + @service router; + @service session; + + async beforeModel(transition) { + // wait for the `loadUserTask.perform()` of either the `application` route, + // or the `session.login()` call + let result = await this.session.loadUserTask.last; + + if (!result.currentUser) { + this.session.savedTransition = transition; + this.router.replaceWith('catch-all', { + transition, + loginNeeded: true, + title: 'This page requires admin authentication', + }); + } else if (!result.currentUser.admin) { + this.session.savedTransition = transition; + this.router.replaceWith('catch-all', { + transition, + loginNeeded: false, + title: 'This page requires admin authentication', + }); + } + } + + redirect() { + this.router.replaceWith('admin.rate-limits'); + } +} diff --git a/app/routes/admin/rate-limits.js b/app/routes/admin/rate-limits.js new file mode 100644 index 00000000000..661dc420337 --- /dev/null +++ b/app/routes/admin/rate-limits.js @@ -0,0 +1,30 @@ +import { inject as service } from '@ember/service'; + +import AuthenticatedRoute from './../-authenticated-route'; + +export default class RateLimitsAdminRoute extends AuthenticatedRoute { + @service router; + @service session; + + async beforeModel(transition) { + // wait for the `loadUserTask.perform()` of either the `application` route, + // or the `session.login()` call + let result = await this.session.loadUserTask.last; + + if (!result.currentUser) { + this.session.savedTransition = transition; + this.router.replaceWith('catch-all', { + transition, + loginNeeded: true, + title: 'This page requires admin authentication', + }); + } else if (!result.currentUser.admin) { + this.session.savedTransition = transition; + this.router.replaceWith('catch-all', { + transition, + loginNeeded: false, + title: 'This page requires admin authentication', + }); + } + } +} diff --git a/app/styles/admin/rate-limits.module.css b/app/styles/admin/rate-limits.module.css new file mode 100644 index 00000000000..a2e6790b58f --- /dev/null +++ b/app/styles/admin/rate-limits.module.css @@ -0,0 +1,18 @@ +.rate-limit {} + +.page { + display: grid; + gap: 16px; + + @media (--min-m) { + grid-template: + "menu content" auto / + 200px auto; + } +} + +.content { + h2:first-child { + margin-top: 4px; + } +} diff --git a/app/templates/admin/rate-limits.hbs b/app/templates/admin/rate-limits.hbs new file mode 100644 index 00000000000..1103f1c594e --- /dev/null +++ b/app/templates/admin/rate-limits.hbs @@ -0,0 +1,18 @@ +{{page-title 'Admin Actions'}} + + + +
+ + Increase Rate Limit + More actions coming soon + + +
+
+

Increase Rate Limit

+ +
+
+ +
diff --git a/tests/acceptance/admin-test.js b/tests/acceptance/admin-test.js new file mode 100644 index 00000000000..0d3eae9b0ed --- /dev/null +++ b/tests/acceptance/admin-test.js @@ -0,0 +1,95 @@ +import { currentURL } from '@ember/test-helpers'; +import { module, test } from 'qunit'; + +import { setupApplicationTest } from 'cargo/tests/helpers'; + +import { visit } from '../helpers/visit-ignoring-abort'; + +module('Acceptance | Admin', function (hooks) { + setupApplicationTest(hooks); + + test('shows "page requires admin authentication" error when not logged in', async function (assert) { + await visit('/admin'); + assert.equal(currentURL(), '/admin'); + assert.dom('[data-test-title]').hasText('This page requires admin authentication'); + assert.dom('[data-test-login]').exists(); + }); + + test('shows "page requires admin authentication" error when logged in but not as an admin', async function (assert) { + let user = this.server.create('user', { + login: 'johnnydee', + name: 'John Doe', + email: 'john@doe.com', + avatar: 'https://avatars2.githubusercontent.com/u/1234567?v=4', + admin: false, + }); + + this.authenticateAs(user); + + await visit('/admin'); + assert.equal(currentURL(), '/admin'); + assert.dom('[data-test-title]').hasText('This page requires admin authentication'); + assert.dom('[data-test-login]').doesNotExist(); + }); + + test('shows admin actions when logged in as an admin', async function (assert) { + let user = this.server.create('user', { + login: 'johnnydee', + name: 'John Doe', + email: 'john@doe.com', + avatar: 'https://avatars2.githubusercontent.com/u/1234567?v=4', + admin: true, + }); + + this.authenticateAs(user); + + await visit('/admin'); + // Rate limits is the default action. + assert.equal(currentURL(), '/admin/rate-limits'); + assert.dom('[data-test-heading]').hasText('Admin Actions'); + assert.dom('[data-test-login]').doesNotExist(); + }); + + module('Rate limits', function () { + test('shows "page requires admin authentication" error when not logged in', async function (assert) { + await visit('/admin/rate-limits'); + assert.equal(currentURL(), '/admin/rate-limits'); + assert.dom('[data-test-title]').hasText('This page requires admin authentication'); + assert.dom('[data-test-login]').exists(); + }); + + test('shows "page requires admin authentication" error when logged in but not as an admin', async function (assert) { + let user = this.server.create('user', { + login: 'johnnydee', + name: 'John Doe', + email: 'john@doe.com', + avatar: 'https://avatars2.githubusercontent.com/u/1234567?v=4', + admin: false, + }); + + this.authenticateAs(user); + + await visit('/admin/rate-limits'); + assert.equal(currentURL(), '/admin/rate-limits'); + assert.dom('[data-test-title]').hasText('This page requires admin authentication'); + assert.dom('[data-test-login]').doesNotExist(); + }); + }); + + test('shows rate limit actions when logged in as an admin', async function (assert) { + let user = this.server.create('user', { + login: 'johnnydee', + name: 'John Doe', + email: 'john@doe.com', + avatar: 'https://avatars2.githubusercontent.com/u/1234567?v=4', + admin: true, + }); + + this.authenticateAs(user); + + await visit('/admin/rate-limits'); + assert.equal(currentURL(), '/admin/rate-limits'); + assert.dom('[data-test-heading]').hasText('Admin Actions'); + assert.dom('[data-test-login]').doesNotExist(); + }); +});