Skip to content

Commit

Permalink
Force password change upon password forgotten
Browse files Browse the repository at this point in the history
  • Loading branch information
TobiasDeBruijn committed Jan 1, 2025
1 parent ddfe7a2 commit 3c69297
Show file tree
Hide file tree
Showing 15 changed files with 191 additions and 40 deletions.
1 change: 1 addition & 0 deletions server/database/migrations/2_user_credentials.sql
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
CREATE TABLE user_credentials (
user_id VARCHAR(64) NOT NULL,
password_hash VARCHAR(72) NOT NULL,
change_required BOOLEAN DEFAULT FALSE,
PRIMARY KEY (user_id)
);
17 changes: 15 additions & 2 deletions server/database/src/user.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,17 +122,20 @@ impl User {
&self,
driver: &Database,
password: P,
require_change: bool,
) -> Result<()> {
if self.get_password_hash(&driver).await?.is_some() {
sqlx::query("UPDATE user_credentials SET password_hash = ? WHERE user_id = ?")
sqlx::query("UPDATE user_credentials SET password_hash = ?, change_required = ? WHERE user_id = ?")
.bind(password.as_ref())
.bind(require_change)
.bind(&self.user_id)
.execute(&**driver)
.await?;
} else {
sqlx::query("INSERT INTO user_credentials (user_id, password_hash) VALUES (?, ?)")
sqlx::query("INSERT INTO user_credentials (user_id, password_hash, change_required) VALUES (?, ?, ?)")
.bind(&self.user_id)
.bind(password.as_ref())
.bind(require_change)
.execute(&**driver)
.await?;
}
Expand All @@ -149,4 +152,14 @@ impl User {
.await?,
)
}

#[instrument]
pub async fn password_change_required(&self, driver: &Database) -> Result<Option<bool>> {
Ok(
sqlx::query_scalar("SELECT change_required FROM user_credentials WHERE user_id = ?")
.bind(&self.user_id)
.fetch_optional(&**driver)
.await?,
)
}
}
12 changes: 8 additions & 4 deletions server/wilford/src/authorization/combined.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ use crate::authorization::espo::{EspoAuthorizationProvider, EspoAuthorizationPro
use crate::authorization::local_provider::{
LocalAuthorizationProvider, LocalAuthorizationProviderError,
};
use crate::authorization::{AuthorizationError, AuthorizationProvider, UserInformation};
use crate::authorization::{
AuthorizationError, AuthorizationProvider, CredentialsValidationResult, UserInformation,
};
use crate::config::{AuthorizationProviderType, Config};
use database::driver::Database;
use espocrm_rs::EspoApiClient;
Expand Down Expand Up @@ -66,7 +68,8 @@ impl<'a> AuthorizationProvider for CombinedAuthorizationProvider<'a> {
username: &str,
password: &str,
totp_code: Option<&str>,
) -> Result<UserInformation, AuthorizationError<CombinedAuthorizationProviderError>> {
) -> Result<CredentialsValidationResult, AuthorizationError<CombinedAuthorizationProviderError>>
{
Ok(match self {
Self::Local(credentials_provider) => credentials_provider
.validate_credentials(username, password, totp_code)
Expand All @@ -91,14 +94,15 @@ impl<'a> AuthorizationProvider for CombinedAuthorizationProvider<'a> {
&self,
user_id: &str,
new_password: &str,
require_change: bool,
) -> Result<(), AuthorizationError<Self::Error>> {
Ok(match self {
Self::Local(local) => local
.set_password(user_id, new_password)
.set_password(user_id, new_password, require_change)
.await
.map_err(AuthorizationError::convert)?,
Self::EspoCrm(espocrm) => espocrm
.set_password(user_id, new_password)
.set_password(user_id, new_password, require_change)
.await
.map_err(AuthorizationError::convert)?,
})
Expand Down
26 changes: 18 additions & 8 deletions server/wilford/src/authorization/espo.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
use crate::authorization::{AuthorizationError, AuthorizationProvider, UserInformation};
use crate::authorization::{
AuthorizationError, AuthorizationProvider, CredentialsValidationResult, UserInformation,
};
use crate::espo::user::{EspoUser, LoginStatus};
use database::driver::Database;
use database::user::User;
Expand Down Expand Up @@ -45,7 +47,7 @@ impl<'a> AuthorizationProvider for EspoAuthorizationProvider<'a> {
username: &str,
password: &str,
totp_code: Option<&str>,
) -> Result<UserInformation, AuthorizationError<Self::Error>> {
) -> Result<CredentialsValidationResult, AuthorizationError<Self::Error>> {
// Check the credentials with the EspoCRM instance
// This will yield the user ID
let user_id = match EspoUser::try_login(&self.host, username, password, totp_code)
Expand Down Expand Up @@ -107,11 +109,14 @@ impl<'a> AuthorizationProvider for EspoAuthorizationProvider<'a> {
.map_err(|e| AuthorizationError::Other(e.into()))?;
}

Ok(UserInformation {
id: espo_user.id,
name: espo_user.name,
email: espo_user.email_address,
is_admin: espo_user.user_type.eq("admin"),
Ok(CredentialsValidationResult {
user_information: UserInformation {
id: espo_user.id,
name: espo_user.name,
email: espo_user.email_address,
is_admin: espo_user.user_type.eq("admin"),
},
require_password_change: false,
})
}

Expand All @@ -122,7 +127,12 @@ impl<'a> AuthorizationProvider for EspoAuthorizationProvider<'a> {
}

#[instrument(skip_all)]
async fn set_password(&self, _: &str, _: &str) -> Result<(), AuthorizationError<Self::Error>> {
async fn set_password(
&self,
_: &str,
_: &str,
_: bool,
) -> Result<(), AuthorizationError<Self::Error>> {
Err(AuthorizationError::UnsupportedOperation)
}

Expand Down
31 changes: 22 additions & 9 deletions server/wilford/src/authorization/local_provider.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
use crate::authorization::{AuthorizationError, AuthorizationProvider, UserInformation};
use crate::authorization::{
AuthorizationError, AuthorizationProvider, CredentialsValidationResult, UserInformation,
};
use bcrypt::{hash_with_result, verify, Version};
use database::driver::Database;
use database::user::User;
Expand Down Expand Up @@ -36,7 +38,7 @@ impl<'a> AuthorizationProvider for LocalAuthorizationProvider<'a> {
username: &str,
password: &str,
_: Option<&str>,
) -> Result<UserInformation, AuthorizationError<Self::Error>> {
) -> Result<CredentialsValidationResult, AuthorizationError<Self::Error>> {
// Fetch the user
let user = User::get_by_email(self.driver, username)
.await
Expand All @@ -56,12 +58,22 @@ impl<'a> AuthorizationProvider for LocalAuthorizationProvider<'a> {
// Use bcrypt to verify the hash is correct
let ok = verify(password, &stored_hash).map_err(Self::Error::from)?;

let require_password_change = user
.password_change_required(&self.driver)
.await
.map_err(Self::Error::from)?
// Unwrap is safe, only None if there is no password either.
.unwrap();

if ok {
Ok(UserInformation {
id: user.user_id,
email: user.email,
name: user.name,
is_admin: user.is_admin,
Ok(CredentialsValidationResult {
user_information: UserInformation {
id: user.user_id,
email: user.email,
name: user.name,
is_admin: user.is_admin,
},
require_password_change,
})
} else {
Err(AuthorizationError::InvalidCredentials)
Expand All @@ -77,6 +89,7 @@ impl<'a> AuthorizationProvider for LocalAuthorizationProvider<'a> {
&self,
user_id: &str,
new_password: &str,
require_change: bool,
) -> Result<(), AuthorizationError<Self::Error>> {
// Fetch the user
let user = User::get_by_id(self.driver, user_id)
Expand All @@ -88,7 +101,7 @@ impl<'a> AuthorizationProvider for LocalAuthorizationProvider<'a> {
let new_password = hash_password(&new_password)?;

// Finally, update the database
user.set_password_hash(self.driver, new_password)
user.set_password_hash(self.driver, new_password, require_change)
.await
.map_err(Self::Error::from)?;

Expand Down Expand Up @@ -131,7 +144,7 @@ impl<'a> AuthorizationProvider for LocalAuthorizationProvider<'a> {
let password = hash_password(password)?;

// Set password
user.set_password_hash(&self.driver, password)
user.set_password_hash(&self.driver, password, false)
.await
.map_err(Self::Error::from)?;

Expand Down
8 changes: 7 additions & 1 deletion server/wilford/src/authorization/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,11 @@ pub struct UserInformation {
pub email: String,
}

pub struct CredentialsValidationResult {
pub user_information: UserInformation,
pub require_password_change: bool,
}

pub trait AuthorizationProvider {
/// Error that can be returned by the authorization provider
type Error: std::error::Error;
Expand All @@ -70,7 +75,7 @@ pub trait AuthorizationProvider {
username: &str,
password: &str,
totp_code: Option<&str>,
) -> Result<UserInformation, AuthorizationError<Self::Error>>;
) -> Result<CredentialsValidationResult, AuthorizationError<Self::Error>>;

/// Whether the authorization provider supports changing the user's password.
fn supports_password_change(&self) -> bool;
Expand All @@ -86,6 +91,7 @@ pub trait AuthorizationProvider {
&self,
user_id: &str,
new_password: &str,
require_change: bool,
) -> Result<(), AuthorizationError<Self::Error>>;

/// Whether the authorization provider supports registering new users.
Expand Down
14 changes: 9 additions & 5 deletions server/wilford/src/routes/v1/auth/login.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ pub async fn login(
let auth_provider = CombinedAuthorizationProvider::new(&config, &database);

// Check the credentials and handle results
let user_information = match auth_provider
let validation_result = match auth_provider
.validate_credentials(
&payload.username,
&payload.password,
Expand Down Expand Up @@ -102,17 +102,21 @@ pub async fn login(
// short-circuiting behaviour, the scope check is only evaluated if the user is _not_ an admin.
// We use the lambda function to reduce the complecity of the if statement.
let scope_check = || {
are_scopes_allowed(&database, &authorization, &user_information)
.instrument(warn_span!("scope_check"))
are_scopes_allowed(
&database,
&authorization,
&validation_result.user_information,
)
.instrument(warn_span!("scope_check"))
};

if !user_information.is_admin && !scope_check().await? {
if !validation_result.user_information.is_admin && !scope_check().await? {
return Err(WebErrorKind::Forbidden.into());
}

// Mark the authorization as authorized.
authorization
.set_user_id(&database, &user_information.id)
.set_user_id(&database, &validation_result.user_information.id)
.instrument(warn_span!("authorization::set_user_id"))
.await
.tap_err(|e| warn!("{e}"))
Expand Down
2 changes: 1 addition & 1 deletion server/wilford/src/routes/v1/user/change_password.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ pub async fn change_password(
// Set new password
auth_error_to_web_error(
provider
.set_password(&auth.user.user_id, &payload.new_password)
.set_password(&auth.user.user_id, &payload.new_password, false)
.await,
)?;

Expand Down
14 changes: 11 additions & 3 deletions server/wilford/src/routes/v1/user/info.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
use crate::routes::auth::Auth;
use crate::routes::error::WebResult;
use crate::routes::WDatabase;
use actix_web::web;
use serde::Serialize;

Expand All @@ -7,13 +9,19 @@ pub struct Response {
name: String,
is_admin: bool,
espo_user_id: String,
require_password_change: bool,
}

/// Get information about the user
pub async fn info(auth: Auth) -> web::Json<Response> {
web::Json(Response {
pub async fn info(auth: Auth, database: WDatabase) -> WebResult<web::Json<Response>> {
Ok(web::Json(Response {
name: auth.name,
espo_user_id: auth.user_id,
is_admin: auth.is_admin,
})
require_password_change: auth
.user
.password_change_required(&database)
.await?
.unwrap_or(false),
}))
}
6 changes: 5 additions & 1 deletion server/wilford/src/routes/v1/user/password_forgotten.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,11 @@ pub async fn password_forgotten(

// Update the password in the database
let tmp_password = tmp_password();
auth_error_to_web_error(provider.set_password(&user.user_id, &tmp_password).await)?;
auth_error_to_web_error(
provider
.set_password(&user.user_id, &tmp_password, true)
.await,
)?;

// Email the user with their temporary password
if let Some(email_cfg) = &config.email {
Expand Down
2 changes: 2 additions & 0 deletions ui/src/components/user/security/ChangePassword.vue
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ export default defineComponent({
const user = await User.getCurrent();
await user.updatePassword(this.oldPassword!, this.newPassword!);
this.$emit('complete')
}
}
})
Expand Down
13 changes: 12 additions & 1 deletion ui/src/router/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,18 @@ const routes = [
{
path: '/me',
name: 'me',
component: () => import('@/views/me/Me.vue'),
children: [
{
path: '',
name: 'me',
component: () => import('@/views/me/Me.vue'),
},
{
path: 'require-password-change',
name: 'require-password-change',
component: () => import('@/views/me/RequirePasswordChange.vue')
}
]
}
]
},
Expand Down
21 changes: 17 additions & 4 deletions ui/src/scripts/user.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,20 @@ export class User {
this.isAdmin = isAdmin;
}

static async getCurrent(): Promise<User> {
static async getCurrent(): Promise<UserInfo> {
return (await (await fetch1(`${server}/api/v1/user/info`))
.map1(async (r) => {
if(r.status == 401) {
const client = await ClientInfo.getInternal();
window.location.href = client.getAuthorizationRedirect();
}

const j: _User = await r.json();
return new User(j.name, j.espo_user_id, j.is_admin);
interface _UserInfo extends _User {
require_password_change: boolean,
}

const j: _UserInfo = await r.json();
return new UserInfo(j.name, j.espo_user_id, j.is_admin, j.require_password_change);
})
).unwrap()
}
Expand Down Expand Up @@ -63,7 +67,7 @@ export class User {
}

async deletePermittedScope(scope: string) {
const r = await fetch(`${server}/api/v1/user/permitted-scopes/remove`, {
await fetch(`${server}/api/v1/user/permitted-scopes/remove`, {
method: 'DELETE',
body: JSON.stringify({
from: this.espoUserId,
Expand Down Expand Up @@ -153,4 +157,13 @@ export class User {
})
})).map(() => {})
}
}

export class UserInfo extends User {
requirePasswordChange: boolean;

constructor(name: string, espoUserId: string, isAdmin: boolean, requirePasswordChange: boolean) {
super(name, espoUserId, isAdmin);
this.requirePasswordChange = requirePasswordChange;
}
}
Loading

0 comments on commit 3c69297

Please sign in to comment.