aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMartin Fischer <martin@push-f.com>2021-06-24 17:45:54 +0200
committerMartin Fischer <martin@push-f.com>2021-06-24 18:02:30 +0200
commitd49d835e63ec654e3a5bf75b3b365354460382e8 (patch)
tree4fc823707248f770b8595b5537bef7a75e503335
parent0bb0af08c3fb7e100e5451e636719ce367fe1138 (diff)
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.
-rw-r--r--src/controller.rs135
-rw-r--r--src/error.rs4
-rw-r--r--src/get_routes.rs3
-rw-r--r--src/main.rs16
-rw-r--r--src/post_routes.rs5
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,
));
}