From 8d35e31d96468d16fd08c552c1d271a35e5f35ae Mon Sep 17 00:00:00 2001 From: Mark Catley Date: Mon, 1 Apr 2019 12:02:56 +1300 Subject: [PATCH 1/5] Adding audit trail actions into the publish, yank and unyank transactions. --- .../down.sql | 17 +++ .../up.sql | 20 +++ src/controllers/krate/publish.rs | 14 ++- src/controllers/version/yank.rs | 10 +- src/middleware/current_user.rs | 29 ++++- src/models.rs | 2 +- src/models/action.rs | 76 +++++++++++- src/models/token.rs | 18 +++ src/models/user.rs | 32 ++--- src/schema.rs | 18 +-- src/tasks/dump_db/dump-db.toml | 4 +- .../krate_publish_records_an_audit_action | 73 +++++++++++ .../krate_unyank_records_an_audit_action | 73 +++++++++++ .../krate_yank_records_an_audit_action | 73 +++++++++++ src/tests/krate.rs | 114 ++++++++++++++++++ 15 files changed, 524 insertions(+), 49 deletions(-) create mode 100644 migrations/2019-11-03-003946_tidy_up_version_owner_actions/down.sql create mode 100644 migrations/2019-11-03-003946_tidy_up_version_owner_actions/up.sql create mode 100644 src/tests/http-data/krate_publish_records_an_audit_action create mode 100644 src/tests/http-data/krate_unyank_records_an_audit_action create mode 100644 src/tests/http-data/krate_yank_records_an_audit_action 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 6061e410dc2..9d4595f69c8 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::{CargoError, ChainError, Maximums}; @@ -143,6 +147,14 @@ pub fn publish(req: &mut dyn Request) -> CargoResult { )? .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 dd1105d3501..d39cc32598f 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::CargoError; /// Handles the `DELETE /crates/:crate_id/:version/yank` route. @@ -35,6 +35,14 @@ fn modify_yank(req: &mut dyn Request, yanked: bool) -> CargoResult { if user.rights(req.app(), &owners)? < Rights::Publish { return Err(human("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 ad760c88857..2a8ecb4e722 100644 --- a/src/middleware/current_user.rs +++ b/src/middleware/current_user.rs @@ -6,16 +6,17 @@ use diesel::prelude::*; use crate::db::RequestTransaction; use crate::util::errors::{std_error, CargoResult, ChainError, Unauthorized}; +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 { @@ -43,10 +44,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 { @@ -85,3 +93,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 806d7c91dc9..d3214dae9b8 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..51329364aeb 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 2f22c20a19e..d0d4e777d7f 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::CargoResult; -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}; @@ -105,27 +104,14 @@ 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)); - - // 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))?; - - users::table.find(user_id_).first(conn) + pub fn find(conn: &PgConnection, id: i32) -> QueryResult { + users::table.find(id).first(conn) + } + + pub fn find_by_api_token(conn: &PgConnection, token: &str) -> QueryResult { + let api_token = ApiToken::find_by_api_token(conn, token)?; + + Self::find(conn, api_token.user_id) } pub fn owning(krate: &Crate, conn: &PgConnection) -> CargoResult> { diff --git a/src/schema.rs b/src/schema.rs index 6b692eeada9..83f1184b61f 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -876,22 +876,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`. @@ -1029,8 +1029,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 12302c891ba..b2d9c7213d7 100644 --- a/src/tasks/dump_db/dump-db.toml +++ b/src/tasks/dump_db/dump-db.toml @@ -194,8 +194,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..f4f9a36dad3 100644 --- a/src/tests/krate.rs +++ b/src/tests/krate.rs @@ -1589,6 +1589,120 @@ 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 doesn't have any audit actions + // 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; + + // make sure it has one audit action + // do this as a full integration test once the api is in place. + 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(); + + // make sure it doesn't have any audit actions + // 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| { + assert!( + VersionOwnerAction::by_version_id_and_action(conn, version_id, VersionAction::Yank) + .unwrap() + .is_empty() + ) + }); + + // yank it + token.yank("fyk", "1.0.0").good(); + + // make sure it has one 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(); + + app.db(|conn| { + assert!(VersionOwnerAction::by_version_id_and_action( + conn, + version_id, + VersionAction::Unyank + ) + .unwrap() + .is_empty()) + }); + + // unyank version 1.0.0 + token.unyank("fyk", "1.0.0").good(); + + // make sure it has one 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(); From a6430a0c1d7d58bd185f092553cb4d7248fa9264 Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Mon, 2 Dec 2019 10:10:29 -0500 Subject: [PATCH 2/5] Make parameter names consistent with conventions --- src/models/action.rs | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/models/action.rs b/src/models/action.rs index 51329364aeb..d764f2b7d1b 100644 --- a/src/models/action.rs +++ b/src/models/action.rs @@ -58,33 +58,33 @@ impl VersionOwnerAction { pub fn by_version_id_and_action( conn: &PgConnection, - _version_id: i32, - _action: VersionAction, + 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)) + .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, + 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), + version_id.eq(version_id_), + user_id.eq(user_id_), + api_token_id.eq(api_token_id_), + action.eq(action_), )) .get_result(conn) } From 3b1061fed6a18d70aa0f782e9c22b6196398161f Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Mon, 2 Dec 2019 10:12:54 -0500 Subject: [PATCH 3/5] Clarify test description comments --- src/tests/krate.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/tests/krate.rs b/src/tests/krate.rs index f4f9a36dad3..73cd31424e4 100644 --- a/src/tests/krate.rs +++ b/src/tests/krate.rs @@ -1601,13 +1601,11 @@ fn publish_records_an_audit_action() { let crate_to_publish = PublishBuilder::new("fyk"); token.enqueue_publish(crate_to_publish).good(); - // make sure it doesn't have any audit actions + // 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; - // make sure it has one audit action - // do this as a full integration test once the api is in place. app.db(|conn| { let actions = VersionOwnerAction::all(conn).unwrap(); assert_eq!(actions.len(), 1); @@ -1629,8 +1627,6 @@ fn yank_records_an_audit_action() { let crate_to_publish = PublishBuilder::new("fyk"); token.enqueue_publish(crate_to_publish).good(); - // make sure it doesn't have any audit actions - // 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| { @@ -1644,7 +1640,7 @@ fn yank_records_an_audit_action() { // yank it token.yank("fyk", "1.0.0").good(); - // make sure it has one audit action + // 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 = @@ -1688,7 +1684,7 @@ fn unyank_records_an_audit_action() { // unyank version 1.0.0 token.unyank("fyk", "1.0.0").good(); - // make sure it has one audit action + // 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 = From e38744c8c15c937c0c1587aa3c938154020143c7 Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Mon, 2 Dec 2019 10:13:50 -0500 Subject: [PATCH 4/5] Remove some unnecessary assertions The first audit action test covers testing that a crate starts without any audit actions, and it's unlikely these assertions will ever fail. --- src/tests/krate.rs | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/src/tests/krate.rs b/src/tests/krate.rs index 73cd31424e4..2e10a8f08b1 100644 --- a/src/tests/krate.rs +++ b/src/tests/krate.rs @@ -1629,13 +1629,6 @@ fn yank_records_an_audit_action() { let json = anon.show_version("fyk", "1.0.0"); let version_id = json.version.id; - app.db(|conn| { - assert!( - VersionOwnerAction::by_version_id_and_action(conn, version_id, VersionAction::Yank) - .unwrap() - .is_empty() - ) - }); // yank it token.yank("fyk", "1.0.0").good(); @@ -1671,16 +1664,6 @@ fn unyank_records_an_audit_action() { // yank version 1.0.0 token.yank("fyk", "1.0.0").good(); - app.db(|conn| { - assert!(VersionOwnerAction::by_version_id_and_action( - conn, - version_id, - VersionAction::Unyank - ) - .unwrap() - .is_empty()) - }); - // unyank version 1.0.0 token.unyank("fyk", "1.0.0").good(); From 4d7f8a8f1eb67020676c3b99b187f29954684b97 Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Mon, 2 Dec 2019 11:04:44 -0500 Subject: [PATCH 5/5] cargo fmt --- src/middleware/current_user.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/current_user.rs b/src/middleware/current_user.rs index 4d68c25b382..a5a02583d8a 100644 --- a/src/middleware/current_user.rs +++ b/src/middleware/current_user.rs @@ -6,8 +6,8 @@ use diesel::prelude::*; use crate::db::RequestTransaction; use crate::util::errors::{std_error, AppResult, ChainError, Unauthorized}; -use crate::models::ApiToken; use crate::models::user::{UserNoEmailType, ALL_COLUMNS}; +use crate::models::ApiToken; use crate::models::User; use crate::schema::users;