From 2d45799b7d5e8bd577308305eb27fe49e198c546 Mon Sep 17 00:00:00 2001
From: Scott Lamb <slamb@slamb.org>
Date: Sat, 6 Mar 2021 05:16:09 -0800
Subject: [PATCH] better error handling for authentication

---
 server/base/error.rs |   9 ++++
 server/db/auth.rs    | 113 ++++++++++++++++++++++++++++---------------
 server/db/db.rs      |   2 +-
 server/src/web.rs    |  42 +++++++++++-----
 4 files changed, 115 insertions(+), 51 deletions(-)

diff --git a/server/base/error.rs b/server/base/error.rs
index cc34968..bb06813 100644
--- a/server/base/error.rs
+++ b/server/base/error.rs
@@ -36,6 +36,15 @@ impl Error {
     pub fn compat(self) -> failure::Compat<Context<ErrorKind>> {
         self.inner.compat()
     }
+
+    pub fn map<F>(self, op: F) -> Self
+    where
+        F: FnOnce(ErrorKind) -> ErrorKind,
+    {
+        Self {
+            inner: self.inner.map(op),
+        }
+    }
 }
 
 impl Fail for Error {
diff --git a/server/db/auth.rs b/server/db/auth.rs
index 8a5b9ce..1928d46 100644
--- a/server/db/auth.rs
+++ b/server/db/auth.rs
@@ -3,7 +3,7 @@
 // SPDX-License-Identifier: GPL-v3.0-or-later WITH GPL-3.0-linking-exception.
 
 use crate::schema::Permissions;
-use base::strutil;
+use base::{bail_t, format_err_t, strutil, ErrorKind, ResultExt};
 use failure::{bail, format_err, Error};
 use fnv::FnvHashMap;
 use lazy_static::lazy_static;
@@ -678,23 +678,31 @@ impl State {
         conn: &Connection,
         req: Request,
         hash: &SessionHash,
-    ) -> Result<(&Session, &User), Error> {
+    ) -> Result<(&Session, &User), base::Error> {
         let s = match self.sessions.entry(*hash) {
             ::std::collections::hash_map::Entry::Occupied(e) => e.into_mut(),
-            ::std::collections::hash_map::Entry::Vacant(e) => e.insert(lookup_session(conn, hash)?),
+            ::std::collections::hash_map::Entry::Vacant(e) => {
+                let s = lookup_session(conn, hash).map_err(|e| {
+                    e.map(|k| match k {
+                        ErrorKind::NotFound => ErrorKind::Unauthenticated,
+                        e => e,
+                    })
+                })?;
+                e.insert(s)
+            }
         };
         let u = match self.users_by_id.get(&s.user_id) {
-            None => bail!("session references nonexistent user!"),
+            None => bail_t!(Internal, "session references nonexistent user!"),
             Some(u) => u,
         };
         if let Some(r) = s.revocation_reason {
-            bail!("session is no longer valid (reason={})", r);
+            bail_t!(Unauthenticated, "session is no longer valid (reason={})", r);
         }
         s.last_use = req;
         s.use_count += 1;
         s.dirty = true;
         if u.disabled() {
-            bail!("user {:?} is disabled", &u.username);
+            bail_t!(Unauthenticated, "user {:?} is disabled", &u.username);
         }
         Ok((s, u))
     }
@@ -811,9 +819,10 @@ impl State {
     }
 }
 
-fn lookup_session(conn: &Connection, hash: &SessionHash) -> Result<Session, Error> {
-    let mut stmt = conn.prepare_cached(
-        r#"
+fn lookup_session(conn: &Connection, hash: &SessionHash) -> Result<Session, base::Error> {
+    let mut stmt = conn
+        .prepare_cached(
+            r#"
         select
             user_id,
             seed,
@@ -839,39 +848,52 @@ fn lookup_session(conn: &Connection, hash: &SessionHash) -> Result<Session, Erro
         where
             session_id_hash = ?
         "#,
-    )?;
-    let mut rows = stmt.query(params![&hash.0[..]])?;
-    let row = rows.next()?.ok_or_else(|| format_err!("no such session"))?;
-    let creation_addr: FromSqlIpAddr = row.get(8)?;
-    let revocation_addr: FromSqlIpAddr = row.get(11)?;
-    let last_use_addr: FromSqlIpAddr = row.get(16)?;
+        )
+        .err_kind(ErrorKind::Internal)?;
+    let mut rows = stmt
+        .query(params![&hash.0[..]])
+        .err_kind(ErrorKind::Internal)?;
+    let row = rows
+        .next()
+        .err_kind(ErrorKind::Internal)?
+        .ok_or_else(|| format_err_t!(NotFound, "no such session"))?;
+    let creation_addr: FromSqlIpAddr = row.get(8).err_kind(ErrorKind::Internal)?;
+    let revocation_addr: FromSqlIpAddr = row.get(11).err_kind(ErrorKind::Internal)?;
+    let last_use_addr: FromSqlIpAddr = row.get(16).err_kind(ErrorKind::Internal)?;
     let mut permissions = Permissions::new();
-    permissions.merge_from_bytes(row.get_raw_checked(18)?.as_blob()?)?;
+    permissions
+        .merge_from_bytes(
+            row.get_raw_checked(18)
+                .err_kind(ErrorKind::Internal)?
+                .as_blob()
+                .err_kind(ErrorKind::Internal)?,
+        )
+        .err_kind(ErrorKind::Internal)?;
     Ok(Session {
-        user_id: row.get(0)?,
-        seed: row.get(1)?,
-        flags: row.get(2)?,
-        domain: row.get(3)?,
-        description: row.get(4)?,
-        creation_password_id: row.get(5)?,
+        user_id: row.get(0).err_kind(ErrorKind::Internal)?,
+        seed: row.get(1).err_kind(ErrorKind::Internal)?,
+        flags: row.get(2).err_kind(ErrorKind::Internal)?,
+        domain: row.get(3).err_kind(ErrorKind::Internal)?,
+        description: row.get(4).err_kind(ErrorKind::Internal)?,
+        creation_password_id: row.get(5).err_kind(ErrorKind::Internal)?,
         creation: Request {
-            when_sec: row.get(6)?,
-            user_agent: row.get(7)?,
+            when_sec: row.get(6).err_kind(ErrorKind::Internal)?,
+            user_agent: row.get(7).err_kind(ErrorKind::Internal)?,
             addr: creation_addr.0,
         },
         revocation: Request {
-            when_sec: row.get(9)?,
-            user_agent: row.get(10)?,
+            when_sec: row.get(9).err_kind(ErrorKind::Internal)?,
+            user_agent: row.get(10).err_kind(ErrorKind::Internal)?,
             addr: revocation_addr.0,
         },
-        revocation_reason: row.get(12)?,
-        revocation_reason_detail: row.get(13)?,
+        revocation_reason: row.get(12).err_kind(ErrorKind::Internal)?,
+        revocation_reason_detail: row.get(13).err_kind(ErrorKind::Internal)?,
         last_use: Request {
-            when_sec: row.get(14)?,
-            user_agent: row.get(15)?,
+            when_sec: row.get(14).err_kind(ErrorKind::Internal)?,
+            user_agent: row.get(15).err_kind(ErrorKind::Internal)?,
             addr: last_use_addr.0,
         },
-        use_count: row.get(17)?,
+        use_count: row.get(17).err_kind(ErrorKind::Internal)?,
         dirty: false,
         permissions,
     })
@@ -969,7 +991,10 @@ mod tests {
         let e = state
             .authenticate_session(&conn, req.clone(), &sid.hash())
             .unwrap_err();
-        assert_eq!(format!("{}", e), "session is no longer valid (reason=1)");
+        assert_eq!(
+            format!("{}", e),
+            "Unauthenticated: session is no longer valid (reason=1)"
+        );
 
         // Everything should persist across reload.
         drop(state);
@@ -977,7 +1002,10 @@ mod tests {
         let e = state
             .authenticate_session(&conn, req, &sid.hash())
             .unwrap_err();
-        assert_eq!(format!("{}", e), "session is no longer valid (reason=1)");
+        assert_eq!(
+            format!("{}", e),
+            "Unauthenticated: session is no longer valid (reason=1)"
+        );
     }
 
     #[test]
@@ -1028,7 +1056,10 @@ mod tests {
         let e = state
             .authenticate_session(&conn, req, &sid.hash())
             .unwrap_err();
-        assert_eq!(format!("{}", e), "session is no longer valid (reason=1)");
+        assert_eq!(
+            format!("{}", e),
+            "Unauthenticated: session is no longer valid (reason=1)"
+        );
     }
 
     #[test]
@@ -1159,7 +1190,10 @@ mod tests {
         let e = state
             .authenticate_session(&conn, req.clone(), &sid.hash())
             .unwrap_err();
-        assert_eq!(format!("{}", e), "user \"slamb\" is disabled");
+        assert_eq!(
+            format!("{}", e),
+            "Unauthenticated: user \"slamb\" is disabled"
+        );
 
         // The user should still be disabled after reload.
         drop(state);
@@ -1167,7 +1201,10 @@ mod tests {
         let e = state
             .authenticate_session(&conn, req, &sid.hash())
             .unwrap_err();
-        assert_eq!(format!("{}", e), "user \"slamb\" is disabled");
+        assert_eq!(
+            format!("{}", e),
+            "Unauthenticated: user \"slamb\" is disabled"
+        );
     }
 
     #[test]
@@ -1206,7 +1243,7 @@ mod tests {
         let e = state
             .authenticate_session(&conn, req.clone(), &sid.hash())
             .unwrap_err();
-        assert_eq!(format!("{}", e), "no such session");
+        assert_eq!(format!("{}", e), "Unauthenticated: no such session");
 
         // The user should still be deleted after reload.
         drop(state);
@@ -1215,7 +1252,7 @@ mod tests {
         let e = state
             .authenticate_session(&conn, req.clone(), &sid.hash())
             .unwrap_err();
-        assert_eq!(format!("{}", e), "no such session");
+        assert_eq!(format!("{}", e), "Unauthenticated: no such session");
     }
 
     #[test]
diff --git a/server/db/db.rs b/server/db/db.rs
index bd63164..271f203 100644
--- a/server/db/db.rs
+++ b/server/db/db.rs
@@ -2173,7 +2173,7 @@ impl LockedDatabase {
         &mut self,
         req: auth::Request,
         sid: &auth::SessionHash,
-    ) -> Result<(&auth::Session, &User), Error> {
+    ) -> Result<(&auth::Session, &User), base::Error> {
         self.auth.authenticate_session(&self.conn, req, sid)
     }
 
diff --git a/server/src/web.rs b/server/src/web.rs
index 673748d..7f4c373 100644
--- a/server/src/web.rs
+++ b/server/src/web.rs
@@ -1180,6 +1180,18 @@ impl Service {
         serve_json(req, &signals)
     }
 
+    /// Authenticates the session (if any) and returns a Caller.
+    ///
+    /// If there's no session,
+    /// 1.  if `allow_unauthenticated_permissions` is configured, returns okay
+    ///     with those permissions.
+    /// 2.  if the caller specifies `unauth_path`, returns okay with no
+    ///     permissions.
+    /// 3.  returns `Unauthenticated` error otherwise.
+    ///
+    /// Does no authorization. That is, this doesn't check that the returned
+    /// permissions are sufficient for whatever operation the caller is
+    /// performing.
     fn authenticate(
         &self,
         req: &Request<hyper::Body>,
@@ -1188,22 +1200,28 @@ impl Service {
         if let Some(sid) = extract_sid(req) {
             let authreq = self.authreq(req);
 
-            // TODO: real error handling! this assumes all errors are due to lack of
-            // authentication, when they could be logic errors in SQL or such.
-            if let Ok((s, u)) = self
+            match self
                 .db
                 .lock()
                 .authenticate_session(authreq.clone(), &sid.hash())
             {
-                return Ok(Caller {
-                    permissions: s.permissions.clone(),
-                    session: Some(json::Session {
-                        username: u.username.clone(),
-                        csrf: s.csrf(),
-                    }),
-                });
-            }
-            info!("authenticate_session failed");
+                Ok((s, u)) => {
+                    return Ok(Caller {
+                        permissions: s.permissions.clone(),
+                        session: Some(json::Session {
+                            username: u.username.clone(),
+                            csrf: s.csrf(),
+                        }),
+                    })
+                }
+                Err(e) if e.kind() == base::ErrorKind::Unauthenticated => {
+                    // Log the specific reason this session is unauthenticated.
+                    // Don't let the API client see it, as it may have a
+                    // revocation reason that isn't for their eyes.
+                    warn!("Session authentication failed: {:?}", &e);
+                }
+                Err(e) => return Err(e),
+            };
         }
 
         if let Some(s) = self.allow_unauthenticated_permissions.as_ref() {