diff --git a/migrations/2019-11-03-003946_tidy_up_version_owner_actions/down.sql b/migrations/2019-11-03-003946_tidy_up_version_owner_actions/down.sql new file mode 100644 index 00000000000..abd9cbd50d4 --- /dev/null +++ b/migrations/2019-11-03-003946_tidy_up_version_owner_actions/down.sql @@ -0,0 +1,17 @@ +ALTER TABLE version_owner_actions +ALTER COLUMN user_id +DROP NOT NULL +; + +ALTER TABLE version_owner_actions +ALTER COLUMN version_id +DROP NOT NULL +; + +ALTER TABLE version_owner_actions +RENAME COLUMN user_id TO owner_id +; + +ALTER TABLE version_owner_actions +RENAME COLUMN api_token_id TO owner_token_id +; diff --git a/migrations/2019-11-03-003946_tidy_up_version_owner_actions/up.sql b/migrations/2019-11-03-003946_tidy_up_version_owner_actions/up.sql new file mode 100644 index 00000000000..61f09c02459 --- /dev/null +++ b/migrations/2019-11-03-003946_tidy_up_version_owner_actions/up.sql @@ -0,0 +1,20 @@ +-- I expect that it will be save to apply this migration in production as the code that adds records +-- to this table is not used until this changeset. + +ALTER TABLE version_owner_actions +RENAME COLUMN owner_id TO user_id +; + +ALTER TABLE version_owner_actions +RENAME COLUMN owner_token_id TO api_token_id +; + +ALTER TABLE version_owner_actions +ALTER COLUMN user_id +SET NOT NULL +; + +ALTER TABLE version_owner_actions +ALTER COLUMN version_id +SET NOT NULL +; diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index 9faeee0694a..527d3588bf7 100644 --- a/src/controllers/krate/publish.rs +++ b/src/controllers/krate/publish.rs @@ -7,7 +7,11 @@ use swirl::Job; use crate::controllers::prelude::*; use crate::git; use crate::models::dependency; -use crate::models::{Badge, Category, Keyword, NewCrate, NewVersion, Rights, User}; +use crate::models::{ + insert_version_owner_action, Badge, Category, Keyword, NewCrate, NewVersion, Rights, User, + VersionAction, +}; + use crate::render; use crate::util::{read_fill, read_le_u32}; use crate::util::{AppError, ChainError, Maximums}; @@ -143,6 +147,14 @@ pub fn publish(req: &mut dyn Request) -> AppResult { )? .save(&conn, &new_crate.authors, &verified_email_address)?; + insert_version_owner_action( + &conn, + version.id, + user.id, + req.authentication_source()?.api_token_id(), + VersionAction::Publish, + )?; + // Link this new version to all dependencies let git_deps = dependency::add_dependencies(&conn, &new_crate.deps, version.id)?; diff --git a/src/controllers/version/yank.rs b/src/controllers/version/yank.rs index f29da798d67..af36b8e579d 100644 --- a/src/controllers/version/yank.rs +++ b/src/controllers/version/yank.rs @@ -5,7 +5,7 @@ use swirl::Job; use super::version_and_crate; use crate::controllers::prelude::*; use crate::git; -use crate::models::Rights; +use crate::models::{insert_version_owner_action, Rights, VersionAction}; use crate::util::AppError; /// Handles the `DELETE /crates/:crate_id/:version/yank` route. @@ -35,6 +35,14 @@ fn modify_yank(req: &mut dyn Request, yanked: bool) -> AppResult { if user.rights(req.app(), &owners)? < Rights::Publish { return Err(cargo_err("must already be an owner to yank or unyank")); } + let action = if yanked { + VersionAction::Yank + } else { + VersionAction::Unyank + }; + let api_token_id = req.authentication_source()?.api_token_id(); + + insert_version_owner_action(&conn, version.id, user.id, api_token_id, action)?; git::yank(krate.name, version, yanked) .enqueue(&conn) diff --git a/src/middleware/current_user.rs b/src/middleware/current_user.rs index 4fb42a3fb23..a5a02583d8a 100644 --- a/src/middleware/current_user.rs +++ b/src/middleware/current_user.rs @@ -7,16 +7,17 @@ use crate::db::RequestTransaction; use crate::util::errors::{std_error, AppResult, ChainError, Unauthorized}; use crate::models::user::{UserNoEmailType, ALL_COLUMNS}; +use crate::models::ApiToken; use crate::models::User; use crate::schema::users; #[derive(Debug, Clone, Copy)] pub struct CurrentUser; -#[derive(Debug, Clone, Eq, PartialEq)] +#[derive(Debug, Copy, Clone, Eq, PartialEq)] pub enum AuthenticationSource { SessionCookie, - ApiToken { auth_header: String }, + ApiToken { api_token_id: i32 }, } impl Middleware for CurrentUser { @@ -48,10 +49,17 @@ impl Middleware for CurrentUser { // Otherwise, look for an `Authorization` header on the request // and try to find a user in the database with a matching API token let user_auth = if let Some(headers) = req.headers().find("Authorization") { - let auth_header = headers[0].to_string(); - - User::find_by_api_token(&conn, &auth_header) - .map(|user| (AuthenticationSource::ApiToken { auth_header }, user)) + ApiToken::find_by_api_token(&conn, headers[0]) + .and_then(|api_token| { + User::find(&conn, api_token.user_id).map(|user| { + ( + AuthenticationSource::ApiToken { + api_token_id: api_token.id, + }, + user, + ) + }) + }) .optional() .map_err(|e| Box::new(e) as Box)? } else { @@ -90,3 +98,12 @@ impl<'a> RequestUser for dyn Request + 'a { .chain_error(|| Unauthorized) } } + +impl AuthenticationSource { + pub fn api_token_id(self) -> Option { + match self { + AuthenticationSource::SessionCookie => None, + AuthenticationSource::ApiToken { api_token_id } => Some(api_token_id), + } + } +} diff --git a/src/models.rs b/src/models.rs index f11eb84e9d4..9fea1f7874d 100644 --- a/src/models.rs +++ b/src/models.rs @@ -1,4 +1,4 @@ -pub use self::action::{VersionAction, VersionOwnerAction}; +pub use self::action::{insert_version_owner_action, VersionAction, VersionOwnerAction}; pub use self::badge::{Badge, CrateBadge, MaintenanceStatus}; pub use self::category::{Category, CrateCategory, NewCategory}; pub use self::crate_owner_invitation::{CrateOwnerInvitation, NewCrateOwnerInvitation}; diff --git a/src/models/action.rs b/src/models/action.rs index 474bc252ab8..d764f2b7d1b 100644 --- a/src/models/action.rs +++ b/src/models/action.rs @@ -1,26 +1,90 @@ use chrono::NaiveDateTime; +use diesel::prelude::*; +use diesel::{ + deserialize::{self, FromSql}, + pg::Pg, + serialize::{self, Output, ToSql}, + sql_types::Integer, +}; +use std::io::Write; use crate::models::{ApiToken, User, Version}; use crate::schema::*; -#[derive(Debug, Clone, Copy)] -#[repr(u32)] +#[derive(Debug, Clone, Copy, PartialEq, FromSqlRow, AsExpression)] +#[repr(i32)] +#[sql_type = "Integer"] pub enum VersionAction { Publish = 0, Yank = 1, Unyank = 2, } +impl FromSql for VersionAction { + fn from_sql(bytes: Option<&[u8]>) -> deserialize::Result { + match >::from_sql(bytes)? { + 0 => Ok(VersionAction::Publish), + 1 => Ok(VersionAction::Yank), + 2 => Ok(VersionAction::Unyank), + n => Err(format!("unknown version action: {}", n).into()), + } + } +} + +impl ToSql for VersionAction { + fn to_sql(&self, out: &mut Output<'_, W, Pg>) -> serialize::Result { + ToSql::::to_sql(&(*self as i32), out) + } +} + #[derive(Debug, Clone, Copy, Queryable, Identifiable, Associations)] #[belongs_to(Version)] -#[belongs_to(User, foreign_key = "owner_id")] -#[belongs_to(ApiToken, foreign_key = "owner_token_id")] +#[belongs_to(User, foreign_key = "user_id")] +#[belongs_to(ApiToken, foreign_key = "api_token_id")] #[table_name = "version_owner_actions"] pub struct VersionOwnerAction { pub id: i32, pub version_id: i32, - pub owner_id: i32, - pub owner_token_id: i32, + pub user_id: i32, + pub api_token_id: Option, pub action: VersionAction, pub time: NaiveDateTime, } + +impl VersionOwnerAction { + pub fn all(conn: &PgConnection) -> QueryResult> { + version_owner_actions::table.load(conn) + } + + pub fn by_version_id_and_action( + conn: &PgConnection, + version_id_: i32, + action_: VersionAction, + ) -> QueryResult> { + use version_owner_actions::dsl::{action, version_id}; + + version_owner_actions::table + .filter(version_id.eq(version_id_)) + .filter(action.eq(action_)) + .load(conn) + } +} + +pub fn insert_version_owner_action( + conn: &PgConnection, + version_id_: i32, + user_id_: i32, + api_token_id_: Option, + action_: VersionAction, +) -> QueryResult { + use version_owner_actions::dsl::{action, api_token_id, user_id, version_id}; + + diesel::insert_into(version_owner_actions::table) + .values(( + version_id.eq(version_id_), + user_id.eq(user_id_), + api_token_id.eq(api_token_id_), + action.eq(action_), + )) + .get_result(conn) +} diff --git a/src/models/token.rs b/src/models/token.rs index 5765a6e6a51..751ce3ee485 100644 --- a/src/models/token.rs +++ b/src/models/token.rs @@ -46,6 +46,24 @@ impl ApiToken { last_used_at: self.last_used_at, } } + + pub fn find_by_api_token(conn: &PgConnection, token_: &str) -> QueryResult { + use crate::schema::api_tokens::dsl::{api_tokens, last_used_at, revoked, token}; + use diesel::{dsl::now, update}; + + let tokens = api_tokens + .filter(token.eq(token_)) + .filter(revoked.eq(false)); + + // If the database is in read only mode, we can't update last_used_at. + // Try updating in a new transaction, if that fails, fall back to reading + conn.transaction(|| { + update(tokens) + .set(last_used_at.eq(now.nullable())) + .get_result::(conn) + }) + .or_else(|_| tokens.first(conn)) + } } #[cfg(test)] diff --git a/src/models/user.rs b/src/models/user.rs index b6c079534bc..e5cb8cbf0a4 100644 --- a/src/models/user.rs +++ b/src/models/user.rs @@ -1,11 +1,10 @@ -use diesel::dsl::now; use diesel::prelude::*; use std::borrow::Cow; use crate::app::App; use crate::util::AppResult; -use crate::models::{Crate, CrateOwner, Email, NewEmail, Owner, OwnerKind, Rights}; +use crate::models::{ApiToken, Crate, CrateOwner, Email, NewEmail, Owner, OwnerKind, Rights}; use crate::schema::{crate_owners, emails, users}; use crate::views::{EncodablePrivateUser, EncodablePublicUser}; @@ -141,31 +140,15 @@ impl<'a> NewUser<'a> { } impl User { - /// Queries the database for a user with a certain `api_token` value. - pub fn find_by_api_token(conn: &PgConnection, token_: &str) -> QueryResult { - use crate::schema::api_tokens::dsl::{api_tokens, last_used_at, revoked, token, user_id}; - use diesel::update; - - let tokens = api_tokens - .filter(token.eq(token_)) - .filter(revoked.eq(false)); + pub fn find(conn: &PgConnection, id: i32) -> QueryResult { + users::table.find(id).first(conn) + } - // If the database is in read only mode, we can't update last_used_at. - // Try updating in a new transaction, if that fails, fall back to reading - let user_id_ = conn - .transaction(|| { - update(tokens) - .set(last_used_at.eq(now.nullable())) - .returning(user_id) - .get_result::(conn) - }) - .or_else(|_| tokens.select(user_id).first(conn))?; + /// Queries the database for a user with a certain `api_token` value. + pub fn find_by_api_token(conn: &PgConnection, token: &str) -> QueryResult { + let api_token = ApiToken::find_by_api_token(conn, token)?; - users::table - .select(ALL_COLUMNS) - .find(user_id_) - .first::(conn) - .map(User::from) + Self::find(conn, api_token.user_id) } pub fn owning(krate: &Crate, conn: &PgConnection) -> AppResult> { diff --git a/src/schema.rs b/src/schema.rs index 26651270b57..a18a3f16cb7 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -882,22 +882,22 @@ table! { id -> Int4, /// The `version_id` column of the `version_owner_actions` table. /// - /// Its SQL type is `Nullable`. + /// Its SQL type is `Int4`. /// /// (Automatically generated by Diesel.) - version_id -> Nullable, - /// The `owner_id` column of the `version_owner_actions` table. + version_id -> Int4, + /// The `user_id` column of the `version_owner_actions` table. /// - /// Its SQL type is `Nullable`. + /// Its SQL type is `Int4`. /// /// (Automatically generated by Diesel.) - owner_id -> Nullable, - /// The `owner_token_id` column of the `version_owner_actions` table. + user_id -> Int4, + /// The `api_token_id` column of the `version_owner_actions` table. /// /// Its SQL type is `Nullable`. /// /// (Automatically generated by Diesel.) - owner_token_id -> Nullable, + api_token_id -> Nullable, /// The `action` column of the `version_owner_actions` table. /// /// Its SQL type is `Int4`. @@ -1035,8 +1035,8 @@ joinable!(recent_crate_downloads -> crates (crate_id)); joinable!(version_authors -> users (user_id)); joinable!(version_authors -> versions (version_id)); joinable!(version_downloads -> versions (version_id)); -joinable!(version_owner_actions -> api_tokens (owner_token_id)); -joinable!(version_owner_actions -> users (owner_id)); +joinable!(version_owner_actions -> api_tokens (api_token_id)); +joinable!(version_owner_actions -> users (user_id)); joinable!(version_owner_actions -> versions (version_id)); joinable!(versions -> crates (crate_id)); joinable!(versions -> users (published_by)); diff --git a/src/tasks/dump_db/dump-db.toml b/src/tasks/dump_db/dump-db.toml index 37042483dc2..9a60bb624d4 100644 --- a/src/tasks/dump_db/dump-db.toml +++ b/src/tasks/dump_db/dump-db.toml @@ -195,8 +195,8 @@ processed = "private" [version_owner_actions.columns] id = "private" version_id = "private" -owner_id = "private" -owner_token_id = "private" +user_id = "private" +api_token_id = "private" action = "private" time = "private" diff --git a/src/tests/http-data/krate_publish_records_an_audit_action b/src/tests/http-data/krate_publish_records_an_audit_action new file mode 100644 index 00000000000..7ece8b4f2de --- /dev/null +++ b/src/tests/http-data/krate_publish_records_an_audit_action @@ -0,0 +1,73 @@ +[ + { + "request": { + "uri": "http://alexcrichton-test.s3.amazonaws.com/crates/fyk/fyk-1.0.0.crate", + "method": "PUT", + "headers": [ + [ + "host", + "alexcrichton-test.s3.amazonaws.com" + ], + [ + "date", + "Fri, 15 Sep 2017 07:53:06 -0700" + ], + [ + "accept", + "*/*" + ], + [ + "user-agent", + "reqwest/0.9.1" + ], + [ + "authorization", + "AWS AKIAICL5IWUZYWWKA7JA:xCp3sUdUdmScjI6ct58zFv6BoGQ=" + ], + [ + "content-length", + "35" + ], + [ + "accept-encoding", + "gzip" + ], + [ + "content-type", + "application/x-tar" + ] + ], + "body": "H4sIAAAAAAAA/+3AAQEAAACCIP+vbkhQwKsBLq+17wAEAAA=" + }, + "response": { + "status": 200, + "headers": [ + [ + "ETag", + "\"f9016ad360cebb4fe2e6e96e5949f022\"" + ], + [ + "x-amz-id-2", + "FCKNKZUo5EeUNwVyhZ9P7ehfXoctqePzXx2RSE1VxoSX9rdfskkyAJUNHAF2AQojRon00LfTLPY=" + ], + [ + "x-amz-request-id", + "3233F8227A852593" + ], + [ + "Server", + "AmazonS3" + ], + [ + "content-length", + "0" + ], + [ + "date", + "Fri, 15 Sep 2017 14:53:07 GMT" + ] + ], + "body": "" + } + } +] \ No newline at end of file diff --git a/src/tests/http-data/krate_unyank_records_an_audit_action b/src/tests/http-data/krate_unyank_records_an_audit_action new file mode 100644 index 00000000000..7ece8b4f2de --- /dev/null +++ b/src/tests/http-data/krate_unyank_records_an_audit_action @@ -0,0 +1,73 @@ +[ + { + "request": { + "uri": "http://alexcrichton-test.s3.amazonaws.com/crates/fyk/fyk-1.0.0.crate", + "method": "PUT", + "headers": [ + [ + "host", + "alexcrichton-test.s3.amazonaws.com" + ], + [ + "date", + "Fri, 15 Sep 2017 07:53:06 -0700" + ], + [ + "accept", + "*/*" + ], + [ + "user-agent", + "reqwest/0.9.1" + ], + [ + "authorization", + "AWS AKIAICL5IWUZYWWKA7JA:xCp3sUdUdmScjI6ct58zFv6BoGQ=" + ], + [ + "content-length", + "35" + ], + [ + "accept-encoding", + "gzip" + ], + [ + "content-type", + "application/x-tar" + ] + ], + "body": "H4sIAAAAAAAA/+3AAQEAAACCIP+vbkhQwKsBLq+17wAEAAA=" + }, + "response": { + "status": 200, + "headers": [ + [ + "ETag", + "\"f9016ad360cebb4fe2e6e96e5949f022\"" + ], + [ + "x-amz-id-2", + "FCKNKZUo5EeUNwVyhZ9P7ehfXoctqePzXx2RSE1VxoSX9rdfskkyAJUNHAF2AQojRon00LfTLPY=" + ], + [ + "x-amz-request-id", + "3233F8227A852593" + ], + [ + "Server", + "AmazonS3" + ], + [ + "content-length", + "0" + ], + [ + "date", + "Fri, 15 Sep 2017 14:53:07 GMT" + ] + ], + "body": "" + } + } +] \ No newline at end of file diff --git a/src/tests/http-data/krate_yank_records_an_audit_action b/src/tests/http-data/krate_yank_records_an_audit_action new file mode 100644 index 00000000000..7ece8b4f2de --- /dev/null +++ b/src/tests/http-data/krate_yank_records_an_audit_action @@ -0,0 +1,73 @@ +[ + { + "request": { + "uri": "http://alexcrichton-test.s3.amazonaws.com/crates/fyk/fyk-1.0.0.crate", + "method": "PUT", + "headers": [ + [ + "host", + "alexcrichton-test.s3.amazonaws.com" + ], + [ + "date", + "Fri, 15 Sep 2017 07:53:06 -0700" + ], + [ + "accept", + "*/*" + ], + [ + "user-agent", + "reqwest/0.9.1" + ], + [ + "authorization", + "AWS AKIAICL5IWUZYWWKA7JA:xCp3sUdUdmScjI6ct58zFv6BoGQ=" + ], + [ + "content-length", + "35" + ], + [ + "accept-encoding", + "gzip" + ], + [ + "content-type", + "application/x-tar" + ] + ], + "body": "H4sIAAAAAAAA/+3AAQEAAACCIP+vbkhQwKsBLq+17wAEAAA=" + }, + "response": { + "status": 200, + "headers": [ + [ + "ETag", + "\"f9016ad360cebb4fe2e6e96e5949f022\"" + ], + [ + "x-amz-id-2", + "FCKNKZUo5EeUNwVyhZ9P7ehfXoctqePzXx2RSE1VxoSX9rdfskkyAJUNHAF2AQojRon00LfTLPY=" + ], + [ + "x-amz-request-id", + "3233F8227A852593" + ], + [ + "Server", + "AmazonS3" + ], + [ + "content-length", + "0" + ], + [ + "date", + "Fri, 15 Sep 2017 14:53:07 GMT" + ] + ], + "body": "" + } + } +] \ No newline at end of file diff --git a/src/tests/krate.rs b/src/tests/krate.rs index accaa6579c2..2e10a8f08b1 100644 --- a/src/tests/krate.rs +++ b/src/tests/krate.rs @@ -1589,6 +1589,99 @@ fn publish_after_yank_max_version() { assert_eq!(json.krate.max_version, "2.0.0"); } +#[test] +fn publish_records_an_audit_action() { + use cargo_registry::models::{VersionAction, VersionOwnerAction}; + + let (app, anon, _, token) = TestApp::full().with_token(); + + app.db(|conn| assert!(VersionOwnerAction::all(&conn).unwrap().is_empty())); + + // Upload a new crate, putting it in the git index + let crate_to_publish = PublishBuilder::new("fyk"); + token.enqueue_publish(crate_to_publish).good(); + + // make sure it has one publish audit action + // do this as a full integration test once the api is in place. + let json = anon.show_version("fyk", "1.0.0"); + let version_id = json.version.id; + + app.db(|conn| { + let actions = VersionOwnerAction::all(conn).unwrap(); + assert_eq!(actions.len(), 1); + let action = actions[0]; + assert_eq!(action.version_id, version_id); + assert_eq!(action.user_id, token.as_model().user_id); + assert_eq!(action.api_token_id, Some(token.as_model().id)); + assert_eq!(action.action, VersionAction::Publish); + }); +} + +#[test] +fn yank_records_an_audit_action() { + use cargo_registry::models::{VersionAction, VersionOwnerAction}; + + let (app, anon, _, token) = TestApp::full().with_token(); + + // Upload a new crate, putting it in the git index + let crate_to_publish = PublishBuilder::new("fyk"); + token.enqueue_publish(crate_to_publish).good(); + + let json = anon.show_version("fyk", "1.0.0"); + let version_id = json.version.id; + + // yank it + token.yank("fyk", "1.0.0").good(); + + // make sure it has one publish and one yank audit action + // do this as a full integration test once the api is in place. + app.db(|conn| { + let actions = + VersionOwnerAction::by_version_id_and_action(conn, version_id, VersionAction::Yank) + .unwrap(); + assert_eq!(actions.len(), 1); + let action = actions[0]; + assert_eq!(action.version_id, version_id); + assert_eq!(action.user_id, token.as_model().user_id); + assert_eq!(action.api_token_id, Some(token.as_model().id)); + assert_eq!(action.action, VersionAction::Yank); + }); +} + +#[test] +fn unyank_records_an_audit_action() { + use cargo_registry::models::{VersionAction, VersionOwnerAction}; + + let (app, anon, _, token) = TestApp::full().with_token(); + + // Upload a new crate + let crate_to_publish = PublishBuilder::new("fyk"); + token.enqueue_publish(crate_to_publish).good(); + + let json = anon.show_version("fyk", "1.0.0"); + let version_id = json.version.id; + + // yank version 1.0.0 + token.yank("fyk", "1.0.0").good(); + + // unyank version 1.0.0 + token.unyank("fyk", "1.0.0").good(); + + // make sure it has one publish, one yank, and one unyank audit action + // do this as a full integration test once the api is in place. + app.db(|conn| { + let actions = + VersionOwnerAction::by_version_id_and_action(conn, version_id, VersionAction::Unyank) + .unwrap(); + assert_eq!(actions.len(), 1); + let action = actions[0]; + assert_eq!(action.version_id, version_id); + assert_eq!(action.user_id, token.as_model().user_id); + assert_eq!(action.api_token_id, Some(token.as_model().id)); + assert_eq!(action.action, VersionAction::Unyank); + }); +} + #[test] fn publish_after_removing_documentation() { let (app, anon, user, token) = TestApp::full().with_token();