Skip to content

Commit f9ac0af

Browse files
qutaxqutax
committed
Hash password in service layer instead of data access layer
Signed-off-by: Knut Borchers <[email protected]>
1 parent 7d237d7 commit f9ac0af

File tree

7 files changed

+187
-50
lines changed

7 files changed

+187
-50
lines changed

‎backend/src/core/repository/user.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -5,5 +5,5 @@ use mockall::automock;
55
#[cfg_attr(test, automock)]
66
pub trait UserRepository {
77
fn find_user_by_username(&self, username: String) -> Option<User>;
8-
fn set_password(&self, username: String, new_password: String) -> anyhow::Result<()>; // talisman-ignore-line
8+
fn set_password_hash(&self, username: String, new_password_hash: String) -> anyhow::Result<()>; // talisman-ignore-line
99
}

‎backend/src/core/service/user.rs

+108-11
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,38 @@ use crate::models::users::User;
33

44
pub trait UserService {
55
fn find_user_by_username(&self, username: String) -> Option<User>;
6-
fn set_password(&self, username: String, new_password: String) -> anyhow::Result<()>; // talisman-ignore-line
6+
fn change_password(
7+
&self,
8+
username: String,
9+
current_password: String,
10+
new_password: String,
11+
) -> anyhow::Result<()>;
12+
}
13+
14+
fn hash_password(password: &str, // talisman-ignore-line
15+
) -> String {
16+
use rand::Rng;
17+
18+
let config = argon2::Config::default();
19+
let salt: String = rand::thread_rng()
20+
.sample_iter(&rand::distributions::Alphanumeric)
21+
.take(16)
22+
.map(char::from)
23+
.collect();
24+
25+
argon2::hash_encoded(
26+
password.as_bytes(), // talisman-ignore-line
27+
salt.as_bytes(),
28+
&config,
29+
)
30+
.unwrap()
31+
}
32+
33+
fn password_matches_hash(
34+
hash: &str,
35+
password: &str, // talisman-ignore-line
36+
) -> bool {
37+
argon2::verify_encoded(hash, password.as_bytes()).is_ok_and(|is_verified| is_verified)
738
}
839

940
pub struct DefaultUserService<UR: UserRepository> {
@@ -21,13 +52,25 @@ impl<UR: UserRepository> UserService for DefaultUserService<UR> {
2152
self.user_repository.find_user_by_username(username)
2253
}
2354

24-
fn set_password(
55+
fn change_password(
2556
// talisman-ignore-line
2657
&self,
2758
username: String,
59+
current_password: String,
2860
new_password: String,
2961
) -> anyhow::Result<()> {
30-
self.user_repository.set_password(username, new_password) // talisman-ignore-line
62+
let current_password_is_valid = self
63+
.find_user_by_username(username.clone())
64+
.ok_or(anyhow::Error::msg("User not found."))
65+
.map(|user| password_matches_hash(&user.password_hash, &current_password))?; // talisman-ignore-line
66+
67+
if current_password_is_valid {
68+
let new_password_hash = hash_password(&new_password); // talisman-ignore-line
69+
self.user_repository
70+
.set_password_hash(username, new_password_hash) // talisman-ignore-line
71+
} else {
72+
Err(anyhow::Error::msg("Password invalid."))
73+
}
3174
}
3275
}
3376

@@ -37,6 +80,12 @@ mod tests {
3780
use crate::core::repository::MockUserRepository;
3881
use mockall::predicate::*;
3982

83+
const EXAMPLE_PASSWORD: &'static str = "xoh7Ongui4oo"; // talisman-ignore-line
84+
85+
/// Example hash for `xoh7Ongui4oo`.
86+
const EXAMPLE_PASSWORD_HASH: &'static str = // talisman-ignore-line
87+
"$argon2i$v=19$m=4096,t=3,p=1$cmFuZG9tc2FsdA$OOA07UjKrh3ijWboNB5/Ur274nxXirUuifmSuGCXwY0"; // talisman-ignore-line
88+
4089
#[test]
4190
fn find_user_by_username_must_call_repository() {
4291
let mut user_repository = MockUserRepository::new();
@@ -51,23 +100,71 @@ mod tests {
51100

52101
let result = user_service.find_user_by_username(User::default().username);
53102

54-
assert_eq!(result, Some(User::default()))
103+
assert_eq!(result, Some(User::default()));
55104
}
56105

57106
#[test]
58-
fn set_password_must_call_repository() {
107+
fn change_password_must_fail_if_current_password_is_wrong() {
108+
let example_user: User = User {
109+
id: 73,
110+
username: "example_user".to_string(),
111+
password_hash: EXAMPLE_PASSWORD_HASH.to_string(), // talisman-ignore-line
112+
role: "admin".to_string(),
113+
};
114+
59115
let mut user_repository = MockUserRepository::new();
116+
117+
user_repository
118+
.expect_find_user_by_username()
119+
.with(eq("example_user".to_string()))
120+
.times(1)
121+
.returning(move |_r| Some(example_user.clone()));
122+
123+
user_repository.expect_set_password_hash().never();
124+
125+
let user_service = DefaultUserService::new(user_repository);
126+
let result = user_service.change_password(
127+
"example_user".to_string(),
128+
"invalid_value".to_string(),
129+
"new_password".to_string(),
130+
);
131+
132+
assert!(result.is_err());
133+
}
134+
135+
#[test]
136+
fn change_password_must_not_fail_if_current_password_is_correct() {
137+
let example_user: User = User {
138+
id: 73,
139+
username: "example_user".to_string(),
140+
password_hash: EXAMPLE_PASSWORD_HASH.to_string(), // talisman-ignore-line
141+
role: "admin".to_string(),
142+
};
143+
144+
let mut user_repository = MockUserRepository::new();
145+
146+
user_repository
147+
.expect_find_user_by_username()
148+
.with(eq("example_user".to_string()))
149+
.times(1)
150+
.returning(move |_r| Some(example_user.clone()));
151+
60152
user_repository
61-
.expect_set_password()
62-
.with(eq(User::default().username), eq("new_password".to_string())) // talisman-ignore-line
153+
.expect_set_password_hash()
154+
.with(
155+
eq("example_user".to_string()),
156+
function(|new_hash: &String| password_matches_hash(&new_hash, "new_password")), // talisman-ignore-line
157+
)
63158
.times(1)
64159
.returning(|_r, _s| Ok(()));
65160

66161
let user_service = DefaultUserService::new(user_repository);
67-
let result = user_service
68-
.set_password(User::default().username, "new_password".to_string()) // talisman-ignore-line
69-
.unwrap();
162+
let result = user_service.change_password(
163+
"example_user".to_string(),
164+
EXAMPLE_PASSWORD.to_string(), // talisman-ignore-line
165+
"new_password".to_string(),
166+
);
70167

71-
assert_eq!(result, ())
168+
assert!(result.is_ok());
72169
}
73170
}

‎backend/src/handlers/admin.rs

+5-1
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,11 @@ pub async fn change_password(
141141
};
142142
if user.eq(&login_data) {
143143
user_service
144-
.set_password(username, change_password_data.new_password.to_string()) // talisman-ignore-line
144+
.change_password(
145+
username,
146+
change_password_data.old_password.to_string(),
147+
change_password_data.new_password.to_string(),
148+
)
145149
.map_err(handlers::error::InternalError::from)?;
146150
let response = LoginResponse::from(&user);
147151
let json = serde_json::to_string(&response)?;

‎backend/src/lib.rs

-13
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,6 @@ use diesel::result::Error;
66
use dotenvy::dotenv;
77
use models::rejected_transaction::{NewRejectedTransaction, RejectedTransaction};
88
use r2d2::PooledConnection;
9-
use rand::distributions::Alphanumeric;
10-
use rand::{thread_rng, Rng};
119
use repository::PostgresThemeRepository;
1210

1311
use self::models::runner::Runner;
@@ -153,17 +151,6 @@ pub fn is_eu_country(country: &str) -> bool {
153151
EU_COUNTRIES.contains(&country)
154152
}
155153

156-
pub fn hash_password(password: String) -> String {
157-
let config = argon2::Config::default();
158-
let salt: String = thread_rng()
159-
.sample_iter(&Alphanumeric)
160-
.take(16)
161-
.map(char::from)
162-
.collect();
163-
164-
argon2::hash_encoded(password.as_bytes(), salt.as_bytes(), &config).unwrap()
165-
}
166-
167154
lazy_static! {
168155
static ref TEMPLATES: tera::Tera = {
169156
match tera::Tera::new("templates/**/*") {

‎backend/src/repository/user.rs

+3-7
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
use crate::core::repository::UserRepository;
2-
use crate::hash_password;
32
use crate::models::users::User;
43
use crate::schema::users::dsl::users;
54
use crate::schema::users::{password_hash as schema_password_hash, username as schema_username}; // talisman-ignore-line
@@ -31,21 +30,18 @@ impl UserRepository for PostgresUserRepository {
3130
.expect("Failed to find user")
3231
}
3332

34-
fn set_password(
35-
// talisman-ignore-line
33+
fn set_password_hash(
3634
&self,
3735
username: String,
38-
new_password: String,
36+
new_password_hash: String, // talisman-ignore-line
3937
) -> anyhow::Result<()> {
4038
let mut connection = self
4139
.connection_pool
4240
.get()
4341
.expect("Unable to get connection.");
4442

45-
let new_hash = hash_password(new_password);
46-
4743
let affected_rows = diesel::update(users)
48-
.set(schema_password_hash.eq(new_hash))
44+
.set(schema_password_hash.eq(new_password_hash))
4945
.filter(schema_username.eq(&username))
5046
.execute(&mut connection)
5147
.expect("Unable to update password");

‎backend/tests/admin.rs

+64-13
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ async fn change_password_should_fail_if_new_password_is_empty() {
9090
.body(
9191
serde_json::to_string(&LoginData {
9292
username: "admin".to_string(),
93-
password: "xoh7Ongui4oo".to_string(),
93+
password: "xoh7Ongui4oo".to_string(), // talisman-ignore-line
9494
})
9595
.unwrap(),
9696
)
@@ -104,7 +104,7 @@ async fn change_password_should_fail_if_new_password_is_empty() {
104104
.expect("Unable to get cookie");
105105

106106
let password_change_data = PasswordChangeData {
107-
old_password: "xoh7Ongui4oo".to_string(),
107+
old_password: "xoh7Ongui4oo".to_string(), // talisman-ignore-line
108108
new_password: "".to_string(),
109109
};
110110

@@ -136,7 +136,7 @@ async fn change_password_should_fail_if_old_password_is_empty() {
136136
.body(
137137
serde_json::to_string(&LoginData {
138138
username: "admin".to_string(),
139-
password: "xoh7Ongui4oo".to_string(),
139+
password: "xoh7Ongui4oo".to_string(), // talisman-ignore-line
140140
})
141141
.unwrap(),
142142
)
@@ -151,7 +151,7 @@ async fn change_password_should_fail_if_old_password_is_empty() {
151151

152152
let password_change_data = PasswordChangeData {
153153
old_password: "".to_string(),
154-
new_password: "new_password".to_string(),
154+
new_password: "new_password".to_string(), // talisman-ignore-line
155155
};
156156

157157
let actual_response = client
@@ -182,7 +182,7 @@ async fn change_password_should_fail_if_old_password_is_invalid() {
182182
.body(
183183
serde_json::to_string(&LoginData {
184184
username: "admin".to_string(),
185-
password: "xoh7Ongui4oo".to_string(),
185+
password: "xoh7Ongui4oo".to_string(), // talisman-ignore-line
186186
})
187187
.unwrap(),
188188
)
@@ -197,7 +197,7 @@ async fn change_password_should_fail_if_old_password_is_invalid() {
197197

198198
let password_change_data = PasswordChangeData {
199199
old_password: "not the correct password!".to_string(),
200-
new_password: "new_password".to_string(),
200+
new_password: "new_password".to_string(), // talisman-ignore-line
201201
};
202202

203203
let actual_response = client
@@ -228,34 +228,85 @@ async fn change_password_should_be_successful_if_new_and_old_passwords_are_valid
228228
.body(
229229
serde_json::to_string(&LoginData {
230230
username: "admin".to_string(),
231-
password: "xoh7Ongui4oo".to_string(),
231+
password: "xoh7Ongui4oo".to_string(), // talisman-ignore-line
232232
})
233233
.unwrap(),
234234
)
235235
.send()
236236
.await
237-
.expect("Unable to send request.");
237+
.expect("Unable to send request");
238238

239239
let cookie = login_response
240240
.cookies()
241241
.next()
242242
.expect("Unable to get cookie");
243243

244244
let password_change_data = PasswordChangeData {
245-
old_password: "xoh7Ongui4oo".to_string(),
246-
new_password: "new_password".to_string(),
245+
old_password: "xoh7Ongui4oo".to_string(), // talisman-ignore-line
246+
new_password: "new_password".to_string(), // talisman-ignore-line
247247
};
248248

249-
let actual_response = client
249+
let change_password_response = client
250250
.put(format!("{address}/api/admin/change_password"))
251251
.header("Content-Type", "application/json")
252252
.header("Cookie", format!("{}={}", cookie.name(), cookie.value()))
253253
.body(serde_json::to_string(&password_change_data).unwrap())
254254
.send()
255255
.await
256-
.expect("Unable to send request.");
256+
.expect("Unable to send request");
257+
258+
assert_eq!(
259+
change_password_response.status(),
260+
actix_web::http::StatusCode::OK
261+
);
262+
263+
let logout_response = client
264+
.post(format!("{address}/api/admin/logout"))
265+
.header("Content-Type", "application/json")
266+
.header("Cookie", format!("{}={}", cookie.name(), cookie.value()))
267+
.send()
268+
.await
269+
.expect("Unable to send request");
270+
271+
assert_eq!(
272+
logout_response.status(),
273+
actix_web::http::StatusCode::NO_CONTENT
274+
);
275+
276+
let failed_login_response = client
277+
.post(format!("{address}/api/admin/login"))
278+
.header("Content-Type", "application/json")
279+
.body(
280+
serde_json::to_string(&LoginData {
281+
username: "admin".to_string(),
282+
password: "xoh7Ongui4oo".to_string(), // talisman-ignore-line
283+
})
284+
.unwrap(),
285+
)
286+
.send()
287+
.await
288+
.expect("Unable to send request");
289+
290+
assert_eq!(
291+
failed_login_response.status(),
292+
actix_web::http::StatusCode::FORBIDDEN
293+
);
294+
295+
let new_login_response = client
296+
.post(format!("{address}/api/admin/login"))
297+
.header("Content-Type", "application/json")
298+
.body(
299+
serde_json::to_string(&LoginData {
300+
username: "admin".to_string(),
301+
password: "new_password".to_string(), // talisman-ignore-line
302+
})
303+
.unwrap(),
304+
)
305+
.send()
306+
.await
307+
.expect("Unable to send request");
257308

258-
assert_eq!(actual_response.status(), actix_web::http::StatusCode::OK);
309+
assert_eq!(new_login_response.status(), actix_web::http::StatusCode::OK);
259310
}
260311

261312
#[actix_web::test]

0 commit comments

Comments
 (0)