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.

1 files changed, 67 insertions, 68 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> {