From 4d39197df2622440db30070e5135d53db61b33fa Mon Sep 17 00:00:00 2001 From: Stuart Heap Date: Thu, 16 Sep 2021 15:58:11 +0200 Subject: [PATCH] use migrations properly, avoid panics --- .../up.sql | 13 +- .../mysql/2021-09-16-133000_add-sso/up.sql | 7 ++ .../2019-09-12-100000_create_tables/up.sql | 13 +- .../2021-09-16-133000_add_sso/up.sql | 7 ++ .../up.sql | 13 +- .../sqlite/2021-09-16-133000_add_sso/up.sql | 7 ++ src/api/identity.rs | 119 +++++++++--------- 7 files changed, 88 insertions(+), 91 deletions(-) create mode 100644 migrations/mysql/2021-09-16-133000_add-sso/up.sql create mode 100644 migrations/postgresql/2021-09-16-133000_add_sso/up.sql create mode 100644 migrations/sqlite/2021-09-16-133000_add_sso/up.sql diff --git a/migrations/mysql/2018-02-17-205753_create_collections_and_orgs/up.sql b/migrations/mysql/2018-02-17-205753_create_collections_and_orgs/up.sql index e1c20b31..dd90f9dc 100644 --- a/migrations/mysql/2018-02-17-205753_create_collections_and_orgs/up.sql +++ b/migrations/mysql/2018-02-17-205753_create_collections_and_orgs/up.sql @@ -5,16 +5,9 @@ CREATE TABLE collections ( ); CREATE TABLE organizations ( - uuid VARCHAR(40) NOT NULL PRIMARY KEY, - name TEXT NOT NULL, - billing_email TEXT NOT NULL, - identifier TEXT NOT NULL, - use_sso BOOLEAN NOT NULL, - callback_path TEXT NOT NULL, - signed_out_callback_path TEXT NOT NULL, - authority TEXT NOT NULL, - client_id TEXT NOT NULL, - client_secret TEXT NOT NULL + uuid VARCHAR(40) NOT NULL PRIMARY KEY, + name TEXT NOT NULL, + billing_email TEXT NOT NULL ); CREATE TABLE users_collections ( diff --git a/migrations/mysql/2021-09-16-133000_add-sso/up.sql b/migrations/mysql/2021-09-16-133000_add-sso/up.sql new file mode 100644 index 00000000..04bc34e0 --- /dev/null +++ b/migrations/mysql/2021-09-16-133000_add-sso/up.sql @@ -0,0 +1,7 @@ +ALTER TABLE organizations ADD COLUMN identifier TEXT NOT NULL; +ALTER TABLE organizations ADD COLUMN use_sso BOOLEAN NOT NULL; +ALTER TABLE organizations ADD COLUMN callback_path TEXT NOT NULL; +ALTER TABLE organizations ADD COLUMN signed_out_callback_path TEXT NOT NULL; +ALTER TABLE organizations ADD COLUMN authority TEXT NOT NULL; +ALTER TABLE organizations ADD COLUMN client_id TEXT NOT NULL; +ALTER TABLE organizations ADD COLUMN client_secret TEXT NOT NULL; diff --git a/migrations/postgresql/2019-09-12-100000_create_tables/up.sql b/migrations/postgresql/2019-09-12-100000_create_tables/up.sql index 74f492f1..d66435b2 100644 --- a/migrations/postgresql/2019-09-12-100000_create_tables/up.sql +++ b/migrations/postgresql/2019-09-12-100000_create_tables/up.sql @@ -33,16 +33,9 @@ CREATE TABLE devices ( ); CREATE TABLE organizations ( - uuid VARCHAR(40) NOT NULL PRIMARY KEY, - name TEXT NOT NULL, - billing_email TEXT NOT NULL, - identifier TEXT NOT NULL, - use_sso BOOLEAN NOT NULL, - callback_path TEXT NOT NULL, - signed_out_callback_path TEXT NOT NULL, - authority TEXT NOT NULL, - client_id TEXT NOT NULL, - client_secret TEXT NOT NULL + uuid VARCHAR(40) NOT NULL PRIMARY KEY, + name TEXT NOT NULL, + billing_email TEXT NOT NULL ); CREATE TABLE ciphers ( diff --git a/migrations/postgresql/2021-09-16-133000_add_sso/up.sql b/migrations/postgresql/2021-09-16-133000_add_sso/up.sql new file mode 100644 index 00000000..04bc34e0 --- /dev/null +++ b/migrations/postgresql/2021-09-16-133000_add_sso/up.sql @@ -0,0 +1,7 @@ +ALTER TABLE organizations ADD COLUMN identifier TEXT NOT NULL; +ALTER TABLE organizations ADD COLUMN use_sso BOOLEAN NOT NULL; +ALTER TABLE organizations ADD COLUMN callback_path TEXT NOT NULL; +ALTER TABLE organizations ADD COLUMN signed_out_callback_path TEXT NOT NULL; +ALTER TABLE organizations ADD COLUMN authority TEXT NOT NULL; +ALTER TABLE organizations ADD COLUMN client_id TEXT NOT NULL; +ALTER TABLE organizations ADD COLUMN client_secret TEXT NOT NULL; diff --git a/migrations/sqlite/2018-02-17-205753_create_collections_and_orgs/up.sql b/migrations/sqlite/2018-02-17-205753_create_collections_and_orgs/up.sql index 7a8c835f..29601a4a 100644 --- a/migrations/sqlite/2018-02-17-205753_create_collections_and_orgs/up.sql +++ b/migrations/sqlite/2018-02-17-205753_create_collections_and_orgs/up.sql @@ -5,16 +5,9 @@ CREATE TABLE collections ( ); CREATE TABLE organizations ( - uuid TEXT NOT NULL PRIMARY KEY, - name TEXT NOT NULL, - billing_email TEXT NOT NULL, - identifier TEXT NOT NULL, - use_sso BOOLEAN NOT NULL, - callback_path TEXT NOT NULL, - signed_out_callback_path TEXT NOT NULL, - authority TEXT NOT NULL, - client_id TEXT NOT NULL, - client_secret TEXT NOT NULL + uuid TEXT NOT NULL PRIMARY KEY, + name TEXT NOT NULL, + billing_email TEXT NOT NULL ); diff --git a/migrations/sqlite/2021-09-16-133000_add_sso/up.sql b/migrations/sqlite/2021-09-16-133000_add_sso/up.sql new file mode 100644 index 00000000..04bc34e0 --- /dev/null +++ b/migrations/sqlite/2021-09-16-133000_add_sso/up.sql @@ -0,0 +1,7 @@ +ALTER TABLE organizations ADD COLUMN identifier TEXT NOT NULL; +ALTER TABLE organizations ADD COLUMN use_sso BOOLEAN NOT NULL; +ALTER TABLE organizations ADD COLUMN callback_path TEXT NOT NULL; +ALTER TABLE organizations ADD COLUMN signed_out_callback_path TEXT NOT NULL; +ALTER TABLE organizations ADD COLUMN authority TEXT NOT NULL; +ALTER TABLE organizations ADD COLUMN client_id TEXT NOT NULL; +ALTER TABLE organizations ADD COLUMN client_secret TEXT NOT NULL; diff --git a/src/api/identity.rs b/src/api/identity.rs index b07f0a8d..8c22c5ae 100644 --- a/src/api/identity.rs +++ b/src/api/identity.rs @@ -96,7 +96,10 @@ struct TokenPayload { fn _authorization_login(data: ConnectData, conn: DbConn, ip: &ClientIp) -> JsonResult { let org_identifier = data.org_identifier.as_ref().unwrap(); let code = data.code.as_ref().unwrap(); - let (access_token, refresh_token) = get_auth_code_access_token(&code, &org_identifier, &conn); + let (access_token, refresh_token) = match get_auth_code_access_token(&code, &org_identifier, &conn) { + Ok((access_token, refresh_token)) => (access_token, refresh_token), + Err(err) => err!(err), + }; let token = jsonwebtoken::dangerous_insecure_decode::(access_token.as_str()).unwrap().claims; let expiry = token.exp; let user_email = token.email; @@ -562,17 +565,7 @@ use openidconnect::{ Scope, OAuth2TokenResponse, }; -fn handle_error(fail: &T, msg: &'static str) { - let mut err_msg = format!("ERROR: {}", msg); - let mut cur_fail: Option<&dyn std::error::Error> = Some(fail); - while let Some(cause) = cur_fail { - err_msg += &format!("\n caused by: {}", cause); - cur_fail = cause.source(); - } - panic!("{}", err_msg); -} - -fn get_client_from_identifier (identifier: &str, conn: &DbConn) -> CoreClient { +fn get_client_from_identifier (identifier: &str, conn: &DbConn) -> Result { let organization = Organization::find_by_identifier(identifier, conn); match organization { @@ -581,21 +574,22 @@ fn get_client_from_identifier (identifier: &str, conn: &DbConn) -> CoreClient { let client_id = ClientId::new(organization.client_id); let client_secret = ClientSecret::new(organization.client_secret); let issuer_url = IssuerUrl::new(organization.authority).expect("invalid issuer URL"); - let provider_metadata = CoreProviderMetadata::discover(&issuer_url, http_client) - .unwrap_or_else(|err| { - handle_error(&err, "Failed to discover OpenID Provider"); - unreachable!(); - }); + let provider_metadata = match CoreProviderMetadata::discover(&issuer_url, http_client) { + Ok(metadata) => metadata, + Err(_err) => { + return Err("Failed to discover OpenID provider"); + }, + }; let client = CoreClient::from_provider_metadata( provider_metadata, client_id, Some(client_secret), ) .set_redirect_uri(RedirectUrl::new(redirect).expect("Invalid redirect URL")); - return client; + return Ok(client); }, None => { - panic!("unable to find org"); + Err("unable to find org") }, } } @@ -605,59 +599,62 @@ fn authorize( domain_hint: &RawStr, state: &RawStr, conn: DbConn, -) -> Redirect { +) -> ApiResult { let domain_hint_decoded = &domain_hint.percent_decode().expect("Invalid domain_hint").into_owned(); let state_decoded = &state.percent_decode().expect("Invalid state").into_owned(); - let client = get_client_from_identifier(domain_hint_decoded, &conn); - - // TODO store the nonce for validation on authorization token exchange - unclear where to store - // this - let (mut authorize_url, _csrf_state, _nonce) = client - .authorize_url( - AuthenticationFlow::::AuthorizationCode, - CsrfToken::new_random, - Nonce::new_random, - ) - .add_scope(Scope::new("email".to_string())) - .add_scope(Scope::new("profile".to_string())) - .url(); - - // it seems impossible to set the state going in dynamically (requires static lifetime string) - // so I change it after the fact - let old_pairs = authorize_url.query_pairs().clone(); - let new_pairs = old_pairs.map(|pair| { - let (key, value) = pair; - if key == "state" { - return format!("{}={}", key, state_decoded); - } - return format!("{}={}", key, value); - }); - let full_query = Vec::from_iter(new_pairs).join("&"); - authorize_url.set_query(Some(full_query.as_str())); + match get_client_from_identifier(domain_hint_decoded, &conn) { + Ok(client) => { + // TODO store the nonce for validation on authorization token exchange - unclear where to store + // this + let (mut authorize_url, _csrf_state, _nonce) = client + .authorize_url( + AuthenticationFlow::::AuthorizationCode, + CsrfToken::new_random, + Nonce::new_random, + ) + .add_scope(Scope::new("email".to_string())) + .add_scope(Scope::new("profile".to_string())) + .url(); + + // it seems impossible to set the state going in dynamically (requires static lifetime string) + // so I change it after the fact + let old_pairs = authorize_url.query_pairs().clone(); + let new_pairs = old_pairs.map(|pair| { + let (key, value) = pair; + if key == "state" { + return format!("{}={}", key, state_decoded); + } + return format!("{}={}", key, value); + }); + let full_query = Vec::from_iter(new_pairs).join("&"); + authorize_url.set_query(Some(full_query.as_str())); - return Redirect::to(authorize_url.to_string()); + return Ok(Redirect::to(authorize_url.to_string())); + }, + Err(_err) => err!("Unable to find client from identifier"), + } } fn get_auth_code_access_token ( code: &str, org_identifier: &str, conn: &DbConn, -) -> (String, String) { +) -> Result<(String, String), &'static str> { let oidc_code = AuthorizationCode::new(String::from(code)); - let client = get_client_from_identifier(org_identifier, conn); - - let token_response = client - .exchange_code(oidc_code) - .request(http_client) - .unwrap_or_else(|err| { - handle_error(&err, "Failed to contact token endpoint"); - unreachable!(); - }); - + match get_client_from_identifier(org_identifier, conn) { + Ok(client) => { + match client.exchange_code(oidc_code).request(http_client) { + Ok(token_response) => { + let access_token = token_response.access_token().secret().to_string(); + let refresh_token = token_response.refresh_token().unwrap().secret().to_string(); - let access_token = token_response.access_token().secret().to_string(); - let refresh_token = token_response.refresh_token().unwrap().secret().to_string(); + Ok((access_token, refresh_token)) + }, + Err(_err) => Err("Failed to contact token endpoint"), + } - (access_token, refresh_token) + }, + Err(_err) => Err("unable to find client"), + } }