From 7c4f1dd06e6e80801a0605b6fc2b3d4326fd1762 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Tue, 16 Feb 2021 19:16:36 +0100 Subject: [PATCH 1/5] controllers::util: Simplify `ApiToken::find_by_api_token()` call --- src/controllers/util.rs | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/controllers/util.rs b/src/controllers/util.rs index 9f25147d6b..8ce6013026 100644 --- a/src/controllers/util.rs +++ b/src/controllers/util.rs @@ -73,17 +73,15 @@ fn authenticate_user(req: &dyn RequestExt) -> AppResult { .map(|h| h.to_string()) }; if let Some(header_value) = maybe_authorization { - let (user_id, token_id) = ApiToken::find_by_api_token(&conn, &header_value) - .map(|token| (token.user_id, Some(token.id))) - .map_err(|e| { - if e.is::() { - e - } else { - e.chain(internal("invalid token")).chain(forbidden()) - } - })?; - - (user_id, token_id) + let token = ApiToken::find_by_api_token(&conn, &header_value).map_err(|e| { + if e.is::() { + e + } else { + e.chain(internal("invalid token")).chain(forbidden()) + } + })?; + + (token.user_id, Some(token.id)) } else { // Unable to authenticate the user return Err(internal("no cookie session or auth header found")).chain_error(forbidden); From 1114534fc5c815becee5e5edfff1b021dcd4e073 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Mon, 15 Feb 2021 23:13:23 +0100 Subject: [PATCH 2/5] controllers::util: Simplify `maybe_authorization` variable assignment There is no need for us to convert the header value to a `String` here when `ApiToken::find_by_api_token()` only needs a `&str` --- src/controllers/util.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/controllers/util.rs b/src/controllers/util.rs index 8ce6013026..16d866df11 100644 --- a/src/controllers/util.rs +++ b/src/controllers/util.rs @@ -66,14 +66,13 @@ fn authenticate_user(req: &dyn RequestExt) -> AppResult { (id, None) } else { // Otherwise, look for an `Authorization` header on the request - let maybe_authorization: Option = { - req.headers() - .get(header::AUTHORIZATION) - .and_then(|h| h.to_str().ok()) - .map(|h| h.to_string()) - }; + let maybe_authorization = req + .headers() + .get(header::AUTHORIZATION) + .and_then(|h| h.to_str().ok()); + if let Some(header_value) = maybe_authorization { - let token = ApiToken::find_by_api_token(&conn, &header_value).map_err(|e| { + let token = ApiToken::find_by_api_token(&conn, header_value).map_err(|e| { if e.is::() { e } else { From b6864faadca15d229335a8c9ae7e71149f86ccda Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Tue, 16 Feb 2021 21:18:31 +0100 Subject: [PATCH 3/5] controllers::util: Move `User::find()` calls into the condition branches This allows us to give more precise error messages and enables further simplification --- src/controllers/util.rs | 15 +++++++++------ src/tests/authentication.rs | 2 +- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/controllers/util.rs b/src/controllers/util.rs index 16d866df11..60e98e1110 100644 --- a/src/controllers/util.rs +++ b/src/controllers/util.rs @@ -62,8 +62,11 @@ fn authenticate_user(req: &dyn RequestExt) -> AppResult { let session = req.session(); let user_id_from_session = session.get("user_id").and_then(|s| s.parse::().ok()); - let (user_id, token_id) = if let Some(id) = user_id_from_session { - (id, None) + let (user, token_id) = if let Some(id) = user_id_from_session { + let user = User::find(&conn, id) + .chain_error(|| internal("user_id from cookie not found in database"))?; + + (user, None) } else { // Otherwise, look for an `Authorization` header on the request let maybe_authorization = req @@ -80,16 +83,16 @@ fn authenticate_user(req: &dyn RequestExt) -> AppResult { } })?; - (token.user_id, Some(token.id)) + let user = User::find(&conn, token.user_id) + .chain_error(|| internal("user_id from token not found in database"))?; + + (user, Some(token.id)) } else { // Unable to authenticate the user return Err(internal("no cookie session or auth header found")).chain_error(forbidden); } }; - let user = User::find(&conn, user_id) - .chain_error(|| internal("user_id from cookie or token not found in database"))?; - Ok(AuthenticatedUser { user, token_id }) } diff --git a/src/tests/authentication.rs b/src/tests/authentication.rs index 23a0340bff..bc63ad8015 100644 --- a/src/tests/authentication.rs +++ b/src/tests/authentication.rs @@ -8,7 +8,7 @@ static URL: &str = "/api/v1/me/updates"; static MUST_LOGIN: &[u8] = b"{\"errors\":[{\"detail\":\"must be logged in to perform that action\"}]}"; static INTERNAL_ERROR_NO_USER: &str = - "user_id from cookie or token not found in database caused by NotFound"; + "user_id from cookie not found in database caused by NotFound"; fn call(app: &TestApp, mut request: MockRequest) -> HandlerResult { app.as_middleware().call(&mut request) From c7455571e65ae634eaf1c842b100c7e8469701f8 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Tue, 16 Feb 2021 21:22:11 +0100 Subject: [PATCH 4/5] controllers::util: Move `return` statements into the condition branches --- src/controllers/util.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/controllers/util.rs b/src/controllers/util.rs index 60e98e1110..2813029eac 100644 --- a/src/controllers/util.rs +++ b/src/controllers/util.rs @@ -62,11 +62,14 @@ fn authenticate_user(req: &dyn RequestExt) -> AppResult { let session = req.session(); let user_id_from_session = session.get("user_id").and_then(|s| s.parse::().ok()); - let (user, token_id) = if let Some(id) = user_id_from_session { + if let Some(id) = user_id_from_session { let user = User::find(&conn, id) .chain_error(|| internal("user_id from cookie not found in database"))?; - (user, None) + return Ok(AuthenticatedUser { + user, + token_id: None, + }); } else { // Otherwise, look for an `Authorization` header on the request let maybe_authorization = req @@ -86,14 +89,15 @@ fn authenticate_user(req: &dyn RequestExt) -> AppResult { let user = User::find(&conn, token.user_id) .chain_error(|| internal("user_id from token not found in database"))?; - (user, Some(token.id)) + return Ok(AuthenticatedUser { + user, + token_id: Some(token.id), + }); } else { // Unable to authenticate the user return Err(internal("no cookie session or auth header found")).chain_error(forbidden); } - }; - - Ok(AuthenticatedUser { user, token_id }) + } } impl<'a> UserAuthenticationExt for dyn RequestExt + 'a { From 9680a6268071c536d7d6438654653d34338a7ddb Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Tue, 16 Feb 2021 21:23:12 +0100 Subject: [PATCH 5/5] controllers::util: Reduce nesting --- src/controllers/util.rs | 54 ++++++++++++++++++++--------------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/src/controllers/util.rs b/src/controllers/util.rs index 2813029eac..d0cb1e32bc 100644 --- a/src/controllers/util.rs +++ b/src/controllers/util.rs @@ -70,34 +70,34 @@ fn authenticate_user(req: &dyn RequestExt) -> AppResult { user, token_id: None, }); - } else { - // Otherwise, look for an `Authorization` header on the request - let maybe_authorization = req - .headers() - .get(header::AUTHORIZATION) - .and_then(|h| h.to_str().ok()); - - if let Some(header_value) = maybe_authorization { - let token = ApiToken::find_by_api_token(&conn, header_value).map_err(|e| { - if e.is::() { - e - } else { - e.chain(internal("invalid token")).chain(forbidden()) - } - })?; - - let user = User::find(&conn, token.user_id) - .chain_error(|| internal("user_id from token not found in database"))?; - - return Ok(AuthenticatedUser { - user, - token_id: Some(token.id), - }); - } else { - // Unable to authenticate the user - return Err(internal("no cookie session or auth header found")).chain_error(forbidden); - } } + + // Otherwise, look for an `Authorization` header on the request + let maybe_authorization = req + .headers() + .get(header::AUTHORIZATION) + .and_then(|h| h.to_str().ok()); + + if let Some(header_value) = maybe_authorization { + let token = ApiToken::find_by_api_token(&conn, header_value).map_err(|e| { + if e.is::() { + e + } else { + e.chain(internal("invalid token")).chain(forbidden()) + } + })?; + + let user = User::find(&conn, token.user_id) + .chain_error(|| internal("user_id from token not found in database"))?; + + return Ok(AuthenticatedUser { + user, + token_id: Some(token.id), + }); + } + + // Unable to authenticate the user + return Err(internal("no cookie session or auth header found")).chain_error(forbidden); } impl<'a> UserAuthenticationExt for dyn RequestExt + 'a {