Skip to content

Commit 9888388

Browse files
committed
feat: accept UUID v4 API keys and simplify user credentials
1 parent 3397c9a commit 9888388

5 files changed

Lines changed: 45 additions & 61 deletions

File tree

‎src/apikeys.rs‎

Lines changed: 11 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -30,25 +30,22 @@ use serde::Deserialize;
3030
/// Request body for creating a new API key. `roles` is a set of role names
3131
/// that must already exist in the tenant; permissions for the backing user
3232
/// are derived from these roles (same flow as native/OAuth users).
33-
/// `providedApiKey` lets a caller register their own key; omitting it preserves
33+
/// `apiKey` lets a caller register their own key; omitting it preserves
3434
/// the existing server-generated key behavior.
3535
#[derive(Debug, Deserialize)]
36-
#[serde(rename_all = "camelCase")]
36+
#[serde(rename_all = "camelCase", deny_unknown_fields)]
3737
pub struct CreateApiKeyRequest {
3838
pub key_name: String,
3939
#[serde(default)]
4040
pub roles: HashSet<String>,
41-
pub provided_api_key: Option<String>,
41+
pub api_key: Option<String>,
4242
}
4343

44-
/// Reject short or malformed caller-supplied credentials before persisting them.
45-
pub fn validate_provided_api_key(value: &str) -> Result<(), ApiKeyError> {
46-
if !(32..=256).contains(&value.len())
47-
|| !value
48-
.bytes()
49-
.all(|byte| byte.is_ascii_alphanumeric() || byte == b'-' || byte == b'_')
50-
{
51-
return Err(ApiKeyError::InvalidProvidedApiKey);
44+
/// Require a hyphenated UUID v4, allowing either letter case.
45+
pub fn validate_api_key(value: &str) -> Result<(), ApiKeyError> {
46+
let uuid = uuid::Uuid::parse_str(value).map_err(|_| ApiKeyError::InvalidApiKey)?;
47+
if value.len() != 36 || uuid.get_version_num() != 4 {
48+
return Err(ApiKeyError::InvalidApiKey);
5249
}
5350
Ok(())
5451
}
@@ -64,8 +61,8 @@ pub enum ApiKeyError {
6461
#[error("API key value is already registered")]
6562
DuplicateApiKey,
6663

67-
#[error("providedApiKey must be 32-256 ASCII letters, digits, hyphens, or underscores")]
68-
InvalidProvidedApiKey,
64+
#[error("apiKey must be a UUID v4 in 8-4-4-4-12 format")]
65+
InvalidApiKey,
6966

7067
#[error("Unauthorized: {0}")]
7168
Unauthorized(String),
@@ -90,7 +87,7 @@ impl actix_web::ResponseError for ApiKeyError {
9087
ApiKeyError::DuplicateKeyName(_) | ApiKeyError::DuplicateApiKey => {
9188
actix_web::http::StatusCode::CONFLICT
9289
}
93-
ApiKeyError::InvalidProvidedApiKey => actix_web::http::StatusCode::BAD_REQUEST,
90+
ApiKeyError::InvalidApiKey => actix_web::http::StatusCode::BAD_REQUEST,
9491
ApiKeyError::Unauthorized(_) => actix_web::http::StatusCode::FORBIDDEN,
9592
ApiKeyError::RevocationSyncFailed(_) => {
9693
actix_web::http::StatusCode::SERVICE_UNAVAILABLE

‎src/handlers/http/apikeys.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use serde::{Deserialize, Serialize};
66
use ulid::Ulid;
77

88
use crate::{
9-
apikeys::{ApiKeyError, CreateApiKeyRequest, validate_provided_api_key},
9+
apikeys::{ApiKeyError, CreateApiKeyRequest, validate_api_key as validate_api_key_value},
1010
handlers::http::{
1111
modal::utils::rbac_utils::{get_metadata, put_metadata},
1212
rbac::{RBACError, UPDATE_LOCK},
@@ -187,9 +187,9 @@ pub async fn create_api_key(
187187
}
188188

189189
let key_id = Ulid::new();
190-
let api_key_value = if let Some(provided_api_key) = body.provided_api_key {
191-
validate_provided_api_key(&provided_api_key)?;
192-
provided_api_key
190+
let api_key_value = if let Some(api_key) = body.api_key {
191+
validate_api_key_value(&api_key)?;
192+
api_key
193193
} else {
194194
generate_api_key_value()
195195
};

‎src/handlers/http/modal/query/querier_rbac.rs‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -45,19 +45,15 @@ use crate::{
4545
pub async fn post_user(
4646
req: HttpRequest,
4747
userid: web::Path<String>,
48-
body: Option<web::Json<serde_json::Value>>,
48+
web::Json(body): web::Json<user::CreateUserOptions>,
4949
) -> Result<impl Responder, RBACError> {
5050
let username = userid.into_inner();
5151
let tenant_id = get_tenant_id_from_request(&req);
5252
let caller_userid = get_user_from_request(&req)?;
5353
validator::user_role_name(&username)?;
5454
let mut metadata = get_metadata(&tenant_id).await?;
5555

56-
let (user_roles, provided_password) = body
57-
.map(|body| serde_json::from_value::<user::CreateUserRequest>(body.into_inner()))
58-
.transpose()?
59-
.map(user::CreateUserRequest::split_request)
60-
.unwrap_or_default();
56+
let (user_roles, password) = body.split_request();
6157

6258
let mut non_existent_roles = Vec::new();
6359
for role in &user_roles {
@@ -83,8 +79,8 @@ pub async fn post_user(
8379
return Err(RBACError::UserExists(username));
8480
}
8581

86-
let (mut user, password) = match provided_password {
87-
Some(password) => user::User::new_basic_with_provided_password(
82+
let (mut user, password) = match password {
83+
Some(password) => user::User::new_basic_with_password(
8884
username.clone(),
8985
tenant_id.clone(),
9086
false,

‎src/handlers/http/rbac.rs‎

Lines changed: 9 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -135,18 +135,14 @@ pub fn get_user_internal(
135135
pub async fn post_user(
136136
req: HttpRequest,
137137
userid: web::Path<String>,
138-
body: Option<web::Json<serde_json::Value>>,
138+
web::Json(body): web::Json<user::CreateUserOptions>,
139139
) -> Result<impl Responder, RBACError> {
140140
let userid = userid.into_inner();
141141
let tenant_id = get_tenant_id_from_request(&req);
142142
validator::user_role_name(&userid)?;
143143
let mut metadata = get_metadata(&tenant_id).await?;
144144

145-
let (user_roles, provided_password) = body
146-
.map(|body| serde_json::from_value::<user::CreateUserRequest>(body.into_inner()))
147-
.transpose()?
148-
.map(user::CreateUserRequest::split_request)
149-
.unwrap_or_default();
145+
let (user_roles, password) = body.split_request();
150146

151147
let mut non_existent_roles = Vec::new();
152148
for role in &user_roles {
@@ -171,13 +167,10 @@ pub async fn post_user(
171167
return Err(RBACError::UserExists(userid));
172168
}
173169

174-
let (user, password) = match provided_password {
175-
Some(password) => user::User::new_basic_with_provided_password(
176-
userid.clone(),
177-
tenant_id.clone(),
178-
false,
179-
password,
180-
)?,
170+
let (user, password) = match password {
171+
Some(password) => {
172+
user::User::new_basic_with_password(userid.clone(), tenant_id.clone(), false, password)?
173+
}
181174
None => user::User::new_basic(userid.clone(), tenant_id.clone(), false),
182175
};
183176

@@ -560,8 +553,8 @@ pub enum RBACError {
560553
ProtectedUser,
561554
#[error("Role is protected")]
562555
ProtectedRole,
563-
#[error("providedPassword must be 8-256 bytes with no control characters")]
564-
InvalidProvidedPassword,
556+
#[error("password must be 8-256 bytes with no control characters or surrounding whitespace")]
557+
InvalidPassword,
565558
#[error("{0}")]
566559
SerdeError(#[from] serde_json::Error),
567560
#[error("Failed to connect to storage: {0}")]
@@ -600,7 +593,7 @@ impl actix_web::ResponseError for RBACError {
600593
Self::UserExists(_) => StatusCode::BAD_REQUEST,
601594
Self::ProtectedUser => StatusCode::BAD_REQUEST,
602595
Self::ProtectedRole => StatusCode::BAD_REQUEST,
603-
Self::InvalidProvidedPassword => StatusCode::BAD_REQUEST,
596+
Self::InvalidPassword => StatusCode::BAD_REQUEST,
604597
Self::UserDoesNotExist => StatusCode::NOT_FOUND,
605598
Self::SerdeError(_) => StatusCode::BAD_REQUEST,
606599
Self::ValidationError(_) => StatusCode::BAD_REQUEST,

‎src/rbac/user.rs‎

Lines changed: 17 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -106,24 +106,24 @@ pub struct User {
106106
/// Accept the UI's existing roles array or an object with a caller-supplied password.
107107
#[derive(Deserialize)]
108108
#[serde(untagged)]
109-
pub enum CreateUserRequest {
109+
pub enum CreateUserOptions {
110110
Roles(HashSet<String>),
111-
Options(CreateUserOptions),
111+
Details(CreateUserDetails),
112112
}
113113

114114
#[derive(Deserialize)]
115115
#[serde(rename_all = "camelCase", deny_unknown_fields)]
116-
pub struct CreateUserOptions {
116+
pub struct CreateUserDetails {
117117
#[serde(default)]
118118
roles: HashSet<String>,
119-
provided_password: Option<String>,
119+
password: Option<String>,
120120
}
121121

122-
impl CreateUserRequest {
122+
impl CreateUserOptions {
123123
pub fn split_request(self) -> (HashSet<String>, Option<String>) {
124124
match self {
125125
Self::Roles(roles) => (roles, None),
126-
Self::Options(options) => (options.roles, options.provided_password),
126+
Self::Details(details) => (details.roles, details.password),
127127
}
128128
}
129129
}
@@ -134,7 +134,7 @@ impl User {
134134
Self::new_basic_with_passcode(username, tenant, protected, Basic::gen_new_password())
135135
}
136136

137-
pub fn new_basic_with_provided_password(
137+
pub fn new_basic_with_password(
138138
username: String,
139139
tenant: Option<String>,
140140
protected: bool,
@@ -144,7 +144,7 @@ impl User {
144144
|| password.trim() != password
145145
|| password.chars().any(char::is_control)
146146
{
147-
return Err(RBACError::InvalidProvidedPassword);
147+
return Err(RBACError::InvalidPassword);
148148
}
149149

150150
let hash = gen_hash(&password);
@@ -761,34 +761,32 @@ impl UserGroup {
761761
}
762762

763763
#[cfg(test)]
764-
mod provided_password_tests {
764+
mod password_request_tests {
765765
use super::*;
766766

767767
#[test]
768768
fn accepts_legacy_roles_array_and_new_password_object() {
769-
let legacy: CreateUserRequest = serde_json::from_str(r#"["reader"]"#).unwrap();
769+
let legacy: CreateUserOptions = serde_json::from_str(r#"["reader"]"#).unwrap();
770770
let (roles, password) = legacy.split_request();
771771
assert!(roles.contains("reader"));
772772
assert!(password.is_none());
773773

774-
let modern: CreateUserRequest = serde_json::from_str(
775-
r#"{"roles":["reader"],"providedPassword":"a_valid_password_123"}"#,
776-
)
777-
.unwrap();
774+
let modern: CreateUserOptions =
775+
serde_json::from_str(r#"{"roles":["reader"],"password":"a_valid_password_123"}"#)
776+
.unwrap();
778777
let (roles, password) = modern.split_request();
779778
assert!(roles.contains("reader"));
780779
assert_eq!(password.as_deref(), Some("a_valid_password_123"));
781780

782781
assert!(
783-
serde_json::from_str::<CreateUserRequest>(r#"{"providedPasswrod":"typo"}"#).is_err()
782+
serde_json::from_str::<CreateUserOptions>(r#"{"providedPassword":"old"}"#).is_err()
784783
);
785784
}
786785

787786
#[test]
788787
fn supplied_password_is_hashed_and_invalid_values_are_rejected() {
789788
let (user, returned) =
790-
User::new_basic_with_provided_password("alice".into(), None, false, "Pa$$w0rd".into())
791-
.unwrap();
789+
User::new_basic_with_password("alice".into(), None, false, "Pa$$w0rd".into()).unwrap();
792790
assert_eq!(returned, "Pa$$w0rd");
793791
let UserType::Native(basic) = &user.ty else {
794792
panic!("expected native user")
@@ -797,8 +795,8 @@ mod provided_password_tests {
797795
assert!(!serde_json::to_string(&user).unwrap().contains(&returned));
798796

799797
assert!(matches!(
800-
User::new_basic_with_provided_password("bob".into(), None, false, "short".into()),
801-
Err(RBACError::InvalidProvidedPassword)
798+
User::new_basic_with_password("bob".into(), None, false, "short".into()),
799+
Err(RBACError::InvalidPassword)
802800
));
803801
}
804802
}

0 commit comments

Comments
 (0)