Merge pull request #5538 from Turbo87/auth-check

auth: Implement AuthCheck struct

Author: Turbo87

src/auth.rs

@@ -10,6 +10,52 @@ use conduit::RequestExt;
 use conduit_cookie::RequestSession;
 use http::header;
 
+#[derive(Debug, Clone)]
+pub struct AuthCheck {
+    allow_token: bool,
+}
+
+impl AuthCheck {
+    #[must_use]
+    pub fn default() -> Self {
+        Self { allow_token: true }
+    }
+
+    #[must_use]
+    pub fn only_cookie() -> Self {
+        Self { allow_token: false }
+    }
+
+    pub fn check(&self, request: &dyn RequestExt) -> AppResult<AuthenticatedUser> {
+        controllers::util::verify_origin(request)?;
+
+        let auth = authenticate_user(request)?;
+
+        if let Some(reason) = &auth.user.account_lock_reason {
+            let still_locked = if let Some(until) = auth.user.account_lock_until {
+                until > Utc::now().naive_utc()
+            } else {
+                true
+            };
+            if still_locked {
+                return Err(account_locked(reason, auth.user.account_lock_until));
+            }
+        }
+
+        log_request::add_custom_metadata("uid", auth.user_id());
+        if let Some(id) = auth.api_token_id() {
+            log_request::add_custom_metadata("tokenid", id);
+        }
+
+        if !self.allow_token && auth.token_id.is_some() {
+            let error_message = "API Token authentication was explicitly disallowed for this API";
+            return Err(internal(error_message).chain(forbidden()));
+        }
+
+        Ok(auth)
+    }
+}
+
 #[derive(Debug)]
 pub struct AuthenticatedUser {
     user: User,
@@ -28,18 +74,6 @@ 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()),
-            )
-        }
-    }
 }
 
 fn authenticate_user(req: &dyn RequestExt) -> AppResult<AuthenticatedUser> {
@@ -85,37 +119,3 @@ fn authenticate_user(req: &dyn RequestExt) -> AppResult<AuthenticatedUser> {
     // Unable to authenticate the user
     return Err(internal("no cookie session or auth header found").chain(forbidden()));
 }
-
-pub trait UserAuthenticationExt {
-    fn authenticate(&self) -> AppResult<AuthenticatedUser>;
-}
-
-impl<'a> UserAuthenticationExt for dyn RequestExt + 'a {
-    /// Obtain `AuthenticatedUser` for the request or return an `Forbidden` error
-    fn authenticate(&self) -> AppResult<AuthenticatedUser> {
-        controllers::util::verify_origin(self)?;
-
-        let authenticated_user = authenticate_user(self)?;
-
-        if let Some(reason) = &authenticated_user.user.account_lock_reason {
-            let still_locked = if let Some(until) = authenticated_user.user.account_lock_until {
-                until > Utc::now().naive_utc()
-            } else {
-                true
-            };
-            if still_locked {
-                return Err(account_locked(
-                    reason,
-                    authenticated_user.user.account_lock_until,
-                ));
-            }
-        }
-
-        log_request::add_custom_metadata("uid", authenticated_user.user_id());
-        if let Some(id) = authenticated_user.api_token_id() {
-            log_request::add_custom_metadata("tokenid", id);
-        }
-
-        Ok(authenticated_user)
-    }
-}

src/controllers.rs

@@ -15,7 +15,6 @@ mod prelude {
     pub use conduit::{header, RequestExt, StatusCode};
     pub use conduit_router::RequestParams;
 
-    pub use crate::auth::UserAuthenticationExt;
     pub use crate::db::RequestTransaction;
     pub use crate::middleware::app::RequestApp;
     pub use crate::util::errors::{cargo_err, AppError, AppResult}; // TODO: Remove cargo_err from here

src/controllers/crate_owner_invitation.rs

@@ -1,5 +1,6 @@
 use super::frontend_prelude::*;
 
+use crate::auth::AuthCheck;
 use crate::auth::AuthenticatedUser;
 use crate::controllers::helpers::pagination::{Page, PaginationOptions};
 use crate::models::{Crate, CrateOwnerInvitation, Rights, User};
@@ -16,7 +17,7 @@ use std::collections::{HashMap, HashSet};
 
 /// Handles the `GET /api/v1/me/crate_owner_invitations` route.
 pub fn list(req: &mut dyn RequestExt) -> EndpointResult {
-    let auth = req.authenticate()?.forbid_api_token_auth()?;
+    let auth = AuthCheck::only_cookie().check(req)?;
     let user_id = auth.user_id();
 
     let PrivateListResponse {
@@ -52,7 +53,7 @@ pub fn list(req: &mut dyn RequestExt) -> EndpointResult {
 
 /// Handles the `GET /api/private/crate_owner_invitations` route.
 pub fn private_list(req: &mut dyn RequestExt) -> EndpointResult {
-    let auth = req.authenticate()?.forbid_api_token_auth()?;
+    let auth = AuthCheck::only_cookie().check(req)?;
 
     let filter = if let Some(crate_name) = req.query().get("crate_name") {
         ListFilter::CrateName(crate_name.clone())
@@ -255,7 +256,9 @@ pub fn handle_invite(req: &mut dyn RequestExt) -> EndpointResult {
         serde_json::from_str(&body).map_err(|_| bad_request("invalid json request"))?;
 
     let crate_invite = crate_invite.crate_owner_invite;
-    let user_id = req.authenticate()?.user_id();
+
+    let auth = AuthCheck::default().check(req)?;
+    let user_id = auth.user_id();
     let conn = &*req.db_write()?;
     let config = &req.app().config;
 

src/controllers/krate/follow.rs

@@ -1,5 +1,6 @@
 //! Endpoints for managing a per user list of followed crates
 
+use crate::auth::AuthCheck;
 use diesel::associations::Identifiable;
 
 use crate::controllers::frontend_prelude::*;
@@ -21,7 +22,7 @@ fn follow_target(
 
 /// Handles the `PUT /crates/:crate_id/follow` route.
 pub fn follow(req: &mut dyn RequestExt) -> EndpointResult {
-    let user_id = req.authenticate()?.user_id();
+    let user_id = AuthCheck::default().check(req)?.user_id();
     let conn = req.db_write()?;
     let follow = follow_target(req, &conn, user_id)?;
     diesel::insert_into(follows::table)
@@ -34,7 +35,7 @@ pub fn follow(req: &mut dyn RequestExt) -> EndpointResult {
 
 /// Handles the `DELETE /crates/:crate_id/follow` route.
 pub fn unfollow(req: &mut dyn RequestExt) -> EndpointResult {
-    let user_id = req.authenticate()?.user_id();
+    let user_id = AuthCheck::default().check(req)?.user_id();
     let conn = req.db_write()?;
     let follow = follow_target(req, &conn, user_id)?;
     diesel::delete(&follow).execute(&*conn)?;
@@ -46,7 +47,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()?.forbid_api_token_auth()?.user_id();
+    let user_id = AuthCheck::only_cookie().check(req)?.user_id();
     let conn = req.db_read_prefer_primary()?;
     let follow = follow_target(req, &conn, user_id)?;
     let following =

src/controllers/krate/owners.rs

@@ -1,5 +1,6 @@
 //! All routes related to managing owners of a crate
 
+use crate::auth::AuthCheck;
 use crate::controllers::prelude::*;
 use crate::models::{Crate, Owner, Rights, Team, User};
 use crate::views::EncodableOwner;
@@ -79,13 +80,14 @@ fn parse_owners_request(req: &mut dyn RequestExt) -> AppResult<Vec<String>> {
 }
 
 fn modify_owners(req: &mut dyn RequestExt, add: bool) -> EndpointResult {
-    let authenticated_user = req.authenticate()?;
+    let auth = AuthCheck::default().check(req)?;
+
     let logins = parse_owners_request(req)?;
     let app = req.app();
     let crate_name = &req.params()["crate_id"];
 
     let conn = req.db_write()?;
-    let user = authenticated_user.user();
+    let user = auth.user();
 
     conn.transaction(|| {
         let krate: Crate = Crate::by_name(crate_name).first(&*conn)?;

src/controllers/krate/publish.rs

@@ -1,5 +1,6 @@
 //! Functionality related to publishing a new crate or version of a crate.
 
+use crate::auth::AuthCheck;
 use flate2::read::GzDecoder;
 use hex::ToHex;
 use sha2::{Digest, Sha256};
@@ -64,9 +65,9 @@ pub fn publish(req: &mut dyn RequestExt) -> EndpointResult {
     add_custom_metadata("crate_version", new_crate.vers.to_string());
 
     let conn = app.primary_database.get()?;
-    let ids = req.authenticate()?;
-    let api_token_id = ids.api_token_id();
-    let user = ids.user();
+    let auth = AuthCheck::default().check(req)?;
+    let api_token_id = auth.api_token_id();
+    let user = auth.user();
 
     let verified_email_address = user.verified_email(&conn)?;
     let verified_email_address = verified_email_address.ok_or_else(|| {

src/controllers/krate/search.rs

@@ -1,5 +1,6 @@
 //! Endpoint for searching and discovery functionality
 
+use crate::auth::AuthCheck;
 use diesel::dsl::*;
 use diesel::sql_types::Array;
 use diesel_full_text_search::*;
@@ -186,7 +187,8 @@ pub fn search(req: &mut dyn RequestExt) -> EndpointResult {
         // Calculating the total number of results with filters is not supported yet.
         supports_seek = false;
 
-        let user_id = req.authenticate()?.user_id();
+        let user_id = AuthCheck::default().check(req)?.user_id();
+
         query = query.filter(
             crates::id.eq_any(
                 follows::table

src/controllers/token.rs

@@ -5,14 +5,15 @@ use crate::schema::api_tokens;
 use crate::util::read_fill;
 use crate::views::EncodableApiTokenWithToken;
 
+use crate::auth::AuthCheck;
 use conduit::{Body, Response};
 use serde_json as json;
 
 /// Handles the `GET /me/tokens` route.
 pub fn list(req: &mut dyn RequestExt) -> EndpointResult {
-    let authenticated_user = req.authenticate()?.forbid_api_token_auth()?;
+    let auth = AuthCheck::only_cookie().check(req)?;
     let conn = req.db_read_prefer_primary()?;
-    let user = authenticated_user.user();
+    let user = auth.user();
 
     let tokens: Vec<ApiToken> = ApiToken::belonging_to(&user)
         .filter(api_tokens::revoked.eq(false))
@@ -59,15 +60,15 @@ pub fn new(req: &mut dyn RequestExt) -> EndpointResult {
         return Err(bad_request("name must have a value"));
     }
 
-    let authenticated_user = req.authenticate()?;
-    if authenticated_user.api_token_id().is_some() {
+    let auth = AuthCheck::default().check(req)?;
+    if auth.api_token_id().is_some() {
         return Err(bad_request(
             "cannot use an API token to create a new API token",
         ));
     }
 
     let conn = req.db_write()?;
-    let user = authenticated_user.user();
+    let user = auth.user();
 
     let max_token_per_user = 500;
     let count: i64 = ApiToken::belonging_to(&user).count().get_result(&*conn)?;
@@ -90,9 +91,9 @@ pub fn revoke(req: &mut dyn RequestExt) -> EndpointResult {
         .parse::<i32>()
         .map_err(|e| bad_request(&format!("invalid token id: {e:?}")))?;
 
-    let authenticated_user = req.authenticate()?;
+    let auth = AuthCheck::default().check(req)?;
     let conn = req.db_write()?;
-    let user = authenticated_user.user();
+    let user = auth.user();
     diesel::update(ApiToken::belonging_to(&user).find(id))
         .set(api_tokens::revoked.eq(true))
         .execute(&*conn)?;
@@ -102,8 +103,8 @@ pub fn revoke(req: &mut dyn RequestExt) -> EndpointResult {
 
 /// Handles the `DELETE /tokens/current` route.
 pub fn revoke_current(req: &mut dyn RequestExt) -> EndpointResult {
-    let authenticated_user = req.authenticate()?;
-    let api_token_id = authenticated_user
+    let auth = AuthCheck::default().check(req)?;
+    let api_token_id = auth
         .api_token_id()
         .ok_or_else(|| bad_request("token not provided"))?;
 

src/controllers/user/me.rs

@@ -1,3 +1,4 @@
+use crate::auth::AuthCheck;
 use std::collections::HashMap;
 
 use crate::controllers::frontend_prelude::*;
@@ -13,7 +14,7 @@ use crate::views::{EncodableMe, EncodablePrivateUser, EncodableVersion, OwnedCra
 
 /// Handles the `GET /me` route.
 pub fn me(req: &mut dyn RequestExt) -> EndpointResult {
-    let user_id = req.authenticate()?.forbid_api_token_auth()?.user_id();
+    let user_id = AuthCheck::only_cookie().check(req)?.user_id();
     let conn = req.db_read_prefer_primary()?;
 
     let (user, verified, email, verification_sent): (User, Option<bool>, Option<String>, bool) =
@@ -54,8 +55,8 @@ 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()?.forbid_api_token_auth()?;
-    let user = authenticated_user.user();
+    let auth = AuthCheck::only_cookie().check(req)?;
+    let user = auth.user();
 
     let followed_crates = Follow::belonging_to(&user).select(follows::crate_id);
     let query = versions::table
@@ -96,14 +97,14 @@ pub fn update_user(req: &mut dyn RequestExt) -> EndpointResult {
     use self::emails::user_id;
     use diesel::insert_into;
 
-    let authenticated_user = req.authenticate()?;
+    let auth = AuthCheck::default().check(req)?;
 
     let mut body = String::new();
     req.body().read_to_string(&mut body)?;
 
     let param_user_id = &req.params()["user_id"];
     let conn = req.db_write()?;
-    let user = authenticated_user.user();
+    let user = auth.user();
 
     // need to check if current user matches user to be updated
     if &user.id.to_string() != param_user_id {
@@ -188,9 +189,11 @@ pub fn regenerate_token_and_send(req: &mut dyn RequestExt) -> EndpointResult {
     let param_user_id = req.params()["user_id"]
         .parse::<i32>()
         .map_err(|err| err.chain(bad_request("invalid user_id")))?;
-    let authenticated_user = req.authenticate()?;
+
+    let auth = AuthCheck::default().check(req)?;
+
     let conn = req.db_write()?;
-    let user = authenticated_user.user();
+    let user = auth.user();
 
     // need to check if current user matches user to be updated
     if user.id != param_user_id {
@@ -230,7 +233,7 @@ pub fn update_email_notifications(req: &mut dyn RequestExt) -> EndpointResult {
         .map(|c| (c.id, c.email_notifications))
         .collect();
 
-    let user_id = req.authenticate()?.user_id();
+    let user_id = AuthCheck::default().check(req)?.user_id();
     let conn = req.db_write()?;
 
     // Build inserts from existing crates belonging to the current user

src/controllers/version/yank.rs

@@ -1,5 +1,6 @@
 //! Endpoints for yanking and unyanking specific versions of crates
 
+use crate::auth::AuthCheck;
 use swirl::Job;
 
 use super::{extract_crate_name_and_semver, version_and_crate};
@@ -31,13 +32,13 @@ pub fn unyank(req: &mut dyn RequestExt) -> EndpointResult {
 fn modify_yank(req: &mut dyn RequestExt, yanked: bool) -> EndpointResult {
     // FIXME: Should reject bad requests before authentication, but can't due to
     // lifetime issues with `req`.
-    let authenticated_user = req.authenticate()?;
+    let auth = AuthCheck::default().check(req)?;
     let (crate_name, semver) = extract_crate_name_and_semver(req)?;
 
     let conn = req.db_write()?;
     let (version, krate) = version_and_crate(&conn, crate_name, semver)?;
-    let api_token_id = authenticated_user.api_token_id();
-    let user = authenticated_user.user();
+    let api_token_id = auth.api_token_id();
+    let user = auth.user();
     let owners = krate.owners(&conn)?;
 
     if user.rights(req.app(), &owners)? < Rights::Publish {