Auto merge of rust-lang#1621 - integer32llc:record-verified-email, r=…

…jtgeibel

Record verified email, if present, of the publisher in the version record

Connects to rust-lang#1620. We can start recording emails if we have them, even though we're not requiring verified emails yet.

This builds on top of rust-lang#1561.

migrations/2018-11-01-223239_add_published_by_to_versions/down.sql

@@ -0,0 +1,3 @@
+ALTER TABLE versions DROP CONSTRAINT fk_versions_published_by;
+
+ALTER TABLE versions DROP COLUMN published_by;

migrations/2018-11-01-223239_add_published_by_to_versions/up.sql

@@ -0,0 +1,7 @@
+ALTER TABLE versions
+ADD COLUMN published_by integer;
+
+ALTER TABLE versions
+ADD CONSTRAINT "fk_versions_published_by"
+FOREIGN KEY (published_by)
+REFERENCES users(id);

migrations/2019-02-12-200540_track_version_published_by_email/down.sql

@@ -0,0 +1 @@
+DROP TABLE versions_published_by;

migrations/2019-02-12-200540_track_version_published_by_email/up.sql

@@ -0,0 +1,4 @@
+CREATE TABLE versions_published_by (
+    version_id INTEGER NOT NULL PRIMARY KEY REFERENCES versions(id) ON DELETE CASCADE,
+    email VARCHAR NOT NULL
+);

src/bin/update-downloads.rs

@@ -146,9 +146,12 @@ mod test {
             None,
             None,
             0,
+            user_id,
         )
         .unwrap();
-        let version = version.save(conn, &[]).unwrap();
+        let version = version
+            .save(conn, &[], Some("someone@example.com".into()))
+            .unwrap();
         (krate, version)
     }
 

src/controllers/krate/metadata.rs

@@ -6,7 +6,8 @@
 
 use crate::controllers::prelude::*;
 use crate::models::{
-    Category, Crate, CrateCategory, CrateDownload, CrateKeyword, CrateVersions, Keyword, Version,
+    Category, Crate, CrateCategory, CrateDownload, CrateKeyword, CrateVersions, Keyword, User,
+    Version,
 };
 use crate::schema::*;
 use crate::views::{
@@ -106,9 +107,13 @@ pub fn show(req: &mut dyn Request) -> CargoResult<Response> {
     let conn = req.db_conn()?;
     let krate = Crate::by_name(name).first::<Crate>(&*conn)?;
 
-    let mut versions = krate.all_versions().load::<Version>(&*conn)?;
-    versions.sort_by(|a, b| b.num.cmp(&a.num));
-    let ids = versions.iter().map(|v| v.id).collect();
+    let mut versions_and_publishers: Vec<(Version, Option<User>)> = krate
+        .all_versions()
+        .left_outer_join(users::table)
+        .select((versions::all_columns, users::all_columns.nullable()))
+        .load(&*conn)?;
+    versions_and_publishers.sort_by(|a, b| b.0.num.cmp(&a.0.num));
+    let ids = versions_and_publishers.iter().map(|v| v.0.id).collect();
 
     let kws = CrateKeyword::belonging_to(&krate)
         .inner_join(keywords::table)
@@ -146,9 +151,9 @@ pub fn show(req: &mut dyn Request) -> CargoResult<Response> {
             false,
             recent_downloads,
         ),
-        versions: versions
+        versions: versions_and_publishers
             .into_iter()
-            .map(|v| v.encodable(&krate.name))
+            .map(|(v, pb)| v.encodable(&krate.name, pb))
             .collect(),
         keywords: kws.into_iter().map(|k| k.encodable()).collect(),
         categories: cats.into_iter().map(|k| k.encodable()).collect(),
@@ -185,11 +190,15 @@ pub fn versions(req: &mut dyn Request) -> CargoResult<Response> {
     let crate_name = &req.params()["crate_id"];
     let conn = req.db_conn()?;
     let krate = Crate::by_name(crate_name).first::<Crate>(&*conn)?;
-    let mut versions = krate.all_versions().load::<Version>(&*conn)?;
-    versions.sort_by(|a, b| b.num.cmp(&a.num));
-    let versions = versions
+    let mut versions_and_publishers: Vec<(Version, Option<User>)> = krate
+        .all_versions()
+        .left_outer_join(users::table)
+        .select((versions::all_columns, users::all_columns.nullable()))
+        .load(&*conn)?;
+    versions_and_publishers.sort_by(|a, b| b.0.num.cmp(&a.0.num));
+    let versions = versions_and_publishers
         .into_iter()
-        .map(|v| v.encodable(crate_name))
+        .map(|(v, pb)| v.encodable(crate_name, pb))
         .collect();
 
     #[derive(Serialize)]
@@ -218,10 +227,15 @@ pub fn reverse_dependencies(req: &mut dyn Request) -> CargoResult<Response> {
     let versions = versions::table
         .filter(versions::id.eq(any(version_ids)))
         .inner_join(crates::table)
-        .select((versions::all_columns, crates::name))
-        .load::<(Version, String)>(&*conn)?
+        .left_outer_join(users::table)
+        .select((
+            versions::all_columns,
+            crates::name,
+            users::all_columns.nullable(),
+        ))
+        .load::<(Version, String, Option<User>)>(&*conn)?
         .into_iter()
-        .map(|(version, krate_name)| version.encodable(&krate_name))
+        .map(|(version, krate_name, published_by)| version.encodable(&krate_name, published_by))
         .collect();
 
     #[derive(Serialize)]

src/controllers/krate/publish.rs

@@ -66,7 +66,8 @@ pub fn publish(req: &mut dyn Request) -> CargoResult<Response> {
     let conn = app.diesel_database.get()?;
 
     let mut other_warnings = vec![];
-    if !user.has_verified_email(&conn)? {
+    let verified_email_address = user.verified_email(&conn)?;
+    if verified_email_address.is_none() {
         other_warnings.push(String::from(
             "You do not currently have a verified email address associated with your crates.io \
              account. Starting 2019-02-28, a verified email will be required to publish crates. \
@@ -146,8 +147,9 @@ pub fn publish(req: &mut dyn Request) -> CargoResult<Response> {
             // Downcast is okay because the file length must be less than the max upload size
             // to get here, and max upload sizes are way less than i32 max
             file_length as i32,
+            user.id,
         )?
-        .save(&conn, &new_crate.authors)?;
+        .save(&conn, &new_crate.authors, verified_email_address)?;
 
         // Link this new version to all dependencies
         let git_deps = dependency::add_dependencies(&conn, &new_crate.deps, version.id)?;

src/controllers/user/me.rs

@@ -56,11 +56,16 @@ pub fn updates(req: &mut dyn Request) -> CargoResult<Response> {
     let followed_crates = Follow::belonging_to(user).select(follows::crate_id);
     let data = versions::table
         .inner_join(crates::table)
+        .left_outer_join(users::table)
         .filter(crates::id.eq(any(followed_crates)))
         .order(versions::created_at.desc())
-        .select((versions::all_columns, crates::name))
+        .select((
+            versions::all_columns,
+            crates::name,
+            users::all_columns.nullable(),
+        ))
         .paginate(limit, offset)
-        .load::<((Version, String), i64)>(&*conn)?;
+        .load::<((Version, String, Option<User>), i64)>(&*conn)?;
 
     let more = data
         .get(0)
@@ -69,7 +74,9 @@ pub fn updates(req: &mut dyn Request) -> CargoResult<Response> {
 
     let versions = data
         .into_iter()
-        .map(|((version, crate_name), _)| version.encodable(&crate_name))
+        .map(|((version, crate_name, published_by), _)| {
+            version.encodable(&crate_name, published_by)
+        })
         .collect();
 
     #[derive(Serialize)]

src/controllers/version/deprecated.rs

@@ -7,7 +7,7 @@
 
 use crate::controllers::prelude::*;
 
-use crate::models::{Crate, Version};
+use crate::models::{Crate, User, Version};
 use crate::schema::*;
 use crate::views::EncodableVersion;
 
@@ -24,11 +24,16 @@ pub fn index(req: &mut dyn Request) -> CargoResult<Response> {
 
     let versions = versions::table
         .inner_join(crates::table)
-        .select((versions::all_columns, crates::name))
+        .left_outer_join(users::table)
+        .select((
+            versions::all_columns,
+            crates::name,
+            users::all_columns.nullable(),
+        ))
         .filter(versions::id.eq(any(ids)))
-        .load::<(Version, String)>(&*conn)?
+        .load::<(Version, String, Option<User>)>(&*conn)?
         .into_iter()
-        .map(|(version, crate_name)| version.encodable(&crate_name))
+        .map(|(version, crate_name, published_by)| version.encodable(&crate_name, published_by))
         .collect();
 
     #[derive(Serialize)]
@@ -45,17 +50,22 @@ pub fn show_by_id(req: &mut dyn Request) -> CargoResult<Response> {
     let id = &req.params()["version_id"];
     let id = id.parse().unwrap_or(0);
     let conn = req.db_conn()?;
-    let (version, krate): (Version, Crate) = versions::table
+    let (version, krate, published_by): (Version, Crate, Option<User>) = versions::table
         .find(id)
         .inner_join(crates::table)
-        .select((versions::all_columns, crate::models::krate::ALL_COLUMNS))
+        .left_outer_join(users::table)
+        .select((
+            versions::all_columns,
+            crate::models::krate::ALL_COLUMNS,
+            users::all_columns.nullable(),
+        ))
         .first(&*conn)?;
 
     #[derive(Serialize)]
     struct R {
         version: EncodableVersion,
     }
     Ok(req.json(&R {
-        version: version.encodable(&krate.name),
+        version: version.encodable(&krate.name, published_by),
     }))
 }

src/controllers/version/metadata.rs

@@ -68,12 +68,14 @@ pub fn authors(req: &mut dyn Request) -> CargoResult<Response> {
 /// API route to have.
 pub fn show(req: &mut dyn Request) -> CargoResult<Response> {
     let (version, krate) = version_and_crate(req)?;
+    let conn = req.db_conn()?;
+    let published_by = version.published_by(&conn);
 
     #[derive(Serialize)]
     struct R {
         version: EncodableVersion,
     }
     Ok(req.json(&R {
-        version: version.encodable(&krate.name),
+        version: version.encodable(&krate.name, published_by),
     }))
 }

src/models/user.rs

@@ -5,7 +5,7 @@ use std::borrow::Cow;
 use crate::app::App;
 use crate::util::CargoResult;
 
-use crate::models::{Crate, CrateOwner, NewEmail, Owner, OwnerKind, Rights};
+use crate::models::{Crate, CrateOwner, Email, NewEmail, Owner, OwnerKind, Rights};
 use crate::schema::{crate_owners, emails, users};
 use crate::views::{EncodablePrivateUser, EncodablePublicUser};
 
@@ -160,15 +160,12 @@ impl User {
         Ok(best)
     }
 
-    pub fn has_verified_email(&self, conn: &PgConnection) -> CargoResult<bool> {
-        use diesel::dsl::exists;
-        let email_exists = diesel::select(exists(
-            emails::table
-                .filter(emails::user_id.eq(self.id))
-                .filter(emails::verified.eq(true)),
-        ))
-        .get_result(&*conn)?;
-        Ok(email_exists)
+    pub fn verified_email(&self, conn: &PgConnection) -> CargoResult<Option<String>> {
+        Ok(Email::belonging_to(self)
+            .select(emails::email)
+            .filter(emails::verified.eq(true))
+            .first::<String>(&*conn)
+            .optional()?)
     }
 
     /// Converts this `User` model into an `EncodablePrivateUser` for JSON serialization.

src/models/version.rs

@@ -5,7 +5,7 @@ use diesel::prelude::*;
 
 use crate::util::{human, CargoResult};
 
-use crate::models::{Crate, Dependency};
+use crate::models::{Crate, Dependency, User};
 use crate::schema::*;
 use crate::views::{EncodableVersion, EncodableVersionLinks};
 
@@ -23,6 +23,7 @@ pub struct Version {
     pub yanked: bool,
     pub license: Option<String>,
     pub crate_size: Option<i32>,
+    pub published_by: Option<i32>,
 }
 
 #[derive(Insertable, Debug)]
@@ -33,10 +34,11 @@ pub struct NewVersion {
     features: serde_json::Value,
     license: Option<String>,
     crate_size: Option<i32>,
+    published_by: i32,
 }
 
 impl Version {
-    pub fn encodable(self, crate_name: &str) -> EncodableVersion {
+    pub fn encodable(self, crate_name: &str, published_by: Option<User>) -> EncodableVersion {
         let Version {
             id,
             num,
@@ -68,6 +70,7 @@ impl Version {
                 authors: format!("/api/v1/crates/{}/{}/authors", crate_name, num),
             },
             crate_size,
+            published_by: published_by.map(|pb| pb.encodable_public()),
         }
     }
 
@@ -107,6 +110,15 @@ impl Version {
             .set(rendered_at.eq(now))
             .execute(conn)
     }
+
+    /// Gets the User who ran `cargo publish` for this version, if recorded.
+    /// Not for use when you have a group of versions you need the publishers for.
+    pub fn published_by(&self, conn: &PgConnection) -> Option<User> {
+        match self.published_by {
+            Some(pb) => users::table.find(pb).first(conn).ok(),
+            None => None,
+        }
+    }
 }
 
 impl NewVersion {
@@ -118,6 +130,7 @@ impl NewVersion {
         license: Option<String>,
         license_file: Option<&str>,
         crate_size: i32,
+        published_by: i32,
     ) -> CargoResult<Self> {
         let features = serde_json::to_value(features)?;
 
@@ -127,14 +140,21 @@ impl NewVersion {
             features,
             license,
             crate_size: Some(crate_size),
+            published_by,
         };
 
         new_version.validate_license(license_file)?;
 
         Ok(new_version)
     }
 
-    pub fn save(&self, conn: &PgConnection, authors: &[String]) -> CargoResult<Version> {
+    // TODO: change `published_by_email` to be `String` after 2019-02-28
+    pub fn save(
+        &self,
+        conn: &PgConnection,
+        authors: &[String],
+        published_by_email: Option<String>,
+    ) -> CargoResult<Version> {
         use crate::schema::version_authors::{name, version_id};
         use crate::schema::versions::dsl::*;
         use diesel::dsl::exists;
@@ -156,6 +176,16 @@ impl NewVersion {
                 .values(self)
                 .get_result::<Version>(conn)?;
 
+            // TODO: Remove the `if` around this insert after 2019-02-28
+            if let Some(published_by_email) = published_by_email {
+                insert_into(versions_published_by::table)
+                    .values((
+                        versions_published_by::version_id.eq(version.id),
+                        versions_published_by::email.eq(published_by_email),
+                    ))
+                    .execute(conn)?;
+            }
+
             let new_authors = authors
                 .iter()
                 .map(|s| (version_id.eq(version.id), name.eq(s)))