Merge #1567

1567: Mark API tokens as revoked r=carols10cents a=joshleeb

With this PR, when a user deletes an API token, rather than deleting it from the database, it will instead be marked as `revoked`.

That means that this PR also adds a migration to add the column `revoked` to the `api_tokens` table.

Ref. #1548 (Task 1)

Co-authored-by: Josh Leeb-du Toit <[email protected]>

Author: bors-voyager[bot]

migrations/2018-11-30-090110_add_revoked_to_api_tokens/down.sql

@@ -0,0 +1 @@
+ALTER TABLE api_tokens DROP COLUMN revoked;

migrations/2018-11-30-090110_add_revoked_to_api_tokens/up.sql

@@ -0,0 +1 @@
+ALTER TABLE api_tokens ADD COLUMN revoked BOOLEAN NOT NULL DEFAULT 'f';

src/controllers/token.rs

@@ -12,6 +12,7 @@ use views::EncodableApiTokenWithToken;
 /// Handles the `GET /me/tokens` route.
 pub fn list(req: &mut dyn Request) -> CargoResult<Response> {
     let tokens = ApiToken::belonging_to(req.user()?)
+        .filter(api_tokens::revoked.eq(false))
         .order(api_tokens::created_at.desc())
         .load(&*req.db_conn()?)?;
     #[derive(Serialize)]
@@ -94,7 +95,9 @@ pub fn revoke(req: &mut dyn Request) -> CargoResult<Response> {
         .parse::<i32>()
         .map_err(|e| bad_request(&format!("invalid token id: {:?}", e)))?;
 
-    diesel::delete(ApiToken::belonging_to(req.user()?).find(id)).execute(&*req.db_conn()?)?;
+    diesel::update(ApiToken::belonging_to(req.user()?).find(id))
+        .set(api_tokens::revoked.eq(true))
+        .execute(&*req.db_conn()?)?;
 
     #[derive(Serialize)]
     struct R {}

src/models/token.rs

@@ -21,6 +21,8 @@ pub struct ApiToken {
     pub created_at: NaiveDateTime,
     #[serde(with = "rfc3339::option")]
     pub last_used_at: Option<NaiveDateTime>,
+    #[serde(skip)]
+    pub revoked: bool,
 }
 
 impl ApiToken {
@@ -40,6 +42,7 @@ impl ApiToken {
             id: self.id,
             name: self.name,
             token: self.token,
+            revoked: self.revoked,
             created_at: self.created_at,
             last_used_at: self.last_used_at,
         }
@@ -58,6 +61,7 @@ mod tests {
             id: 12345,
             user_id: 23456,
             token: "".to_string(),
+            revoked: false,
             name: "".to_string(),
             created_at: NaiveDate::from_ymd(2017, 1, 6).and_hms(14, 23, 11),
             last_used_at: Some(NaiveDate::from_ymd(2017, 1, 6).and_hms(14, 23, 12)),
@@ -81,6 +85,7 @@ mod tests {
             id: 12345,
             name: "".to_string(),
             token: "".to_string(),
+            revoked: false,
             created_at: NaiveDate::from_ymd(2017, 1, 6).and_hms(14, 23, 11),
             last_used_at: Some(NaiveDate::from_ymd(2017, 1, 6).and_hms(14, 23, 12)),
         };

src/models/user.rs

@@ -109,9 +109,12 @@ impl User {
     /// Queries the database for a user with a certain `api_token` value.
     pub fn find_by_api_token(conn: &PgConnection, token_: &str) -> CargoResult<User> {
         use diesel::update;
-        use schema::api_tokens::dsl::{api_tokens, last_used_at, token, user_id};
+        use schema::api_tokens::dsl::{api_tokens, last_used_at, revoked, token, user_id};
         use schema::users::dsl::{id, users};
-        let user_id_ = update(api_tokens.filter(token.eq(token_)))
+        let tokens = api_tokens
+            .filter(token.eq(token_))
+            .filter(revoked.eq(false));
+        let user_id_ = update(tokens)
             .set(last_used_at.eq(now.nullable()))
             .returning(user_id)
             .get_result::<i32>(conn)?;

src/schema.rs

@@ -45,6 +45,12 @@ table! {
         ///
         /// (Automatically generated by Diesel.)
         last_used_at -> Nullable<Timestamp>,
+        /// The `revoked` column of the `api_tokens` table.
+        ///
+        /// Its SQL type is `Bool`.
+        ///
+        /// (Automatically generated by Diesel.)
+        revoked -> Bool,
     }
 }
 

src/tests/token.rs

@@ -3,6 +3,7 @@ use std::collections::HashSet;
 use diesel::prelude::*;
 
 use models::ApiToken;
+use schema::api_tokens;
 use views::{EncodableApiTokenWithToken, EncodableMe};
 use {user::UserShowPrivateResponse, RequestHelper, TestApp};
 
@@ -69,6 +70,37 @@ fn list_tokens() {
     );
 }
 
+#[test]
+fn list_tokens_exclude_revoked() {
+    let (app, _, user) = TestApp::init().with_user();
+    let id = user.as_model().id;
+    let tokens = app.db(|conn| {
+        vec![
+            t!(ApiToken::insert(conn, id, "bar")),
+            t!(ApiToken::insert(conn, id, "baz")),
+        ]
+    });
+
+    // List tokens expecting them all to be there.
+    let json: ListResponse = user.get(URL).good();
+    assert_eq!(json.api_tokens.len(), tokens.len());
+
+    // Revoke the first token.
+    let _json: RevokedResponse = user
+        .delete(&format!("/api/v1/me/tokens/{}", tokens[0].id))
+        .good();
+
+    // Check that we now have one less token being listed.
+    let json: ListResponse = user.get(URL).good();
+    assert_eq!(json.api_tokens.len(), tokens.len() - 1);
+    assert!(
+        json.api_tokens
+            .iter()
+            .find(|token| token.name == tokens[0].name)
+            .is_none()
+    );
+}
+
 #[test]
 fn create_token_logged_out() {
     let (_, anon) = TestApp::init().empty();
@@ -128,6 +160,7 @@ fn create_token_success() {
     assert_eq!(tokens.len(), 1);
     assert_eq!(tokens[0].name, "bar");
     assert_eq!(tokens[0].token, json.api_token.token);
+    assert_eq!(tokens[0].revoked, false);
     assert_eq!(tokens[0].last_used_at, None);
 }
 
@@ -218,6 +251,7 @@ fn revoke_token_success() {
     // List tokens no longer contains the token
     app.db(|conn| {
         let count = ApiToken::belonging_to(user.as_model())
+            .filter(api_tokens::revoked.eq(false))
             .count()
             .get_result(conn);
         assert_eq!(count, Ok(0));

src/views/mod.rs

@@ -144,6 +144,7 @@ pub struct EncodableApiTokenWithToken {
     pub id: i32,
     pub name: String,
     pub token: String,
+    pub revoked: bool,
     #[serde(with = "rfc3339")]
     pub created_at: NaiveDateTime,
     #[serde(with = "rfc3339::option")]