Auto merge of #1700 - markcatley:version-owner-actions, r=carols10cents

Add audit trail to the publish, yank and unyank transactions.

Contributes to [Issue 1548](#1548)
Based on the commits from [PR 1604](#1604)

Author: bors

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
+;

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
+;

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<Response> {
         )?
         .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)?;
 

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<Response> {
     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)

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<dyn Error + Send>)?
             } else {
@@ -90,3 +98,12 @@ impl<'a> RequestUser for dyn Request + 'a {
             .chain_error(|| Unauthorized)
     }
 }
+
+impl AuthenticationSource {
+    pub fn api_token_id(self) -> Option<i32> {
+        match self {
+            AuthenticationSource::SessionCookie => None,
+            AuthenticationSource::ApiToken { api_token_id } => Some(api_token_id),
+        }
+    }
+}

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};

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<Integer, Pg> for VersionAction {
+    fn from_sql(bytes: Option<&[u8]>) -> deserialize::Result<Self> {
+        match <i32 as FromSql<Integer, Pg>>::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<Integer, Pg> for VersionAction {
+    fn to_sql<W: Write>(&self, out: &mut Output<'_, W, Pg>) -> serialize::Result {
+        ToSql::<Integer, Pg>::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<i32>,
     pub action: VersionAction,
     pub time: NaiveDateTime,
 }
+
+impl VersionOwnerAction {
+    pub fn all(conn: &PgConnection) -> QueryResult<Vec<VersionOwnerAction>> {
+        version_owner_actions::table.load(conn)
+    }
+
+    pub fn by_version_id_and_action(
+        conn: &PgConnection,
+        version_id_: i32,
+        action_: VersionAction,
+    ) -> QueryResult<Vec<VersionOwnerAction>> {
+        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<i32>,
+    action_: VersionAction,
+) -> QueryResult<VersionOwnerAction> {
+    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)
+}

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<ApiToken> {
+        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::<ApiToken>(conn)
+        })
+        .or_else(|_| tokens.first(conn))
+    }
 }
 
 #[cfg(test)]

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<User> {
-        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<User> {
+        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::<i32>(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<User> {
+        let api_token = ApiToken::find_by_api_token(conn, token)?;
 
-        users::table
-            .select(ALL_COLUMNS)
-            .find(user_id_)
-            .first::<UserNoEmailType>(conn)
-            .map(User::from)
+        Self::find(conn, api_token.user_id)
     }
 
     pub fn owning(krate: &Crate, conn: &PgConnection) -> AppResult<Vec<Owner>> {

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<Int4>`.
+        /// Its SQL type is `Int4`.
         ///
         /// (Automatically generated by Diesel.)
-        version_id -> Nullable<Int4>,
-        /// 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<Int4>`.
+        /// Its SQL type is `Int4`.
         ///
         /// (Automatically generated by Diesel.)
-        owner_id -> Nullable<Int4>,
-        /// 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<Int4>`.
         ///
         /// (Automatically generated by Diesel.)
-        owner_token_id -> Nullable<Int4>,
+        api_token_id -> Nullable<Int4>,
         /// 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));

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"