mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-21 00:02:30 +00:00
fix: skip instance group members that are not email addresses (#11128)
* fix: skip instance group members that are not email addresses * fix: keep provisioned members whose address only proper_email accepts * fix: judge instance group members by a mirror of the usr email constraint * fix: fold ascii only in the proper_email mirror, like the constraint * fix: let the database judge which instance group members usr will store * fix: cut a derived username to the column width so a long local part can be provisioned * chore: move the ee pin to the scim member doc fix * chore: update ee-repo-ref to 0780955effb657807d14f0eb503cba1d49cee007 This commit updates the EE repository reference after PR #801 was merged in windmill-ee-private. Previous ee-repo-ref: ee6452d489563204a98df883703f78d5e74cdd69 New ee-repo-ref: 0780955effb657807d14f0eb503cba1d49cee007 Automated by sync-ee-ref workflow. --------- Co-authored-by: windmill-internal-app[bot] <windmill-internal-app[bot]@users.noreply.github.com>
This commit is contained in:
Ruben Fiszel
co-authored by
windmill-internal-app[bot]
windmill-internal-app[bot]
parent
75ee497011
commit
e3e638f7f5
Generated
+1
@@ -15253,6 +15253,7 @@ dependencies = [
|
||||
"serde_json",
|
||||
"sql-builder",
|
||||
"sqlx",
|
||||
"tracing",
|
||||
"uuid",
|
||||
"windmill-api-auth",
|
||||
"windmill-api-workspaces",
|
||||
|
||||
@@ -1 +1 @@
|
||||
1ba6fe83451f0a1f8fafe04b7187087d51e0f769
|
||||
0780955effb657807d14f0eb503cba1d49cee007
|
||||
|
||||
@@ -29,4 +29,5 @@ serde.workspace = true
|
||||
serde_json.workspace = true
|
||||
sql-builder.workspace = true
|
||||
sqlx.workspace = true
|
||||
tracing.workspace = true
|
||||
uuid.workspace = true
|
||||
|
||||
@@ -22,7 +22,10 @@ use windmill_common::{
|
||||
error::{Error, JsonResult, Result},
|
||||
utils::{not_found_if_none, paginate, Pagination},
|
||||
};
|
||||
use windmill_common::{db::UserDB, users::username_to_permissioned_as};
|
||||
use windmill_common::{
|
||||
db::UserDB,
|
||||
users::{username_to_permissioned_as, usr_accepts_email},
|
||||
};
|
||||
|
||||
use serde::{Deserialize, Serialize};
|
||||
use sqlx::{query_scalar, FromRow, Postgres, Transaction};
|
||||
@@ -972,6 +975,15 @@ async fn add_user_igroup(
|
||||
) -> Result<String> {
|
||||
require_super_admin(&db, &authed).await?;
|
||||
|
||||
// `email_to_igroup` has no shape constraint of its own; `usr`, which the member is
|
||||
// promoted into on reconcile, has `proper_email`, and a value failing it there would
|
||||
// roll back every member of the group.
|
||||
if !usr_accepts_email(&db, &email).await? {
|
||||
return Err(Error::BadRequest(format!(
|
||||
"'{email}' is not a valid email address"
|
||||
)));
|
||||
}
|
||||
|
||||
let mut tx: Transaction<'_, Postgres> = db.begin().await?;
|
||||
|
||||
// FOR UPDATE: the group row is the group-level mutex, taken before the workspace
|
||||
@@ -1424,6 +1436,16 @@ async fn overwrite_igroups(
|
||||
|
||||
if let Some(emails) = &igroup.emails {
|
||||
for email in emails.iter() {
|
||||
// An export can carry a member the source instance stored before ingest
|
||||
// validated member values; it is dropped rather than failing the import.
|
||||
if !usr_accepts_email(&mut *tx, email).await? {
|
||||
tracing::warn!(
|
||||
"Skipping member '{}' of imported instance group '{}': not an email address",
|
||||
email,
|
||||
igroup.name
|
||||
);
|
||||
continue;
|
||||
}
|
||||
sqlx::query!(
|
||||
"INSERT INTO email_to_igroup (email, igroup) VALUES ($1, $2)",
|
||||
email,
|
||||
|
||||
@@ -913,3 +913,140 @@ async fn test_preserve_orphaned_members_migration(db: Pool<Postgres>) -> anyhow:
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// A membership row whose value is not an email (an IdP object id a SCIM sync stored before
|
||||
/// member values were validated) must not break the workspace's instance-group save: the
|
||||
/// reconciler skips it and still provisions the valid members. The admin endpoint refuses to
|
||||
/// add such a value in the first place.
|
||||
#[cfg(feature = "private")]
|
||||
#[sqlx::test(migrations = "../migrations", fixtures("base"))]
|
||||
async fn test_instance_group_member_that_is_not_an_email_is_skipped(
|
||||
db: Pool<Postgres>,
|
||||
) -> anyhow::Result<()> {
|
||||
initialize_tracing().await;
|
||||
let server = ApiServer::start(db.clone()).await?;
|
||||
let port = server.addr.port();
|
||||
let global_base = format!("http://localhost:{port}/api/groups");
|
||||
let ws_base = format!("http://localhost:{port}/api/w/test-workspace/workspaces");
|
||||
const ENTRA_OBJECT_ID: &str = "ef40ea04-1a9e-4a84-9e65-cb1baa81dfed";
|
||||
|
||||
let resp = authed(client().post(format!("{global_base}/create")))
|
||||
.json(&json!({ "name": "entra_grp" }))
|
||||
.send()
|
||||
.await?;
|
||||
assert_eq!(resp.status(), 200, "create");
|
||||
let resp = authed(client().post(format!("{global_base}/adduser/entra_grp")))
|
||||
.json(&json!({ "email": "kept@example.com" }))
|
||||
.send()
|
||||
.await?;
|
||||
assert_eq!(resp.status(), 200, "adduser");
|
||||
|
||||
let resp = authed(client().post(format!("{global_base}/adduser/entra_grp")))
|
||||
.json(&json!({ "email": ENTRA_OBJECT_ID }))
|
||||
.send()
|
||||
.await?;
|
||||
assert_eq!(
|
||||
resp.status(),
|
||||
400,
|
||||
"adduser must refuse a value that is not an email"
|
||||
);
|
||||
let too_wide = format!("{}@example.com", "a".repeat(244));
|
||||
let resp = authed(client().post(format!("{global_base}/adduser/entra_grp")))
|
||||
.json(&json!({ "email": too_wide }))
|
||||
.send()
|
||||
.await?;
|
||||
assert_eq!(
|
||||
resp.status(),
|
||||
400,
|
||||
"adduser must refuse a value wider than the email columns"
|
||||
);
|
||||
// A valid address whose local part is wider than the username columns: the derived
|
||||
// username is cut to fit rather than failing the promotion.
|
||||
let long_local_part = format!("{}@example.com", "a".repeat(60));
|
||||
let resp = authed(client().post(format!("{global_base}/adduser/entra_grp")))
|
||||
.json(&json!({ "email": long_local_part }))
|
||||
.send()
|
||||
.await?;
|
||||
assert_eq!(resp.status(), 200, "adduser long local part");
|
||||
|
||||
sqlx::query("INSERT INTO email_to_igroup (email, igroup) VALUES ($1, 'entra_grp')")
|
||||
.bind(ENTRA_OBJECT_ID)
|
||||
.execute(&db)
|
||||
.await?;
|
||||
|
||||
// A member whose address only the wider `proper_email` of `usr` accepts, already
|
||||
// provisioned through the group: reconciliation must keep and re-role them, since
|
||||
// removal destroys their drafts, inputs and permissions.
|
||||
sqlx::raw_sql(
|
||||
r#"
|
||||
INSERT INTO email_to_igroup (email, igroup) VALUES ('"quoted"@example.com', 'entra_grp');
|
||||
INSERT INTO usr (workspace_id, username, email, is_admin, operator, added_via)
|
||||
VALUES ('test-workspace', 'quoted', '"quoted"@example.com', false, true,
|
||||
'{"source": "instance_group", "group": "entra_grp"}'::jsonb);
|
||||
"#,
|
||||
)
|
||||
.execute(&db)
|
||||
.await?;
|
||||
|
||||
let resp = authed(client().post(format!("{ws_base}/edit_instance_groups")))
|
||||
.json(&json!({
|
||||
"groups": ["entra_grp"],
|
||||
"roles": { "entra_grp": "developer" }
|
||||
}))
|
||||
.send()
|
||||
.await?;
|
||||
assert_eq!(resp.status(), 200, "edit: {}", resp.text().await?);
|
||||
|
||||
let mut members: Vec<(String, bool)> = sqlx::query_as(
|
||||
"SELECT email, operator FROM usr WHERE workspace_id = 'test-workspace'
|
||||
AND added_via->>'source' = 'instance_group'",
|
||||
)
|
||||
.fetch_all(&db)
|
||||
.await?;
|
||||
members.sort();
|
||||
assert_eq!(
|
||||
members,
|
||||
vec![
|
||||
("\"quoted\"@example.com".to_string(), false),
|
||||
(long_local_part.clone(), false),
|
||||
("kept@example.com".to_string(), false),
|
||||
],
|
||||
"valid members provisioned and existing member kept, all as developers; non-email one skipped"
|
||||
);
|
||||
|
||||
// A full import carrying the same rows: the object id is dropped, the address only
|
||||
// `proper_email` accepts is kept, and neither member loses their workspace row.
|
||||
let resp = authed(client().post(format!("{global_base}/overwrite")))
|
||||
.json(&json!([{
|
||||
"name": "entra_grp",
|
||||
"emails": ["kept@example.com", "\"quoted\"@example.com", long_local_part, ENTRA_OBJECT_ID]
|
||||
}]))
|
||||
.send()
|
||||
.await?;
|
||||
assert_eq!(resp.status(), 200, "overwrite: {}", resp.text().await?);
|
||||
|
||||
let mut stored: Vec<String> =
|
||||
sqlx::query_scalar("SELECT email FROM email_to_igroup WHERE igroup = 'entra_grp'")
|
||||
.fetch_all(&db)
|
||||
.await?;
|
||||
stored.sort();
|
||||
assert_eq!(
|
||||
stored,
|
||||
vec![
|
||||
"\"quoted\"@example.com".to_string(),
|
||||
long_local_part.clone(),
|
||||
"kept@example.com".to_string(),
|
||||
],
|
||||
"import drops the object id and keeps the rest"
|
||||
);
|
||||
let mut after_import: Vec<(String, bool)> = sqlx::query_as(
|
||||
"SELECT email, operator FROM usr WHERE workspace_id = 'test-workspace'
|
||||
AND added_via->>'source' = 'instance_group'",
|
||||
)
|
||||
.fetch_all(&db)
|
||||
.await?;
|
||||
after_import.sort();
|
||||
assert_eq!(after_import, members, "import must not evict either member");
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -53,8 +53,8 @@ use windmill_common::per_minute_counter::PerMinuteCounter;
|
||||
use windmill_common::users::truncate_token;
|
||||
use windmill_common::users::COOKIE_NAME;
|
||||
use windmill_common::users::{
|
||||
username_to_permissioned_as, PERMISSIONED_AS_MAX_LEN, SUPERADMIN_NOTIFICATION_EMAIL,
|
||||
SUPERADMIN_SECRET_EMAIL, SUPERADMIN_SYNC_EMAIL, VALID_EMAIL,
|
||||
username_to_permissioned_as, EMAIL_COLUMN_MAX_LEN, PERMISSIONED_AS_MAX_LEN,
|
||||
SUPERADMIN_NOTIFICATION_EMAIL, SUPERADMIN_SECRET_EMAIL, SUPERADMIN_SYNC_EMAIL, VALID_EMAIL,
|
||||
};
|
||||
use windmill_common::utils::paginate;
|
||||
use windmill_common::worker::CLOUD_HOSTED;
|
||||
@@ -1758,7 +1758,6 @@ struct ChangeUserEmail {
|
||||
/// `varchar(50)`, and `v2_job.permissioned_as` in a `varchar(55)`; every other email column is
|
||||
/// `varchar(255)`. The strictest of the two bounds is used for all of them.
|
||||
const SHORT_EMAIL_COLUMN_MAX_LEN: usize = 50;
|
||||
const EMAIL_COLUMN_MAX_LEN: usize = 255;
|
||||
|
||||
/// Move an account to a new email address, in place: the `password` row (and with it the
|
||||
/// instance-wide username, the role and the login type) is kept and every email-keyed row is
|
||||
@@ -3253,7 +3252,10 @@ mod same_origin_rd_tests {
|
||||
|
||||
/// Both provisioning writes reference `password(email)`; a typo'd address from the
|
||||
/// provisioning script should read as "no such account", not as a foreign-key error.
|
||||
async fn require_account(tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, email: &str) -> Result<()> {
|
||||
async fn require_account(
|
||||
tx: &mut sqlx::Transaction<'_, sqlx::Postgres>,
|
||||
email: &str,
|
||||
) -> Result<()> {
|
||||
let exists = sqlx::query_scalar!(
|
||||
"SELECT EXISTS(SELECT 1 FROM password WHERE email = $1)",
|
||||
email
|
||||
|
||||
@@ -17,6 +17,23 @@ lazy_static::lazy_static! {
|
||||
pub static ref VALID_USERNAME: Regex = Regex::new(r#"^[a-zA-Z][a-zA-Z_0-9]*$"#).unwrap();
|
||||
}
|
||||
|
||||
/// Width of the `username` columns of `usr`, `password` and `pending_user`.
|
||||
pub const USERNAME_MAX_LEN: usize = 50;
|
||||
|
||||
/// `base` with the collision suffix of `attempt` appended (none for the first attempt), cut
|
||||
/// to `USERNAME_MAX_LEN`. A local part longer than the column is a valid email, and an
|
||||
/// insert that fails on the derived username rolls back everything around it.
|
||||
pub fn fit_username(base: &str, attempt: u32) -> String {
|
||||
let suffix = if attempt > 1 {
|
||||
attempt.to_string()
|
||||
} else {
|
||||
String::new()
|
||||
};
|
||||
let mut username: String = base.chars().take(USERNAME_MAX_LEN - suffix.len()).collect();
|
||||
username.push_str(&suffix);
|
||||
username
|
||||
}
|
||||
|
||||
pub async fn generate_instance_wide_unique_username<'c>(
|
||||
tx: &mut Transaction<'c, Postgres>,
|
||||
email: &str,
|
||||
@@ -41,9 +58,7 @@ pub async fn generate_instance_wide_unique_username<'c>(
|
||||
email
|
||||
)));
|
||||
}
|
||||
if i > 1 {
|
||||
username = format!("{}{}", base_username, i)
|
||||
}
|
||||
username = fit_username(&base_username, i);
|
||||
username_conflict = sqlx::query_scalar!(
|
||||
"SELECT EXISTS(SELECT 1 FROM usr WHERE username = $1 and email != $2 UNION SELECT 1 FROM password WHERE username = $1 UNION SELECT 1 FROM pending_user WHERE username = $1)",
|
||||
&username,
|
||||
@@ -164,3 +179,19 @@ pub async fn get_instance_username_or_create_pending<'c>(
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn test_fit_username_keeps_the_column_width() {
|
||||
assert_eq!(fit_username("alice", 1), "alice");
|
||||
assert_eq!(fit_username("alice", 2), "alice2");
|
||||
let base = "a".repeat(60);
|
||||
assert_eq!(fit_username(&base, 1), "a".repeat(USERNAME_MAX_LEN));
|
||||
let with_suffix = fit_username(&base, 1000);
|
||||
assert_eq!(with_suffix.len(), USERNAME_MAX_LEN);
|
||||
assert!(with_suffix.ends_with("1000"));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -13,6 +13,35 @@ lazy_static::lazy_static! {
|
||||
pub static ref VALID_EMAIL: regex::Regex = regex::Regex::new(
|
||||
r"^[A-Za-z0-9!#$%&'*+/=?^_`{|}~-]+(\.[A-Za-z0-9!#$%&'*+/=?^_`{|}~-]+)*@([A-Za-z0-9]([A-Za-z0-9-]*[A-Za-z0-9])?\.)+[A-Za-z0-9]([A-Za-z0-9-]*[A-Za-z0-9])?$"
|
||||
).unwrap();
|
||||
|
||||
}
|
||||
|
||||
/// Width of the `email` columns of `usr`, `workspace_invite` and `email_to_igroup`.
|
||||
pub const EMAIL_COLUMN_MAX_LEN: usize = 255;
|
||||
|
||||
/// The regex of the `proper_email` CHECK constraint on `usr` and `workspace_invite`
|
||||
/// (`20220620210708_regex_fix`), verbatim, for [`usr_accepts_email`]. Evaluated by the
|
||||
/// database and never by a Rust engine: `~*` folds case under the database collation, so a
|
||||
/// fixed mirror accepts addresses the constraint rejects, or rejects ones it holds, on some
|
||||
/// locale. `windmill-common/tests/usr_accepts_email.rs` pins the text to the constraint.
|
||||
pub const PROPER_EMAIL_PATTERN: &str = r#"^(?:[a-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)*|"(?:[\x01-\x08\x0b\x0c\x0e-\x1f\x21\x23-\x5b\x5d-\x7f]|\\[\x01-\x09\x0b\x0c\x0e-\x7f])*")@(?:(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?|\[(?:(?:(2(5[0-5]|[0-4][0-9])|1[0-9][0-9]|[1-9]?[0-9]))\.){3}(?:(2(5[0-5]|[0-4][0-9])|1[0-9][0-9]|[1-9]?[0-9])|[a-z0-9-]*[a-z0-9]:(?:[\x01-\x08\x0b\x0c\x0e-\x1f\x21-\x5a\x53-\x7f]|\\[\x01-\x09\x0b\x0c\x0e-\x7f])+)\])$"#;
|
||||
|
||||
/// Whether `usr` (and `workspace_invite`) will store `email`: the `proper_email` regex as the
|
||||
/// database evaluates it, plus the column width. Unlike [`VALID_EMAIL`] this admits every
|
||||
/// address those tables already hold, which matters wherever an existing member is judged.
|
||||
pub async fn usr_accepts_email<'c, E>(db: E, email: &str) -> crate::error::Result<bool>
|
||||
where
|
||||
E: sqlx::Executor<'c, Database = sqlx::Postgres>,
|
||||
{
|
||||
if email.contains('\0') || email.chars().count() > EMAIL_COLUMN_MAX_LEN {
|
||||
return Ok(false);
|
||||
}
|
||||
let accepted: bool = sqlx::query_scalar("SELECT $1::text ~* $2::text")
|
||||
.bind(email)
|
||||
.bind(PROPER_EMAIL_PATTERN)
|
||||
.fetch_one(db)
|
||||
.await?;
|
||||
Ok(accepted)
|
||||
}
|
||||
|
||||
pub const SUPERADMIN_SECRET_EMAIL: &str = "superadmin_secret@windmill.dev";
|
||||
|
||||
@@ -0,0 +1,59 @@
|
||||
//! `usr_accepts_email` predicts whether `usr` will store an address by evaluating
|
||||
//! `PROPER_EMAIL_PATTERN` in the database. It only stays right while that text matches the
|
||||
//! `proper_email` constraint and the width matches the column: each sample below must be
|
||||
//! stored by `usr` exactly when the check accepts it, and everything `VALID_EMAIL` accepts
|
||||
//! within the width must be stored too.
|
||||
|
||||
use sqlx::{Pool, Postgres};
|
||||
use windmill_common::users::{usr_accepts_email, EMAIL_COLUMN_MAX_LEN, VALID_EMAIL};
|
||||
|
||||
#[sqlx::test(migrations = "../migrations")]
|
||||
async fn usr_accepts_email_agrees_with_the_constraint(db: Pool<Postgres>) -> anyhow::Result<()> {
|
||||
let domain = "@example.com";
|
||||
let widest = format!(
|
||||
"{}{domain}",
|
||||
"a".repeat(EMAIL_COLUMN_MAX_LEN - domain.len())
|
||||
);
|
||||
let too_wide = format!("a{widest}");
|
||||
for email in [
|
||||
"alice@example.com",
|
||||
"Alice@Example.COM",
|
||||
"alice.bob+tag@sub.example.co.uk",
|
||||
"\"quoted\"@example.com",
|
||||
"\"quoted local\"@example.com",
|
||||
"alice@[192.168.0.1]",
|
||||
widest.as_str(),
|
||||
too_wide.as_str(),
|
||||
"ef40ea04-1a9e-4a84-9e65-cb1baa81dfed",
|
||||
// Unicode case folding would map the long s and the Kelvin sign into `[a-z]`.
|
||||
"u\u{17f}er@example.com",
|
||||
"alice@example\u{212a}.com",
|
||||
"alice",
|
||||
"alice@example",
|
||||
"alice@@example.com",
|
||||
"alice @example.com",
|
||||
"alice@example.com\nbob@example.com",
|
||||
"",
|
||||
] {
|
||||
let mut tx = db.begin().await?;
|
||||
let stored = sqlx::query(
|
||||
"INSERT INTO usr (workspace_id, username, email, is_admin, operator)
|
||||
VALUES ('admins', 'probe', $1, false, false)",
|
||||
)
|
||||
.bind(email)
|
||||
.execute(&mut *tx)
|
||||
.await
|
||||
.is_ok();
|
||||
tx.rollback().await?;
|
||||
|
||||
assert_eq!(
|
||||
stored,
|
||||
usr_accepts_email(&db, email).await?,
|
||||
"{email:?}: `usr` and usr_accepts_email disagree"
|
||||
);
|
||||
if VALID_EMAIL.is_match(email) && email.len() <= EMAIL_COLUMN_MAX_LEN {
|
||||
assert!(stored, "{email:?}: VALID_EMAIL accepts what `usr` rejects");
|
||||
}
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
Reference in New Issue
Block a user