From a836cc864a3171d01dfb33b9a3cf856175f76517 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Thu, 8 Dec 2022 12:19:28 +0100 Subject: [PATCH] storage: unify most of the remaining errors --- .../src/oauth2/authorization/complete.rs | 13 +- crates/handlers/src/oauth2/consent.rs | 74 ++++---- crates/handlers/src/oauth2/token.rs | 9 +- crates/handlers/src/views/shared.rs | 4 +- crates/storage/src/compat.rs | 4 +- crates/storage/src/lib.rs | 15 +- crates/storage/src/oauth2/access_token.rs | 6 +- .../storage/src/oauth2/authorization_grant.rs | 161 ++++++++++++------ crates/storage/src/oauth2/client.rs | 10 +- crates/storage/src/oauth2/consent.rs | 18 +- crates/storage/src/oauth2/mod.rs | 43 +++-- crates/storage/src/oauth2/refresh_token.rs | 6 +- .../storage/src/upstream_oauth2/provider.rs | 6 +- crates/storage/src/user.rs | 2 +- 14 files changed, 238 insertions(+), 133 deletions(-) diff --git a/crates/handlers/src/oauth2/authorization/complete.rs b/crates/handlers/src/oauth2/authorization/complete.rs index 82a6c0d3..1f312379 100644 --- a/crates/handlers/src/oauth2/authorization/complete.rs +++ b/crates/handlers/src/oauth2/authorization/complete.rs @@ -47,6 +47,9 @@ pub enum RouteError { #[error(transparent)] Anyhow(#[from] anyhow::Error), + #[error("authorization grant was not found")] + NotFound, + #[error("authorization grant is not in a pending state")] NotPending, } @@ -55,6 +58,9 @@ impl IntoResponse for RouteError { fn into_response(self) -> axum::response::Response { // TODO: better error pages match self { + RouteError::NotFound => { + (StatusCode::NOT_FOUND, "authorization grant was not found").into_response() + } RouteError::NotPending => ( StatusCode::BAD_REQUEST, "authorization grant not in a pending state", @@ -88,10 +94,12 @@ pub(crate) async fn get( let maybe_session = session_info.load_session(&mut txn).await?; - let grant = get_grant_by_id(&mut txn, grant_id).await?; + let grant = get_grant_by_id(&mut txn, grant_id) + .await? + .ok_or(RouteError::NotFound)?; let callback_destination = CallbackDestination::try_from(&grant)?; - let continue_grant = PostAuthAction::continue_grant(grant_id); + let continue_grant = PostAuthAction::continue_grant(grant.id); let session = if let Some(session) = maybe_session { session @@ -143,6 +151,7 @@ pub enum GrantCompletionError { } impl_from_error_for_route!(GrantCompletionError: sqlx::Error); +impl_from_error_for_route!(GrantCompletionError: mas_storage::DatabaseError); impl_from_error_for_route!(GrantCompletionError: super::callback::IntoCallbackDestinationError); pub(crate) async fn complete( diff --git a/crates/handlers/src/oauth2/consent.rs b/crates/handlers/src/oauth2/consent.rs index a963c74e..9ab2f063 100644 --- a/crates/handlers/src/oauth2/consent.rs +++ b/crates/handlers/src/oauth2/consent.rs @@ -14,7 +14,6 @@ use std::sync::Arc; -use anyhow::Context; use axum::{ extract::{Form, Path, State}, response::{Html, IntoResponse, Response}, @@ -38,12 +37,33 @@ use sqlx::PgPool; use thiserror::Error; use ulid::Ulid; +use crate::impl_from_error_for_route; + #[derive(Debug, Error)] pub enum RouteError { + #[error(transparent)] + Internal(Box), + #[error(transparent)] Anyhow(#[from] anyhow::Error), + + #[error(transparent)] + Csrf(#[from] mas_axum_utils::csrf::CsrfError), + + #[error("Authorization grant not found")] + GrantNotFound, + + #[error("Authorization grant already used")] + GrantNotPending, + + #[error("Policy violation")] + PolicyViolation, } +impl_from_error_for_route!(sqlx::Error); +impl_from_error_for_route!(mas_templates::TemplateError); +impl_from_error_for_route!(mas_storage::DatabaseError); + impl IntoResponse for RouteError { fn into_response(self) -> axum::response::Response { StatusCode::INTERNAL_SERVER_ERROR.into_response() @@ -58,22 +78,18 @@ pub(crate) async fn get( Path(grant_id): Path, ) -> Result { let (clock, mut rng) = crate::rng_and_clock()?; - let mut conn = pool - .acquire() - .await - .context("failed to acquire db connection")?; + let mut conn = pool.acquire().await?; let (session_info, cookie_jar) = cookie_jar.session_info(); - let maybe_session = session_info - .load_session(&mut conn) - .await - .context("could not load session")?; + let maybe_session = session_info.load_session(&mut conn).await?; - let grant = get_grant_by_id(&mut conn, grant_id).await?; + let grant = get_grant_by_id(&mut conn, grant_id) + .await? + .ok_or(RouteError::GrantNotFound)?; if !matches!(grant.stage, AuthorizationGrantStage::Pending) { - return Err(anyhow::anyhow!("authorization grant not pending").into()); + return Err(RouteError::GrantNotPending); } if let Some(session) = maybe_session { @@ -89,10 +105,7 @@ pub(crate) async fn get( .with_session(session) .with_csrf(csrf_token.form_value()); - let content = templates - .render_consent(&ctx) - .await - .context("failed to render template")?; + let content = templates.render_consent(&ctx).await?; Ok((cookie_jar, Html(content)).into_response()) } else { @@ -100,10 +113,7 @@ pub(crate) async fn get( .with_session(session) .with_csrf(csrf_token.form_value()); - let content = templates - .render_policy_violation(&ctx) - .await - .context("failed to render template")?; + let content = templates.render_policy_violation(&ctx).await?; Ok((cookie_jar, Html(content)).into_response()) } @@ -121,23 +131,17 @@ pub(crate) async fn post( Form(form): Form>, ) -> Result { let (clock, mut rng) = crate::rng_and_clock()?; - let mut txn = pool - .begin() - .await - .context("failed to begin db transaction")?; + let mut txn = pool.begin().await?; - cookie_jar - .verify_form(clock.now(), form) - .context("csrf verification failed")?; + cookie_jar.verify_form(clock.now(), form)?; let (session_info, cookie_jar) = cookie_jar.session_info(); - let maybe_session = session_info - .load_session(&mut txn) - .await - .context("could not load session")?; + let maybe_session = session_info.load_session(&mut txn).await?; - let grant = get_grant_by_id(&mut txn, grant_id).await?; + let grant = get_grant_by_id(&mut txn, grant_id) + .await? + .ok_or(RouteError::GrantNotFound)?; let next = PostAuthAction::continue_grant(grant_id); let session = if let Some(session) = maybe_session { @@ -153,7 +157,7 @@ pub(crate) async fn post( .await?; if !res.valid() { - return Err(anyhow::anyhow!("policy violation").into()); + return Err(RouteError::PolicyViolation); } // Do not consent for the "urn:matrix:org.matrix.msc2967.client:device:*" scope @@ -173,11 +177,9 @@ pub(crate) async fn post( ) .await?; - let _grant = give_consent_to_grant(&mut txn, grant) - .await - .context("failed to give consent to grant")?; + let _grant = give_consent_to_grant(&mut txn, grant).await?; - txn.commit().await.context("could not commit txn")?; + txn.commit().await?; Ok((cookie_jar, next.go_next()).into_response()) } diff --git a/crates/handlers/src/oauth2/token.rs b/crates/handlers/src/oauth2/token.rs index b9f4c2c9..56560c83 100644 --- a/crates/handlers/src/oauth2/token.rs +++ b/crates/handlers/src/oauth2/token.rs @@ -98,6 +98,9 @@ pub(crate) enum RouteError { #[error("could not verify client credentials")] ClientCredentialsVerification(#[from] CredentialsVerificationError), + #[error("grant not found")] + GrantNotFound, + #[error("invalid grant")] InvalidGrant, @@ -131,7 +134,7 @@ impl IntoResponse for RouteError { StatusCode::UNAUTHORIZED, Json(ClientError::from(ClientErrorCode::UnauthorizedClient)), ), - Self::InvalidGrant => ( + Self::InvalidGrant | Self::GrantNotFound => ( StatusCode::BAD_REQUEST, Json(ClientError::from(ClientErrorCode::InvalidGrant)), ), @@ -207,7 +210,9 @@ async fn authorization_code_grant( // TODO: there is a bunch of unnecessary cloning here // TODO: handle "not found" cases - let authz_grant = lookup_grant_by_code(&mut txn, &grant.code).await?; + let authz_grant = lookup_grant_by_code(&mut txn, &grant.code) + .await? + .ok_or(RouteError::GrantNotFound)?; let now = clock.now(); diff --git a/crates/handlers/src/views/shared.rs b/crates/handlers/src/views/shared.rs index 693fb4b4..941040ce 100644 --- a/crates/handlers/src/views/shared.rs +++ b/crates/handlers/src/views/shared.rs @@ -45,7 +45,9 @@ impl OptionalPostAuthAction { let Some(action) = self.post_auth_action.clone() else { return Ok(None) }; let ctx = match action { PostAuthAction::ContinueAuthorizationGrant { data } => { - let grant = get_grant_by_id(conn, data).await?; + let grant = get_grant_by_id(conn, data) + .await? + .context("Failed to load authorization grant")?; let grant = Box::new(grant); PostAuthContextInner::ContinueAuthorizationGrant { grant } } diff --git a/crates/storage/src/compat.rs b/crates/storage/src/compat.rs index 50be3d81..9ae43c5b 100644 --- a/crates/storage/src/compat.rs +++ b/crates/storage/src/compat.rs @@ -911,7 +911,7 @@ pub async fn fullfill_compat_sso_login( device: Device, ) -> Result { if !matches!(compat_sso_login.state, CompatSsoLoginState::Pending) { - return Err(DatabaseError::InvalidOperation); + return Err(DatabaseError::invalid_operation()); }; let mut txn = conn.begin().await?; @@ -986,7 +986,7 @@ pub async fn mark_compat_sso_login_as_exchanged( mut compat_sso_login: CompatSsoLogin, ) -> Result { let CompatSsoLoginState::Fulfilled { fulfilled_at, session } = compat_sso_login.state else { - return Err(DatabaseError::InvalidOperation); + return Err(DatabaseError::invalid_operation()); }; let exchanged_at = clock.now(); diff --git a/crates/storage/src/lib.rs b/crates/storage/src/lib.rs index 22fe1cb9..a03c9edf 100644 --- a/crates/storage/src/lib.rs +++ b/crates/storage/src/lib.rs @@ -104,7 +104,10 @@ pub enum DatabaseError { /// An error which happened because the requested database operation is /// invalid #[error("Invalid database operation")] - InvalidOperation, + InvalidOperation { + #[source] + source: Option>, + }, /// An error which happens when an operation affects not enough or too many /// rows @@ -124,6 +127,16 @@ impl DatabaseError { Err(DatabaseError::RowsAffected { expected, actual }) } } + + pub(crate) fn to_invalid_operation(e: E) -> Self { + Self::InvalidOperation { + source: Some(Box::new(e)), + } + } + + pub(crate) const fn invalid_operation() -> Self { + Self::InvalidOperation { source: None } + } } #[derive(Debug, Error)] diff --git a/crates/storage/src/oauth2/access_token.rs b/crates/storage/src/oauth2/access_token.rs index dccc7635..1bda49c0 100644 --- a/crates/storage/src/oauth2/access_token.rs +++ b/crates/storage/src/oauth2/access_token.rs @@ -12,7 +12,6 @@ // See the License for the specific language governing permissions and // limitations under the License. -use anyhow::Context; use chrono::{DateTime, Duration, Utc}; use mas_data_model::{AccessToken, Authentication, BrowserSession, Session, User, UserEmail}; use rand::Rng; @@ -40,7 +39,7 @@ pub async fn add_access_token( session: &Session, access_token: String, expires_after: Duration, -) -> Result { +) -> Result { let created_at = clock.now(); let expires_at = created_at + expires_after; let id = Ulid::from_datetime_with_source(created_at.into(), &mut rng); @@ -61,8 +60,7 @@ pub async fn add_access_token( expires_at, ) .execute(executor) - .await - .context("could not insert oauth2 access token")?; + .await?; Ok(AccessToken { id, diff --git a/crates/storage/src/oauth2/authorization_grant.rs b/crates/storage/src/oauth2/authorization_grant.rs index 5917f0d8..bb73cc0f 100644 --- a/crates/storage/src/oauth2/authorization_grant.rs +++ b/crates/storage/src/oauth2/authorization_grant.rs @@ -12,11 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. -#![allow(clippy::unused_async)] - use std::num::NonZeroU32; -use anyhow::Context; use chrono::{DateTime, Utc}; use mas_data_model::{ Authentication, AuthorizationCode, AuthorizationGrant, AuthorizationGrantStage, BrowserSession, @@ -31,7 +28,7 @@ use url::Url; use uuid::Uuid; use super::client::lookup_client; -use crate::{Clock, DatabaseInconsistencyError}; +use crate::{Clock, DatabaseError, DatabaseInconsistencyError2, LookupResultExt}; #[tracing::instrument( skip_all, @@ -39,7 +36,7 @@ use crate::{Clock, DatabaseInconsistencyError}; %client.id, grant.id, ), - err(Debug), + err, )] #[allow(clippy::too_many_arguments)] pub async fn new_authorization_grant( @@ -57,7 +54,7 @@ pub async fn new_authorization_grant( response_mode: ResponseMode, response_type_id_token: bool, requires_consent: bool, -) -> Result { +) -> Result { let code_challenge = code .as_ref() .and_then(|c| c.pkce.as_ref()) @@ -113,8 +110,7 @@ pub async fn new_authorization_grant( created_at, ) .execute(executor) - .await - .context("could not insert oauth2 authorization grant")?; + .await?; Ok(AuthorizationGrant { id, @@ -171,17 +167,23 @@ impl GrantLookup { async fn into_authorization_grant( self, executor: impl PgExecutor<'_>, - ) -> Result { - let scope: Scope = self - .oauth2_authorization_grant_scope - .parse() - .map_err(|_e| DatabaseInconsistencyError)?; + ) -> Result { + let id = self.oauth2_authorization_grant_id.into(); + let scope: Scope = self.oauth2_authorization_grant_scope.parse().map_err(|e| { + DatabaseInconsistencyError2::on("oauth2_authorization_grants") + .column("scope") + .row(id) + .source(e) + })?; // TODO: don't unwrap let client = lookup_client(executor, self.oauth2_client_id.into()) - .await - .unwrap() - .unwrap(); + .await? + .ok_or_else(|| { + DatabaseInconsistencyError2::on("oauth2_authorization_grants") + .column("client_id") + .row(id) + })?; let last_authentication = match ( self.user_session_last_authentication_id, @@ -192,7 +194,9 @@ impl GrantLookup { created_at, }), (None, None) => None, - _ => return Err(DatabaseInconsistencyError), + _ => { + return Err(DatabaseInconsistencyError2::on("user_session_authentications").into()) + } }; let primary_email = match ( @@ -208,7 +212,11 @@ impl GrantLookup { confirmed_at, }), (None, None, None, None) => None, - _ => return Err(DatabaseInconsistencyError), + _ => { + return Err(DatabaseInconsistencyError2::on("users") + .column("primary_user_email_id") + .into()) + } }; let session = match ( @@ -257,7 +265,14 @@ impl GrantLookup { Some(session) } (None, None, None, None, None, None, None) => None, - _ => return Err(DatabaseInconsistencyError), + _ => { + return Err( + DatabaseInconsistencyError2::on("oauth2_authorization_grants") + .column("oauth2_session_id") + .row(id) + .into(), + ) + } }; let stage = match ( @@ -282,7 +297,12 @@ impl GrantLookup { AuthorizationGrantStage::Cancelled { cancelled_at } } _ => { - return Err(DatabaseInconsistencyError); + return Err( + DatabaseInconsistencyError2::on("oauth2_authorization_grants") + .column("stage") + .row(id) + .into(), + ); } }; @@ -302,7 +322,12 @@ impl GrantLookup { }), (None, None) => None, _ => { - return Err(DatabaseInconsistencyError); + return Err( + DatabaseInconsistencyError2::on("oauth2_authorization_grants") + .column("code_challenge_method") + .row(id) + .into(), + ); } }; @@ -314,38 +339,63 @@ impl GrantLookup { (false, None, None) => None, (true, Some(code), pkce) => Some(AuthorizationCode { code, pkce }), _ => { - return Err(DatabaseInconsistencyError); + return Err( + DatabaseInconsistencyError2::on("oauth2_authorization_grants") + .column("authorization_code") + .row(id) + .into(), + ); } }; let redirect_uri = self .oauth2_authorization_grant_redirect_uri .parse() - .map_err(|_e| DatabaseInconsistencyError)?; + .map_err(|e| { + DatabaseInconsistencyError2::on("oauth2_authorization_grants") + .column("redirect_uri") + .row(id) + .source(e) + })?; let response_mode = self .oauth2_authorization_grant_response_mode .parse() - .map_err(|_e| DatabaseInconsistencyError)?; + .map_err(|e| { + DatabaseInconsistencyError2::on("oauth2_authorization_grants") + .column("response_mode") + .row(id) + .source(e) + })?; let max_age = self .oauth2_authorization_grant_max_age .map(u32::try_from) .transpose() - .map_err(|_e| DatabaseInconsistencyError)? + .map_err(|e| { + DatabaseInconsistencyError2::on("oauth2_authorization_grants") + .column("max_age") + .row(id) + .source(e) + })? .map(NonZeroU32::try_from) .transpose() - .map_err(|_e| DatabaseInconsistencyError)?; + .map_err(|e| { + DatabaseInconsistencyError2::on("oauth2_authorization_grants") + .column("max_age") + .row(id) + .source(e) + })?; Ok(AuthorizationGrant { - id: self.oauth2_authorization_grant_id.into(), + id, stage, client, code, scope, state: self.oauth2_authorization_grant_state, nonce: self.oauth2_authorization_grant_nonce, - max_age, // TODO + max_age, response_mode, redirect_uri, created_at: self.oauth2_authorization_grant_created_at, @@ -358,13 +408,12 @@ impl GrantLookup { #[tracing::instrument( skip_all, fields(grant.id = %id), - err(Debug), + err, )] pub async fn get_grant_by_id( conn: &mut PgConnection, id: Ulid, -) -> Result { - // TODO: handle "not found" cases +) -> Result, DatabaseError> { let res = sqlx::query_as!( GrantLookup, r#" @@ -420,19 +469,20 @@ pub async fn get_grant_by_id( ) .fetch_one(&mut *conn) .await - .context("failed to get grant by id")?; + .to_option()?; + + let Some(res) = res else { return Ok(None) }; let grant = res.into_authorization_grant(&mut *conn).await?; - Ok(grant) + Ok(Some(grant)) } -#[tracing::instrument(skip_all, err(Debug))] +#[tracing::instrument(skip_all, err)] pub async fn lookup_grant_by_code( conn: &mut PgConnection, code: &str, -) -> Result { - // TODO: handle "not found" cases +) -> Result, DatabaseError> { let res = sqlx::query_as!( GrantLookup, r#" @@ -488,11 +538,13 @@ pub async fn lookup_grant_by_code( ) .fetch_one(&mut *conn) .await - .context("failed to lookup grant by code")?; + .to_option()?; + + let Some(res) = res else { return Ok(None) }; let grant = res.into_authorization_grant(&mut *conn).await?; - Ok(grant) + Ok(Some(grant)) } #[tracing::instrument( @@ -504,7 +556,7 @@ pub async fn lookup_grant_by_code( user_session.id = %browser_session.id, user.id = %browser_session.user.id, ), - err(Debug), + err, )] pub async fn derive_session( executor: impl PgExecutor<'_>, @@ -512,7 +564,7 @@ pub async fn derive_session( clock: &Clock, grant: &AuthorizationGrant, browser_session: BrowserSession, -) -> Result { +) -> Result { let created_at = clock.now(); let id = Ulid::from_datetime_with_source(created_at.into(), &mut rng); tracing::Span::current().record("session.id", tracing::field::display(id)); @@ -538,8 +590,7 @@ pub async fn derive_session( Uuid::from(grant.id), ) .execute(executor) - .await - .context("could not insert oauth2 session")?; + .await?; Ok(Session { id, @@ -558,13 +609,13 @@ pub async fn derive_session( user_session.id = %session.browser_session.id, user.id = %session.browser_session.user.id, ), - err(Debug), + err, )] pub async fn fulfill_grant( executor: impl PgExecutor<'_>, mut grant: AuthorizationGrant, session: Session, -) -> Result { +) -> Result { let fulfilled_at = sqlx::query_scalar!( r#" UPDATE oauth2_authorization_grants AS og @@ -581,10 +632,12 @@ pub async fn fulfill_grant( Uuid::from(session.id), ) .fetch_one(executor) - .await - .context("could not mark grant as fulfilled")?; + .await?; - grant.stage = grant.stage.fulfill(fulfilled_at, session)?; + grant.stage = grant + .stage + .fulfill(fulfilled_at, session) + .map_err(DatabaseError::to_invalid_operation)?; Ok(grant) } @@ -595,7 +648,7 @@ pub async fn fulfill_grant( %grant.id, client.id = %grant.client.id, ), - err(Debug), + err, )] pub async fn give_consent_to_grant( executor: impl PgExecutor<'_>, @@ -625,13 +678,13 @@ pub async fn give_consent_to_grant( %grant.id, client.id = %grant.client.id, ), - err(Debug), + err, )] pub async fn exchange_grant( executor: impl PgExecutor<'_>, clock: &Clock, mut grant: AuthorizationGrant, -) -> Result { +) -> Result { let exchanged_at = clock.now(); sqlx::query!( r#" @@ -643,10 +696,12 @@ pub async fn exchange_grant( exchanged_at, ) .execute(executor) - .await - .context("could not mark grant as exchanged")?; + .await?; - grant.stage = grant.stage.exchange(exchanged_at)?; + grant.stage = grant + .stage + .exchange(exchanged_at) + .map_err(DatabaseError::to_invalid_operation)?; Ok(grant) } diff --git a/crates/storage/src/oauth2/client.rs b/crates/storage/src/oauth2/client.rs index a1619bf2..c2f4740f 100644 --- a/crates/storage/src/oauth2/client.rs +++ b/crates/storage/src/oauth2/client.rs @@ -468,8 +468,12 @@ pub async fn insert_client_from_config( jwks: Option<&PublicJsonWebKeySet>, jwks_uri: Option<&Url>, redirect_uris: &[Url], -) -> Result<(), anyhow::Error> { - let jwks = jwks.map(serde_json::to_value).transpose()?; +) -> Result<(), DatabaseError> { + let jwks = jwks + .map(serde_json::to_value) + .transpose() + .map_err(DatabaseError::to_invalid_operation)?; + let jwks_uri = jwks_uri.map(Url::as_str); let client_auth_method = client_auth_method.to_string(); @@ -526,7 +530,7 @@ pub async fn insert_client_from_config( Ok(()) } -pub async fn truncate_clients(executor: impl PgExecutor<'_>) -> Result<(), anyhow::Error> { +pub async fn truncate_clients(executor: impl PgExecutor<'_>) -> Result<(), sqlx::Error> { sqlx::query!("TRUNCATE oauth2_client_redirect_uris, oauth2_clients CASCADE") .execute(executor) .await?; diff --git a/crates/storage/src/oauth2/consent.rs b/crates/storage/src/oauth2/consent.rs index 95bd78cb..c1596584 100644 --- a/crates/storage/src/oauth2/consent.rs +++ b/crates/storage/src/oauth2/consent.rs @@ -21,7 +21,7 @@ use sqlx::PgExecutor; use ulid::Ulid; use uuid::Uuid; -use crate::Clock; +use crate::{Clock, DatabaseError, DatabaseInconsistencyError2}; #[tracing::instrument( skip_all, @@ -29,13 +29,13 @@ use crate::Clock; %user.id, %client.id, ), - err(Debug), + err, )] pub async fn fetch_client_consent( executor: impl PgExecutor<'_>, user: &User, client: &Client, -) -> Result { +) -> Result { let scope_tokens: Vec = sqlx::query_scalar!( r#" SELECT scope_token @@ -53,7 +53,13 @@ pub async fn fetch_client_consent( .map(|s| ScopeToken::from_str(&s)) .collect(); - Ok(scope?) + let scope = scope.map_err(|e| { + DatabaseInconsistencyError2::on("oauth2_consents") + .column("scope_token") + .source(e) + })?; + + Ok(scope) } #[tracing::instrument( @@ -63,7 +69,7 @@ pub async fn fetch_client_consent( %client.id, %scope, ), - err(Debug), + err, )] pub async fn insert_client_consent( executor: impl PgExecutor<'_>, @@ -72,7 +78,7 @@ pub async fn insert_client_consent( user: &User, client: &Client, scope: &Scope, -) -> Result<(), anyhow::Error> { +) -> Result<(), sqlx::Error> { let now = clock.now(); let (tokens, ids): (Vec, Vec) = scope .iter() diff --git a/crates/storage/src/oauth2/mod.rs b/crates/storage/src/oauth2/mod.rs index 20fd195e..6f1ba4a0 100644 --- a/crates/storage/src/oauth2/mod.rs +++ b/crates/storage/src/oauth2/mod.rs @@ -14,7 +14,6 @@ use std::collections::{BTreeSet, HashMap}; -use anyhow::Context; use mas_data_model::{BrowserSession, Session, User}; use sqlx::{PgConnection, PgExecutor, QueryBuilder}; use tracing::{info_span, Instrument}; @@ -25,7 +24,7 @@ use self::client::lookup_clients; use crate::{ pagination::{process_page, QueryBuilderExt}, user::lookup_active_session, - Clock, + Clock, DatabaseError, DatabaseInconsistencyError2, }; pub mod access_token; @@ -42,13 +41,13 @@ pub mod refresh_token; user_session.id = %session.browser_session.id, client.id = %session.client.id, ), - err(Debug), + err, )] pub async fn end_oauth_session( executor: impl PgExecutor<'_>, clock: &Clock, session: Session, -) -> Result<(), anyhow::Error> { +) -> Result<(), DatabaseError> { let finished_at = clock.now(); let res = sqlx::query!( r#" @@ -62,9 +61,7 @@ pub async fn end_oauth_session( .execute(executor) .await?; - anyhow::ensure!(res.rows_affected() == 1); - - Ok(()) + DatabaseError::ensure_affected_rows(&res, 1) } #[derive(sqlx::FromRow)] @@ -81,7 +78,7 @@ struct OAuthSessionLookup { %user.id, %user.username, ), - err(Display), + err, )] pub async fn get_paginated_user_oauth_sessions( conn: &mut PgConnection, @@ -90,7 +87,7 @@ pub async fn get_paginated_user_oauth_sessions( after: Option, first: Option, last: Option, -) -> Result<(bool, bool, Vec), anyhow::Error> { +) -> Result<(bool, bool, Vec), DatabaseError> { let mut query = QueryBuilder::new( r#" SELECT @@ -139,26 +136,42 @@ pub async fn get_paginated_user_oauth_sessions( for id in browser_session_ids { let v = lookup_active_session(&mut *conn, id) .await? - .context("Failed to load active session")?; + .ok_or_else(|| { + DatabaseInconsistencyError2::on("oauth2_sessions").column("user_session_id") + })?; browser_sessions.insert(id, v); } - let page: Result, _> = page + let page: Result, DatabaseInconsistencyError2> = page .into_iter() .map(|item| { + let id = Ulid::from(item.oauth2_session_id); let client = clients .get(&Ulid::from(item.oauth2_client_id)) - .context("client was not fetched")? + .ok_or_else(|| { + DatabaseInconsistencyError2::on("oauth2_sessions") + .column("oauth2_client_id") + .row(id) + })? .clone(); let browser_session = browser_sessions .get(&Ulid::from(item.user_session_id)) - .context("browser session was not fetched")? + .ok_or_else(|| { + DatabaseInconsistencyError2::on("oauth2_sessions") + .column("user_session_id") + .row(id) + })? .clone(); - let scope = item.scope.parse()?; + let scope = item.scope.parse().map_err(|e| { + DatabaseInconsistencyError2::on("oauth2_sessions") + .column("scope") + .row(id) + .source(e) + })?; - anyhow::Ok(Session { + Ok(Session { id: Ulid::from(item.oauth2_session_id), client, browser_session, diff --git a/crates/storage/src/oauth2/refresh_token.rs b/crates/storage/src/oauth2/refresh_token.rs index 6a97c2f6..afed1eb8 100644 --- a/crates/storage/src/oauth2/refresh_token.rs +++ b/crates/storage/src/oauth2/refresh_token.rs @@ -12,7 +12,6 @@ // See the License for the specific language governing permissions and // limitations under the License. -use anyhow::Context; use chrono::{DateTime, Utc}; use mas_data_model::{ AccessToken, Authentication, BrowserSession, RefreshToken, Session, User, UserEmail, @@ -43,7 +42,7 @@ pub async fn add_refresh_token( session: &Session, access_token: AccessToken, refresh_token: String, -) -> anyhow::Result { +) -> Result { let created_at = clock.now(); let id = Ulid::from_datetime_with_source(created_at.into(), &mut rng); tracing::Span::current().record("refresh_token.id", tracing::field::display(id)); @@ -63,8 +62,7 @@ pub async fn add_refresh_token( created_at, ) .execute(executor) - .await - .context("could not insert oauth2 refresh token")?; + .await?; Ok(RefreshToken { id, diff --git a/crates/storage/src/upstream_oauth2/provider.rs b/crates/storage/src/upstream_oauth2/provider.rs index 351fb3f2..4e9e6495 100644 --- a/crates/storage/src/upstream_oauth2/provider.rs +++ b/crates/storage/src/upstream_oauth2/provider.rs @@ -179,14 +179,14 @@ pub async fn add_provider( }) } -#[tracing::instrument(skip_all, err(Display))] +#[tracing::instrument(skip_all, err)] pub async fn get_paginated_providers( executor: impl PgExecutor<'_>, before: Option, after: Option, first: Option, last: Option, -) -> Result<(bool, bool, Vec), anyhow::Error> { +) -> Result<(bool, bool, Vec), DatabaseError> { let mut query = QueryBuilder::new( r#" SELECT @@ -224,7 +224,7 @@ pub async fn get_paginated_providers( #[tracing::instrument(skip_all, err)] pub async fn get_providers( executor: impl PgExecutor<'_>, -) -> Result, anyhow::Error> { +) -> Result, DatabaseError> { let res = sqlx::query_as!( ProviderLookup, r#" diff --git a/crates/storage/src/user.rs b/crates/storage/src/user.rs index a660ee96..95233fe4 100644 --- a/crates/storage/src/user.rs +++ b/crates/storage/src/user.rs @@ -1205,7 +1205,7 @@ pub async fn consume_email_verification( user_email_verification.state, UserEmailVerificationState::Valid ) { - return Err(DatabaseError::InvalidOperation); + return Err(DatabaseError::invalid_operation()); } let consumed_at = clock.now();