Auto merge of #3470 - jakeswenson:restrict-api-token-access-on-GET-requests, r=pietroalbini

Add API Token Auth restrictions on GET APIs

This PR adds explicit denial of API token authentication on most GET APIs.
Currently, CloudFront was never passing along the `Authorization` header for `GET` requests.
(See rust-lang/simpleinfra#43 for more details.)

In addressing that issue it was pointed out that there may be GET APIs that the team might not want to commit to allowing API tokens access to.

This PR attempts to limit the exposed APIs to:
- `/api/v1/crates?following=1` (the crates "search" API, which is the only way to get the list of crates a user is following.)
- `/api/v1/me`
  > Note: There were already [tests verifying token access to this API][me-tests], but with the CloudFront issue this API was not actually accessible.

[me-tests]: https://github.com/rust-lang/crates.io/blob/03f7b273eb7a024de8282ac2297daaa47df33fa6/src/tests/token.rs#L270

Author: bors

src/controllers/crate_owner_invitation.rs

@@ -9,7 +9,7 @@ use std::collections::HashMap;
 /// Handles the `GET /me/crate_owner_invitations` route.
 pub fn list(req: &mut dyn RequestExt) -> EndpointResult {
     // Ensure that the user is authenticated
-    let user = req.authenticate()?.user();
+    let user = req.authenticate()?.forbid_api_token_auth()?.user();
 
     // Load all pending invitations for the user
     let conn = &*req.db_read_only()?;

src/controllers/krate/follow.rs

@@ -46,7 +46,7 @@ pub fn unfollow(req: &mut dyn RequestExt) -> EndpointResult {
 pub fn following(req: &mut dyn RequestExt) -> EndpointResult {
     use diesel::dsl::exists;
 
-    let user_id = req.authenticate()?.user_id();
+    let user_id = req.authenticate()?.forbid_api_token_auth()?.user_id();
     let conn = req.db_read_only()?;
     let follow = follow_target(req, &conn, user_id)?;
     let following = diesel::select(exists(follows::table.find(follow.id()))).get_result(&*conn)?;

src/controllers/token.rs

@@ -9,7 +9,7 @@ use serde_json as json;
 
 /// Handles the `GET /me/tokens` route.
 pub fn list(req: &mut dyn RequestExt) -> EndpointResult {
-    let authenticated_user = req.authenticate()?;
+    let authenticated_user = req.authenticate()?.forbid_api_token_auth()?;
     let conn = req.db_conn()?;
     let user = authenticated_user.user();
 

src/controllers/user/me.rs

@@ -54,7 +54,7 @@ pub fn me(req: &mut dyn RequestExt) -> EndpointResult {
 pub fn updates(req: &mut dyn RequestExt) -> EndpointResult {
     use diesel::dsl::any;
 
-    let authenticated_user = req.authenticate()?;
+    let authenticated_user = req.authenticate()?.forbid_api_token_auth()?;
     let user = authenticated_user.user();
 
     let followed_crates = Follow::belonging_to(&user).select(follows::crate_id);

src/controllers/util.rs

@@ -28,6 +28,18 @@ impl AuthenticatedUser {
     pub fn user(self) -> User {
         self.user
     }
+
+    /// Disallows token authenticated users
+    pub fn forbid_api_token_auth(self) -> AppResult<Self> {
+        if self.token_id.is_none() {
+            Ok(self)
+        } else {
+            Err(
+                internal("API Token authentication was explicitly disallowed for this API")
+                    .chain(forbidden()),
+            )
+        }
+    }
 }
 
 /// The Origin header (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Origin)

src/tests/krate/following.rs

@@ -57,3 +57,63 @@ fn following() {
     assert!(!is_following());
     assert_eq!(user.search("following=1").crates.len(), 0);
 }
+
+#[test]
+fn disallow_api_token_auth_for_get_crate_following_status() {
+    let (app, _, _, token) = TestApp::init().with_token();
+    let api_token = token.as_model();
+
+    let a_crate = "a_crate";
+
+    app.db(|conn| {
+        CrateBuilder::new(a_crate, api_token.user_id).expect_build(conn);
+    });
+
+    // Token auth on GET for get following status is disallowed
+    token
+        .get(&format!("/api/v1/crates/{}/following", a_crate))
+        .assert_forbidden();
+}
+
+#[test]
+fn getting_followed_crates_allows_api_token_auth() {
+    let (app, _, user, token) = TestApp::init().with_token();
+    let api_token = token.as_model();
+
+    let crate_to_follow = "some_crate_to_follow";
+    let crate_not_followed = "another_crate";
+
+    app.db(|conn| {
+        CrateBuilder::new(crate_to_follow, api_token.user_id).expect_build(conn);
+        CrateBuilder::new(crate_not_followed, api_token.user_id).expect_build(conn);
+    });
+
+    let is_following = |crate_name: &str| -> bool {
+        #[derive(Deserialize)]
+        struct F {
+            following: bool,
+        }
+
+        // Token auth on GET for get following status is disallowed
+        user.get::<F>(&format!("/api/v1/crates/{}/following", crate_name))
+            .good()
+            .following
+    };
+
+    let follow = |crate_name: &str| {
+        assert!(
+            token
+                .put::<OkBool>(&format!("/api/v1/crates/{}/follow", crate_name), b"")
+                .good()
+                .ok
+        );
+    };
+
+    follow(crate_to_follow);
+
+    assert!(is_following(crate_to_follow));
+    assert!(!is_following(crate_not_followed));
+
+    let json = token.search("following=1");
+    assert_eq!(json.crates.len(), 1);
+}

src/tests/owners.rs

@@ -421,6 +421,15 @@ fn invitations_are_empty_by_default() {
     assert_eq!(json.crate_owner_invitations.len(), 0);
 }
 
+#[test]
+fn api_token_cannot_list_invitations() {
+    let (_, _, _, token) = TestApp::init().with_token();
+
+    token
+        .get("/api/v1/me/crate_owner_invitations")
+        .assert_forbidden();
+}
+
 #[test]
 fn invitations_list() {
     let (app, _, owner, token) = TestApp::init().with_token();

src/tests/token.rs

@@ -36,6 +36,12 @@ fn list_logged_out() {
     anon.get(URL).assert_forbidden();
 }
 
+#[test]
+fn list_with_api_token_is_forbidden() {
+    let (_, _, _, token) = TestApp::init().with_token();
+    token.get(URL).assert_forbidden();
+}
+
 #[test]
 fn list_empty() {
     let (_, _, user) = TestApp::init().with_user();

src/tests/user.rs

@@ -206,6 +206,12 @@ fn crates_by_user_id_not_including_deleted_owners() {
     assert_eq!(response.crates.len(), 0);
 }
 
+#[test]
+fn api_token_cannot_get_user_updates() {
+    let (_, _, _, token) = TestApp::init().with_token();
+    token.get("/api/v1/me/updates").assert_forbidden();
+}
+
 #[test]
 fn following() {
     use cargo_registry::schema::versions;