Skip to content

Commit b0c39ae

Browse files
staduststadust
authored
Refactor authentication to more easily support oauth (stadust#171)
* refactor: split `password_hash` field from `AuthenticatedUser` Replace it with a `AuthenticationMethod` field. For now, this enum only contains the `Legacy` variant, indicating that the given user is using the current (but soon to be legacy) account system. Later, other authentication methods such as OAuth will add their own variants here. When matching in on `auth_method`, introduce unreachable `_ => ...` arms that will return sensible default-errors for functions that that only operate on legacy accounts (e.g. when doing oauth, you won't be able to change your password). Signed-off-by: stadust <[email protected]> * refactor: hide `register` endpoint behind `legacy_accounts` feature Once we support oauth accounts, we want to disable legacy accounts. However, for testing purposes, as well as for people wanting to run pointercrate clones, it is still desirable to use the legacy account system. Thus, hide it behind a cargo feature. Note that this cargo flag only disables the creation of new legacy accounts. Pre-existing legacy accounts will continue to be able to log in. Signed-off-by: stadust <[email protected]> * test: run with all features Feature flags are meant to be additive, so running tests only with all features enabled should not loose any coverage. Signed-off-by: stadust <[email protected]> * test: Add action for building all feature permutations While runnings tests with only with `--all-features` is fine, we need to make sure that all combinations of features actually build. Use `cargo-all-features` for that. I don't expect us to ever have sufficiently many feature for the combinatorial explosion of testing all permutations to get bad. Signed-off-by: stadust <[email protected]> --------- Signed-off-by: stadust <[email protected]>
1 parent 8bbf544 commit b0c39ae

File tree

16 files changed

+206
-68
lines changed

16 files changed

+206
-68
lines changed

‎.github/workflows/check_all_features.yml

+62
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
name: Check all feature permutations
2+
on: pull_request
3+
4+
jobs:
5+
# Label of the container job
6+
container-job:
7+
# Containers must run in Linux based operating systems
8+
runs-on: ubuntu-latest
9+
10+
# Service containers to run with `container-job`
11+
services:
12+
# Label used to access the service container
13+
postgres:
14+
# Docker Hub image
15+
image: postgres
16+
# Provide the password for postgres
17+
env:
18+
POSTGRES_USER: pointercrate
19+
POSTGRES_PASSWORD: postgres
20+
POSTGRES_DB: postgres
21+
ports:
22+
- 5432:5432
23+
# Set health checks to wait until postgres has started
24+
options: >-
25+
--health-cmd pg_isready
26+
--health-interval 10s
27+
--health-timeout 5s
28+
--health-retries 5
29+
30+
steps:
31+
# Downloads a copy of the code in your repository before running CI tests
32+
- name: Check out repository code
33+
uses: actions/checkout@v3
34+
35+
- name: Install rust toolchain
36+
uses: actions-rs/toolchain@v1
37+
with:
38+
profile: minimal
39+
toolchain: stable
40+
override: true
41+
components: llvm-tools-preview
42+
43+
- name: Install sqlx-cli and cargo-all-features
44+
uses: actions-rs/cargo@v1
45+
with:
46+
command: install
47+
args: sqlx-cli cargo-all-features
48+
49+
- name: Load pointercrate schema
50+
uses: actions-rs/cargo@v1
51+
with:
52+
command: sqlx
53+
args: migrate run
54+
env:
55+
DATABASE_URL: postgresql://pointercrate:postgres@localhost/postgres
56+
57+
- name: Check
58+
uses: actions-rs/cargo@v1
59+
with:
60+
command: check-all-features
61+
env:
62+
DATABASE_URL: postgresql://pointercrate:postgres@localhost/postgres

‎.github/workflows/test.yml

+1
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ jobs:
6262
uses: actions-rs/cargo@v1
6363
with:
6464
command: test
65+
args: --all-features
6566
env:
6667
DATABASE_URL: postgresql://pointercrate:postgres@localhost/postgres
6768
RUST_BACKTRACE: 1

‎pointercrate-example/Cargo.toml

+1-1
Original file line numberDiff line numberDiff line change
@@ -17,5 +17,5 @@ pointercrate-demonlist-api = { version = "0.1.0", path = "../pointercrate-demonl
1717
pointercrate-demonlist-pages = { version = "0.1.0", path = "../pointercrate-demonlist-pages" }
1818
pointercrate-user = { version = "0.1.0", path = "../pointercrate-user" }
1919
pointercrate-user-api = { version = "0.1.0", path = "../pointercrate-user-api" }
20-
pointercrate-user-pages = { version = "0.1.0", path = "../pointercrate-user-pages" }
20+
pointercrate-user-pages = { version = "0.1.0", path = "../pointercrate-user-pages", features = ["legacy_accounts"] }
2121
rocket = "0.5.1"

‎pointercrate-test/Cargo.toml

+1-1
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ pointercrate-core = {path = "../pointercrate-core"}
1313
pointercrate-core-api = {path = "../pointercrate-core-api"}
1414
pointercrate-user = {path = "../pointercrate-user"}
1515
pointercrate-user-api = {path = "../pointercrate-user-api"}
16-
pointercrate-user-pages = {path = "../pointercrate-user-pages"}
16+
pointercrate-user-pages = {path = "../pointercrate-user-pages", features = ["legacy_accounts"]}
1717
serde = "1.0.209"
1818
sqlx = { version = "0.8", default-features = false, features = [ "runtime-tokio-native-tls", "macros", "postgres", "chrono", "migrate" ] }
1919
rocket = "0.5.1"

‎pointercrate-user-api/Cargo.toml

+3
Original file line numberDiff line numberDiff line change
@@ -19,3 +19,6 @@ base64 = "0.22.1"
1919
nonzero_ext = "0.3.0"
2020
serde_urlencoded = "0.7.0"
2121
governor = "0.6.0"
22+
23+
[features]
24+
legacy_accounts = ["pointercrate-user/legacy_accounts"]

‎pointercrate-user-api/src/endpoints/auth.rs

+7-2
Original file line numberDiff line numberDiff line change
@@ -2,20 +2,25 @@ use crate::{
22
auth::{BasicAuth, TokenAuth},
33
ratelimits::UserRatelimits,
44
};
5-
use pointercrate_core::{etag::Taggable, pool::PointercratePool};
5+
use pointercrate_core::etag::Taggable;
6+
#[cfg(feature = "legacy_accounts")]
7+
use pointercrate_core::pool::PointercratePool;
68
use pointercrate_core_api::{
79
error::Result,
810
etag::{Precondition, Tagged},
911
response::Response2,
1012
};
11-
use pointercrate_user::{error::UserError, AuthenticatedUser, PatchMe, Registration, User};
13+
#[cfg(feature = "legacy_accounts")]
14+
use pointercrate_user::{AuthenticatedUser, Registration};
15+
use pointercrate_user::{error::UserError, PatchMe, User};
1216
use rocket::{
1317
http::Status,
1418
serde::json::{serde_json, Json},
1519
State,
1620
};
1721
use std::net::IpAddr;
1822

23+
#[cfg(feature = "legacy_accounts")]
1924
#[rocket::post("/register", data = "<body>")]
2025
pub async fn register(
2126
ip: IpAddr, body: Json<Registration>, ratelimits: &State<UserRatelimits>, pool: &State<PointercratePool>,

‎pointercrate-user-api/src/lib.rs

+16-15
Original file line numberDiff line numberDiff line change
@@ -7,22 +7,26 @@ mod endpoints;
77
mod pages;
88
mod ratelimits;
99

10+
#[allow(unused_mut)]
1011
pub fn setup(rocket: Rocket<Build>) -> Rocket<Build> {
1112
let ratelimits = UserRatelimits::new();
1213

14+
let mut auth_routes = rocket::routes![
15+
endpoints::auth::login,
16+
endpoints::auth::invalidate,
17+
endpoints::auth::get_me,
18+
endpoints::auth::patch_me,
19+
endpoints::auth::delete_me,
20+
];
21+
let mut page_routes = rocket::routes![pages::login_page, pages::account_page, pages::login];
22+
#[cfg(feature = "legacy_accounts")]
23+
auth_routes.extend(rocket::routes![endpoints::auth::register]);
24+
#[cfg(feature = "legacy_accounts")]
25+
page_routes.extend(rocket::routes![pages::register]);
26+
1327
rocket
1428
.manage(ratelimits)
15-
.mount(
16-
"/api/v1/auth/",
17-
rocket::routes![
18-
endpoints::auth::register,
19-
endpoints::auth::login,
20-
endpoints::auth::invalidate,
21-
endpoints::auth::get_me,
22-
endpoints::auth::patch_me,
23-
endpoints::auth::delete_me,
24-
],
25-
)
29+
.mount("/api/v1/auth/", auth_routes)
2630
.mount(
2731
"/api/v1/users/",
2832
rocket::routes![
@@ -32,8 +36,5 @@ pub fn setup(rocket: Rocket<Build>) -> Rocket<Build> {
3236
endpoints::user::delete_user
3337
],
3438
)
35-
.mount(
36-
"/",
37-
rocket::routes![pages::login_page, pages::account_page, pages::login, pages::register],
38-
)
39+
.mount("/", page_routes)
3940
}

‎pointercrate-user-api/src/pages.rs

+9-3
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,20 @@ use crate::{
22
auth::{BasicAuth, TokenAuth},
33
ratelimits::UserRatelimits,
44
};
5-
use pointercrate_core::{permission::PermissionsManager, pool::PointercratePool};
5+
use pointercrate_core::permission::PermissionsManager;
6+
#[cfg(feature = "legacy_accounts")]
7+
use pointercrate_core::pool::PointercratePool;
68
use pointercrate_core_api::response::Page;
79
use pointercrate_core_pages::head::HeadLike;
8-
use pointercrate_user::{error::UserError, AuthenticatedUser, Registration, User};
10+
use pointercrate_user::error::UserError;
11+
#[cfg(feature = "legacy_accounts")]
12+
use pointercrate_user::{AuthenticatedUser, Registration, User};
913
use pointercrate_user_pages::account::AccountPageConfig;
14+
#[cfg(feature = "legacy_accounts")]
15+
use rocket::serde::json::Json;
1016
use rocket::{
1117
http::{Cookie, CookieJar, SameSite, Status},
1218
response::Redirect,
13-
serde::json::Json,
1419
State,
1520
};
1621
use std::net::IpAddr;
@@ -43,6 +48,7 @@ pub async fn login(
4348
Ok(Status::NoContent)
4449
}
4550

51+
#[cfg(feature = "legacy_accounts")]
4652
#[rocket::post("/register", data = "<registration>")]
4753
pub async fn register(
4854
ip: IpAddr, ratelimits: &State<UserRatelimits>, cookies: &CookieJar<'_>, registration: Json<Registration>,

‎pointercrate-user-pages/Cargo.toml

+3
Original file line numberDiff line numberDiff line change
@@ -12,3 +12,6 @@ pointercrate-core = {path = "../pointercrate-core"}
1212
pointercrate-user = {path = "../pointercrate-user"}
1313
pointercrate-core-pages = {path = "../pointercrate-core-pages"}
1414
async-trait = "0.1.81"
15+
16+
[features]
17+
legacy_accounts = ["pointercrate-user/legacy_accounts"]

‎pointercrate-user/Cargo.toml

+3
Original file line numberDiff line numberDiff line change
@@ -19,3 +19,6 @@ lazy_static = "1.5.0"
1919
bcrypt = "0.15.1"
2020
url = "2.5.2"
2121
serde_json = "1.0.127"
22+
23+
[features]
24+
legacy_accounts = []

‎pointercrate-user/src/auth/get.rs

+8-2
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@ use log::{debug, info};
1010
use pointercrate_core::error::CoreError;
1111
use sqlx::{Error, PgConnection};
1212

13+
use super::AuthenticationMethod;
14+
1315
impl AuthenticatedUser {
1416
pub async fn basic_auth(username: &str, password: &str, connection: &mut PgConnection) -> Result<AuthenticatedUser> {
1517
info!("We are expected to perform basic authentication");
@@ -59,7 +61,9 @@ impl AuthenticatedUser {
5961
Err(err) => Err(err.into()),
6062
Ok(row) => Ok(AuthenticatedUser {
6163
user: construct_from_row!(row),
62-
password_hash: row.password_hash,
64+
auth_method: AuthenticationMethod:: Legacy {
65+
password_hash: row.password_hash,
66+
}
6367
}),
6468
}
6569
}
@@ -77,7 +81,9 @@ impl AuthenticatedUser {
7781
Err(err) => Err(err.into()),
7882
Ok(row) => Ok(AuthenticatedUser {
7983
user: construct_from_row!(row),
80-
password_hash: row.password_hash,
84+
auth_method: AuthenticationMethod:: Legacy {
85+
password_hash: row.password_hash,
86+
}
8187
}),
8288
}
8389
}

‎pointercrate-user/src/auth/mod.rs

+41-19
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,9 @@
55
//! * Deletion of own account
66
//! * Modification of own account
77
8-
pub use self::{patch::PatchMe, post::Registration};
8+
#[cfg(feature = "legacy_accounts")]
9+
pub use self::post::Registration;
10+
pub use self::patch::PatchMe;
911
use crate::{
1012
error::{Result, UserError},
1113
User,
@@ -26,7 +28,11 @@ mod post;
2628

2729
pub struct AuthenticatedUser {
2830
user: User,
29-
password_hash: String,
31+
auth_method: AuthenticationMethod,
32+
}
33+
34+
pub enum AuthenticationMethod {
35+
Legacy { password_hash: String },
3036
}
3137

3238
#[derive(Debug, Deserialize, Serialize, Copy, Clone)]
@@ -147,31 +153,41 @@ impl AuthenticatedUser {
147153
}
148154

149155
fn password_salt(&self) -> Vec<u8> {
150-
let raw_parts: Vec<_> = self.password_hash.split('$').filter(|s| !s.is_empty()).collect();
156+
match &self.auth_method {
157+
AuthenticationMethod::Legacy { password_hash } => {
158+
let raw_parts: Vec<_> = password_hash.split('$').filter(|s| !s.is_empty()).collect();
151159

152-
match &raw_parts[..] {
153-
[_, _, hash] => b64::decode(&hash[..22]),
154-
_ => unreachable!(),
160+
match &raw_parts[..] {
161+
[_, _, hash] => b64::decode(&hash[..22]),
162+
_ => unreachable!(),
163+
}
164+
},
165+
_ => Vec::new(),
155166
}
156167
}
157168

158169
pub fn verify_password(self, password: &str) -> Result<Self> {
159-
debug!("Verifying a password!");
170+
match &self.auth_method {
171+
AuthenticationMethod::Legacy { password_hash } => {
172+
debug!("Verifying a password!");
160173

161-
let valid = bcrypt::verify(password, &self.password_hash).map_err(|err| {
162-
warn!("Password verification FAILED for account {}: {}", self.user, err);
174+
let valid = bcrypt::verify(password, password_hash).map_err(|err| {
175+
warn!("Password verification FAILED for account {}: {}", self.user, err);
163176

164-
UserError::Core(CoreError::Unauthorized)
165-
})?;
177+
UserError::Core(CoreError::Unauthorized)
178+
})?;
166179

167-
if valid {
168-
debug!("Password correct, proceeding");
180+
if valid {
181+
debug!("Password correct, proceeding");
169182

170-
Ok(self)
171-
} else {
172-
warn!("Potentially malicious log-in attempt to account {}", self.user);
183+
Ok(self)
184+
} else {
185+
warn!("Potentially malicious log-in attempt to account {}", self.user);
173186

174-
Err(CoreError::Unauthorized.into())
187+
Err(CoreError::Unauthorized.into())
188+
}
189+
},
190+
_ => Err(CoreError::Unauthorized.into()),
175191
}
176192
}
177193
}
@@ -180,6 +196,8 @@ impl AuthenticatedUser {
180196
mod tests {
181197
use crate::{AuthenticatedUser, User};
182198

199+
use super::AuthenticationMethod;
200+
183201
fn patrick() -> AuthenticatedUser {
184202
AuthenticatedUser {
185203
user: User {
@@ -189,7 +207,9 @@ mod tests {
189207
display_name: None,
190208
youtube_channel: None,
191209
},
192-
password_hash: bcrypt::hash("bad password", bcrypt::DEFAULT_COST).unwrap(),
210+
auth_method: AuthenticationMethod::Legacy {
211+
password_hash: bcrypt::hash("bad password", bcrypt::DEFAULT_COST).unwrap(),
212+
},
193213
}
194214
}
195215

@@ -202,7 +222,9 @@ mod tests {
202222
display_name: None,
203223
youtube_channel: None,
204224
},
205-
password_hash: bcrypt::hash("bad password", bcrypt::DEFAULT_COST).unwrap(),
225+
auth_method: AuthenticationMethod::Legacy {
226+
password_hash: bcrypt::hash("bad password", bcrypt::DEFAULT_COST).unwrap(),
227+
},
206228
}
207229
}
208230

0 commit comments

Comments
 (0)