refactor: simplify Controller request interception

In multi-user mode if Alice attempts to access /~bob/ she would get an
Unauthorized error since branches are private.  To improve the UX we
instead already showed Alice a list of which files Bob has shared with
her. Previously this was achieved with an before_return_error hook
in the Controller trait.

While this worked fine, it wasn't elegant, since it required passing
the Context struct in all Unauthorized errors, so that the
before_return_error hook could access the context.

This commit refactors the code to intercept requests to paths like
/~bob/ before the regular request handling instead of afterwards.

While this could have been implemented in the before_route hook, this
would have required either invoking parse_url_path a second time or
passing the Result of parse_url_path, both of which would be akward.
Therefore this commit also merges before_route into parse_url_path.

5 files changed, 74 insertions, 89 deletions

diff --git a/src/controller.rs b/src/controller.rs
index 5dc2a47..2d8b73f 100644
--- a/src/controller.rs
+++ b/src/controller.rs
@@ -19,14 +19,17 @@ use crate::Page;
 use crate::Response;
 
 pub trait Controller {
-    fn parse_url_path<'a>(&'a self, url_path: &'a str) -> Result<(Branch, &'a str), Error>;
+    /// Parse the URL into a Branch and file path or intercept the request handling.
+    fn parse_url_path<'a>(
+        &'a self,
+        path: &'a str,
+        parts: &Parts,
+        repo: &Repository,
+    ) -> Result<(Branch, &'a str), Result<Response, Error>>;
 
     /// Builds a URL path from a given Git branch and file path.
     fn build_url_path<'a>(&self, branch: &Branch, path: &'a str) -> String;
 
-    /// Allows the controller to intercept the request handling.
-    fn before_route(&self, parts: &Parts, repo: &Repository) -> Option<Result<Response, Error>>;
-
     /// Returns some HTML info to display for the current page.
     fn user_info_html(&self, parts: &Parts) -> Option<String> {
         None
@@ -62,11 +65,6 @@ pub trait Controller {
 
     /// Executed after successfully writing a file.
     fn after_write(&self, context: &mut Context) {}
-
-    /// Lets the controller optionally intercept error responses.
-    fn before_return_error(&self, error: &Error) -> Option<Response> {
-        None
-    }
 }
 
 pub struct SoloController(pub Branch);
@@ -74,24 +72,26 @@ pub struct SoloController(pub Branch);
 const USERNAME_HEADER: &str = "Username";
 
 impl Controller for SoloController {
-    fn parse_url_path<'a>(&'a self, url_path: &'a str) -> Result<(Branch, &'a str), Error> {
-        Ok((self.0.clone(), url_path))
-    }
-
-    fn build_url_path<'a>(&self, branch: &Branch, path: &'a str) -> String {
-        path.to_owned()
-    }
-
-    fn before_route(&self, parts: &Parts, repo: &Repository) -> Option<Result<Response, Error>> {
+    fn parse_url_path<'a>(
+        &'a self,
+        path: &'a str,
+        parts: &Parts,
+        repo: &Repository,
+    ) -> Result<(Branch, &'a str), Result<Response, Error>> {
         if parts.headers.contains_key(USERNAME_HEADER) {
-            return Some(Err(Error::BadRequest(format!(
+            return Err(Err(Error::BadRequest(format!(
                 "unexpected header {} (only \
             allowed in multi-user mode), aborting to prevent accidental \
             information leakage",
                 USERNAME_HEADER
             ))));
         }
-        None
+
+        Ok((self.0.clone(), path))
+    }
+
+    fn build_url_path<'a>(&self, branch: &Branch, path: &'a str) -> String {
+        path.to_owned()
     }
 
     fn signature(&self, repo: &Repository, parts: &Parts) -> Result<Signature, Error> {
@@ -187,8 +187,8 @@ impl MultiUserController {
         callback(entry);
     }
 
-    fn list_shares(&self, context: &Context, username: &str, out: &mut String) {
-        self.with_shares_cache(&context.repo, context.branch.clone(), |shares| {
+    fn list_shares(&self, repo: &Repository, branch: &Branch, username: &str, out: &mut String) {
+        self.with_shares_cache(&repo, branch.clone(), |shares| {
             out.push_str("<a href=..>../</a>");
             let exact_shares: Vec<_> = shares
                 .exact_rules
@@ -201,14 +201,11 @@ impl MultiUserController {
                 .filter_map(|(path, rules)| rules.0.get(username).map(|rule| (path, &rule.mode)))
                 .collect();
             if exact_shares.is_empty() && prefix_shares.is_empty() {
-                out.push_str(&format!(
-                    "{} hasn't shared any files with you.",
-                    context.branch.0
-                ));
+                out.push_str(&format!("{} hasn't shared any files with you.", branch.0));
             } else {
                 out.push_str(&format!(
                     "{} has shared the following files with you:",
-                    context.branch.0
+                    branch.0
                 ));
                 // TODO: display modes?
                 out.push_str("<ul>");
@@ -305,24 +302,60 @@ fn multi_user_startpage(
 }
 
 impl Controller for MultiUserController {
-    fn parse_url_path<'a>(&'a self, url_path: &'a str) -> Result<(Branch, &'a str), Error> {
-        let mut iter = url_path.splitn(3, '/');
+    fn parse_url_path<'a>(
+        &'a self,
+        path: &'a str,
+        parts: &Parts,
+        repo: &Repository,
+    ) -> Result<(Branch, &'a str), Result<Response, Error>> {
+        if !parts.headers.contains_key(USERNAME_HEADER) {
+            return Err(Err(Error::BadRequest(format!(
+                "expected header {} because of multi-user mode \
+            (this shouldn't be happening because a reverse-proxy \
+            should be used to set this header)",
+                USERNAME_HEADER
+            ))));
+        }
+        if path == "/" {
+            return Err(multi_user_startpage(&self, parts, repo));
+        }
+
+        let mut iter = path.splitn(3, '/');
         iter.next();
         let rev = iter.next().unwrap();
         if !rev.starts_with('~') {
-            return Err(Error::NotFound(
+            return Err(Err(Error::NotFound(
                 "branch name must be prefixed a tilde (~)".into(),
-            ));
+            )));
         }
         let rev = &rev[1..];
         if rev.trim().is_empty() {
-            return Err(Error::NotFound("invalid branch name".into()));
+            return Err(Err(Error::NotFound("invalid branch name".into())));
         }
         let rev = Branch(rev.to_owned());
         let unsanitized_path = match iter.next() {
             Some(value) => value,
-            None => return Err(Error::MissingTrailingSlash(url_path.to_string())),
+            None => {
+                return Err(Err(Error::MissingTrailingSlash(
+                    parts.uri.path().to_string(),
+                )))
+            }
         };
+        if unsanitized_path.is_empty() {
+            let username = username_from_parts(&parts).unwrap();
+            if username != rev.0 {
+                let mut page = Page {
+                    title: "".into(),
+                    header: None,
+                    body: String::new(),
+                    controller: self,
+                    parts,
+                };
+
+                self.list_shares(repo, &rev, username, &mut page.body);
+                return Err(Ok(page.into()));
+            }
+        }
         Ok((rev, unsanitized_path))
     }
 
@@ -330,21 +363,6 @@ impl Controller for MultiUserController {
         format!("/~{}/{}", branch.0, path)
     }
 
-    fn before_route(&self, parts: &Parts, repo: &Repository) -> Option<Result<Response, Error>> {
-        if !parts.headers.contains_key(USERNAME_HEADER) {
-            return Some(Err(Error::BadRequest(format!(
-                "expected header {} because of multi-user mode \
-            (this shouldn't be happening because a reverse-proxy \
-            should be used to set this header)",
-                USERNAME_HEADER
-            ))));
-        }
-        if parts.uri.path() == "/" {
-            return Some(multi_user_startpage(&self, parts, repo));
-        }
-        None
-    }
-
     fn user_info_html(&self, parts: &Parts) -> Option<String> {
         let username = username_from_parts(parts).unwrap();
         Some(format!(
@@ -368,28 +386,9 @@ impl Controller for MultiUserController {
                     }
                 }
             } else {
-                self.list_shares(context, username, &mut page.body);
-            }
-        }
-    }
-
-    fn before_return_error(&self, error: &Error) -> Option<Response> {
-        if let Error::Unauthorized(_, context) = error {
-            let username = username_from_parts(&context.parts).unwrap();
-            if context.path.components().count() == 0 {
-                let mut page = Page {
-                    title: "".into(),
-                    header: None,
-                    body: String::new(),
-                    controller: self,
-                    parts: &context.parts,
-                };
-
-                self.list_shares(context, username, &mut page.body);
-                return Some(page.into());
+                self.list_shares(&context.repo, &context.branch, username, &mut page.body);
             }
         }
-        None
     }
 
     fn signature(&self, _repo: &Repository, parts: &Parts) -> Result<Signature, Error> {
diff --git a/src/error.rs b/src/error.rs
index c006255..8460376 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -2,13 +2,11 @@ use sputnik::hyper_body::FormError;
 use sputnik::request::QueryError;
 use std::str::Utf8Error;
 
-use crate::Context;
-
 pub enum Error {
     /// A 400 bad request error.
     BadRequest(String),
     /// A 401 unauthorized error.
-    Unauthorized(String, Context),
+    Unauthorized(String),
     /// A 403 forbidden error.
     Forbidden(String),
     /// A 404 not found error.
diff --git a/src/get_routes.rs b/src/get_routes.rs
index d876b4f..ff86869 100644
--- a/src/get_routes.rs
+++ b/src/get_routes.rs
@@ -100,7 +100,6 @@ fn edit_blob<C: Controller>(
     if !controller.may_write_path(&ctx) {
         return Err(Error::Unauthorized(
             "you are not authorized to edit this file".into(),
-            ctx,
         ));
     }
     let blob = ctx.repo.find_blob(entr.id()).unwrap();
@@ -359,7 +358,6 @@ fn move_blob<C: Controller>(
     if !controller.may_move_path(&ctx) {
         return Err(Error::Unauthorized(
             "you are not authorized to move this file".into(),
-            ctx,
         ));
     }
 
@@ -386,7 +384,6 @@ fn remove_blob<C: Controller>(
     if !controller.may_move_path(&ctx) {
         return Err(Error::Unauthorized(
             "you are not authorized to remove this file".into(),
-            ctx,
         ));
     }
 
diff --git a/src/main.rs b/src/main.rs
index 99956c3..276a3e2 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -200,12 +200,9 @@ async fn service<C: Controller>(
     let mut resp = build_response(args, &*controller, parts, body)
         .await
         .unwrap_or_else(|err| {
-            if let Some(resp) = controller.before_return_error(&err) {
-                return resp;
-            }
             let (status, message) = match err {
                 Error::BadRequest(msg) => (400, msg),
-                Error::Unauthorized(msg, _ctx) => (401, msg),
+                Error::Unauthorized(msg) => (401, msg),
                 Error::Forbidden(msg) => (403, msg),
                 Error::NotFound(msg) => (404, msg),
                 Error::Internal(msg) => (500, msg),
@@ -322,11 +319,11 @@ async fn build_response<C: Controller>(
 
     let repo = Repository::open_bare(env::current_dir().unwrap()).unwrap();
 
-    if let Some(resp) = controller.before_route(&parts, &repo) {
-        return resp;
-    }
-
-    let (rev, unsanitized_path) = controller.parse_url_path(&unsanitized_path)?;
+    let (rev, unsanitized_path) = match controller.parse_url_path(&unsanitized_path, &parts, &repo)
+    {
+        Ok(parsed) => parsed,
+        Err(res) => return res,
+    };
 
     let mut comps = Vec::new();
 
@@ -355,7 +352,6 @@ async fn build_response<C: Controller>(
     if !controller.may_read_path(&ctx) {
         return Err(Error::Unauthorized(
             "you are not authorized to view this file".into(),
-            ctx,
         ));
     }
 
diff --git a/src/post_routes.rs b/src/post_routes.rs
index 311ee57..0374cae 100644
--- a/src/post_routes.rs
+++ b/src/post_routes.rs
@@ -111,7 +111,6 @@ async fn update_blob<C: Controller>(
     if !controller.may_write_path(&ctx) {
         return Err(Error::Unauthorized(
             "you are not authorized to edit this file".into(),
-            ctx,
         ));
     }
     let mut data: EditForm = body.into_form().await?;
@@ -162,7 +161,6 @@ async fn upload_blob<C: Controller>(
     if !controller.may_write_path(&ctx) {
         return Err(Error::Unauthorized(
             "you are not authorized to edit this file".into(),
-            ctx,
         ));
     }
     // Extract the `multipart/form-data` boundary from the headers.
@@ -206,7 +204,6 @@ async fn move_entry<C: Controller>(
     if !controller.may_move_path(&ctx) {
         return Err(Error::Unauthorized(
             "you are not authorized to move this file".into(),
-            ctx,
         ));
     }
     let mut data: MoveForm = body.into_form().await?;
@@ -288,7 +285,6 @@ async fn remove_entry<C: Controller>(
     if !controller.may_move_path(&ctx) {
         return Err(Error::Unauthorized(
             "you are not authorized to remove this file".into(),
-            ctx,
         ));
     }
     let data: RemoveForm = body.into_form().await?;
@@ -324,7 +320,6 @@ async fn diff_blob<C: Controller>(
     if !controller.may_write_path(&ctx) {
         return Err(Error::Unauthorized(
             "you are not authorized to edit this file".into(),
-            ctx,
         ));
     }