Skip to content
Sign up

flagon-io/g1t

public
00

Where people and agents ship software together. The open-source git platform for the whole job: issues, agents, checks and deploys to the edge.

gitagentscloudflarerustforgemcp

Commit

Fix regressions found reviewing the SSO hardening

- SAML metadata works before a connection is saved: sso_summary names the workspace (its id and current name) whether or not it has one, so an owner can give their provider g1t's metadata first. - Pages reached by the workspace's id (the provider sends people back to it) show and link by its name now: the start page redirects an id or old name to /auth/sso/<name>, so members go straight on rather than being asked to join; problem pages and titles say the name. - A sign-in that SCIM unassigns midway, or whose g1t session ended, is refused rather than reported as done (the SSO session insert must land). - SCIM's optimistic write also checks the link it read, and every writer of scim_users.user_id bumps updated_at, so a PATCH never restores a link a sign-in or an account erasure just changed. - A pushed group reads back by its id (hex of its name), for Okta's group push; externalId is left out when not sent. - Deprovisioning revokes invitations only to addresses the person confirmed. - configure_sso treats a field sent as null as left out. End to end against a local identity worker and D1: all checks pass, including per-workspace isolation (a member held out of one workspace keeps their others, and a workspace with SSO set up but not required asks nothing). Refs #56

G1t-Session:
ses_01m54fhewne5tbgqqh4n8f5fvz
syntaqxcommitted Parente1b5230Browse files
11 files+104−330/11 viewed
+7−2
203203 let same = saved.filter(|saved| saved.protocol == protocol);
204204 if let Some(saved) = same {
205205 let kept = &saved.config;
206+ // Left out, or sent as null (some clients send every parameter).
207+ let absent = |field: &str| input.get(field).is_none_or(Value::is_null);
206208 let keep = |sent: &mut Option<String>, kept: &Option<String>, field: &str| {
207− if input.get(field).is_none() {
209+ if absent(field) {
208210 *sent = kept.clone();
209211 }
210212 };
218220 keep(&mut config.email_claim, &kept.email_claim, "email_claim");
219221 keep(&mut config.idp_entity_id, &kept.idp_entity_id, "idp_entity_id");
220222 keep(&mut config.idp_sso_url, &kept.idp_sso_url, "idp_sso_url");
221− if input.get("join_domains").is_none() {
223+ if absent("join_domains") {
222224 config.join_domains = kept.join_domains.clone();
223225 }
224226 }
379381 // Rotating the secret alone changes nothing else.
380382 let (config, allow_join) = merged(&json!({ "protocol": "oidc", "client_secret": "new" }), Some(&saved), SsoProtocol::Oidc);
381383 assert_eq!(config.issuer.as_deref(), Some("https://acme.okta.com"));
384+ let (nulls, _) = merged(&json!({ "protocol": "oidc", "issuer": null, "join_domains": null }), Some(&saved), SsoProtocol::Oidc);
385+ assert_eq!(nulls.issuer.as_deref(), Some("https://acme.okta.com"));
386+ assert_eq!(nulls.join_domains, vec!["acme.com".to_owned()]);
382387 assert_eq!(config.join_domains, vec!["acme.com".to_owned()]);
383388 assert!(allow_join);
384389 // Sent empty is changed; another protocol starts afresh.
+8−4
99 import { SsoProblem } from "./auth-sso";
1010
1111 export function meta({ params, ...args }: Route.MetaArgs) {
12− return page(args, { title: `Single sign-on · ${params.owner} · g1t` });
12+ return page(args, { title: `Single sign-on · ${args.loaderData?.slug ?? params.owner} · g1t` });
1313 }
1414
1515 /**
1919 * code, and links, admits and marks the session.
2020 */
2121 export async function loader({ request, params }: Route.LoaderArgs) {
22− const slug = params.owner.toLowerCase();
22+ // The provider sends people back to the workspace's id.
23+ const key = params.owner.toLowerCase();
2324 const url = new URL(request.url);
2425 const clear = cookie(SSO_STATE_COOKIE, "", 0);
25− const failed = (error: string) => data({ slug, error }, { headers: { "set-cookie": clear } });
26+ const failed = async (error: string) => {
27+ const named = await sso.summary(key).catch(() => null);
28+ return data({ slug: named?.slug || key, error }, { headers: { "set-cookie": clear } });
29+ };
2630 const refused = url.searchParams.get("error");
2731 if (refused) {
2832 const said = url.searchParams.get("error_description")?.slice(0, 300);
3539 }
3640 const token = sessionTokenOf(request);
3741 if (!token) return failed("Sign in to g1t again, then start single sign-on again.");
38− const finished = await sso.finish({ slug, session_token: token, state: state!, code });
42+ const finished = await sso.finish({ slug: key, session_token: token, state: state!, code });
3943 if (!finished.ok) return failed(finished.error.message);
4044 throw redirect(afterSso(finished.value), { headers: { "set-cookie": clear } });
4145 }
+2−2
1212 */
1313 export async function loader({ request, params }: Route.LoaderArgs) {
1414 // Keyed by the workspace's id, whatever names it here.
15− const summary = await sso.summary(params.owner);
15+ const summary = await sso.summary(params.owner.toLowerCase());
1616 if (!summary.workspace_id) throw data("No such workspace.", { status: 404 });
17− const addresses = ssoAddresses(new URL(request.url).origin, summary.workspace_id, params.owner);
17+ const addresses = ssoAddresses(new URL(request.url).origin, summary.workspace_id, summary.slug);
1818 const xml = await sso.metadata(addresses.entityId, addresses.acsUrl);
1919 return new Response(xml, {
2020 headers: {
+4−2
33 import type { Route } from "./+types/auth-sso-saml";
44 import { page } from "../lib/meta";
55 import { clearErrorCookie, readErrorCookie } from "../lib/sso";
6+import { sso } from "../lib/sso.server";
67 import { SsoProblem } from "./auth-sso";
78
89 export function meta({ params, ...args }: Route.MetaArgs) {
9− return page(args, { title: `Single sign-on · ${params.owner} · g1t` });
10+ return page(args, { title: `Single sign-on · ${args.loaderData?.slug ?? params.owner} · g1t` });
1011 }
1112
1213 /**
1516 * why it did not work. Opened by itself, it starts again.
1617 */
1718 export async function loader({ request, params }: Route.LoaderArgs) {
18− const slug = params.owner.toLowerCase();
19+ const key = params.owner.toLowerCase();
20+ const slug = (await sso.summary(key).catch(() => null))?.slug || key;
1921 const error = readErrorCookie(request.headers.get("cookie"));
2022 if (!error) throw redirect(`/auth/sso/${slug}`);
2123 return data({ slug, error }, { headers: { "set-cookie": clearErrorCookie() } });
+10−3
5454 */
5555 export async function loader({ request, params, context }: Route.LoaderArgs) {
5656 const user = requireUser(context, request);
57− const slug = params.owner.toLowerCase();
58− if (belongs(user, slug)) return start(request, user, slug, nextPath(request));
59− const summary = await sso.summary(slug);
57+ const key = params.owner.toLowerCase();
58+ const summary = await sso.summary(key);
59+ // Named by its id (from a page the provider sent them back to) or an old
60+ // name: to its name now, so pages and links say it.
61+ if (summary.slug && summary.slug !== key) {
62+ const url = new URL(request.url);
63+ throw redirect(`/auth/sso/${summary.slug}${url.search}`);
64+ }
65+ const slug = summary.slug || key;
6066 if (!summary.enabled) return data({ slug, error: `${slug} has not set up single sign-on.` }, { status: 404 });
67+ if (belongs(user, slug)) return start(request, user, slug, nextPath(request));
6168 return { slug, confirm: true as const, username: user.display_username ?? user.username, next: nextPath(request) };
6269 }
6370
+3−0
240240 /// and the notice for someone it holds out.
241241 #[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize)]
242242 pub struct SsoSummary {
243+ /// The workspace's id and name now, whatever named it: empty for none.
243244 #[serde(default)]
244245 pub workspace_id: String,
246+ #[serde(default)]
247+ pub slug: String,
245248 pub enabled: bool,
246249 pub protocol: Option<SsoProtocol>,
247250 pub required: bool,
+2−0
8585 };
8686
8787 export type SsoSummary = {
88+ /** The workspace's id and name now, whatever named it; empty when there is no such workspace. */
8889 workspace_id: string;
90+ slug: string;
8991 enabled: boolean;
9092 protocol: SsoProtocol | null;
9193 required: boolean;
+1−1
278278 // Deploy keys it added to repositories.
279279 ("DELETE FROM deploy_keys WHERE created_by = ?1".to_owned(), 1),
280280 // Its SCIM assignments link to nobody; the provider still has them.
281− ("UPDATE scim_users SET user_id = NULL WHERE user_id = ?1".to_owned(), 1),
281+ ("UPDATE scim_users SET user_id = NULL, updated_at = strftime('%Y-%m-%dT%H:%M:%fZ', 'now') WHERE user_id = ?1".to_owned(), 1),
282282 // g1t keeps no GitHub token for it any more.
283283 ("UPDATE github_accounts SET tokens = NULL WHERE user_id = ?1".to_owned(), 1),
284284 ("DELETE FROM repo_grants WHERE principal_kind = 'user' AND principal_id = ?1".to_owned(), 1),
+24−9
478478
479479 /// `sso_summary`: whether a workspace has single sign-on, for anyone.
480480 pub async fn sso_summary(&self, a: SlugArgs) -> Result<SsoSummary> {
481− let Some((workspace_id, _)) = self.sso_workspace(&a.slug).await? else {
481+ let Some((workspace_id, slug)) = self.sso_workspace(&a.slug).await? else {
482482 return Ok(SsoSummary::default());
483483 };
484+ // Which workspace, even before it has a connection: an owner gives
485+ // its provider g1t's metadata before saving one.
484486 let Some(row) = self.connection_where("workspace_id", &workspace_id).await? else {
485− return Ok(SsoSummary::default());
487+ return Ok(SsoSummary { workspace_id, slug, ..SsoSummary::default() });
486488 };
487489 Ok(SsoSummary {
488490 workspace_id: workspace_id.clone(),
491+ slug,
489492 enabled: true,
490493 protocol: Some(row.protocol()),
491494 required: self.require_sso_of(&workspace_id).await?,
705708 .prepare("DELETE FROM sso_sessions WHERE workspace_id = ? AND session_id IN (SELECT id FROM sessions WHERE user_id = ?)")
706709 .bind(&[workspace_id.as_str().into(), user_id.into()])?,
707710 self.db
708− .prepare("UPDATE scim_users SET user_id = NULL WHERE workspace_id = ? AND user_id = ?")
711+ .prepare(format!("UPDATE scim_users SET user_id = NULL, updated_at = {SQL_NOW} WHERE workspace_id = ? AND user_id = ?"))
709712 .bind(&[workspace_id.as_str().into(), user_id.into()])?,
710713 ])
711714 .await?;
10901093 }
10911094 if let Some(id) = &provisioned {
10921095 self.db
1093− .prepare("UPDATE scim_users SET user_id = ? WHERE id = ? AND workspace_id = ?")
1094− .bind(&[user_id.into(), id.as_str().into(), row.workspace_id.as_str().into()])?
1096+ .prepare("UPDATE scim_users SET user_id = ?, updated_at = ? WHERE id = ? AND workspace_id = ?")
1097+ .bind(&[user_id.into(), now.as_str().into(), id.as_str().into(), row.workspace_id.as_str().into()])?
10951098 .run()
10961099 .await?;
10971100 }
11171120 }
11181121 }
11191122 // The session counts as signed in through the provider, while it lasts.
1120− self.db
1123+ let counted = self
1124+ .db
11211125 .prepare(format!(
11221126 "INSERT INTO sso_sessions (session_id, workspace_id, expires_at)
11231127 SELECT ?1, ?2, {} WHERE EXISTS (SELECT 1 FROM sessions WHERE id = ?1 AND user_id = ?3 AND expires_at > {SQL_NOW}) AND {still_assigned}
1124− ON CONFLICT (session_id, workspace_id) DO UPDATE SET expires_at = excluded.expires_at",
1128+ ON CONFLICT (session_id, workspace_id) DO UPDATE SET expires_at = excluded.expires_at RETURNING session_id",
11251129 sql_after(SSO_SESSION_SECONDS)
11261130 ))
11271131 .bind(&[state.session_id.as_str().into(), row.workspace_id.as_str().into(), user_id.into(), assignment])?
1128− .run()
1129− .await?;
1132+ .first::<Value>(None)
1133+ .await?
1134+ .is_some();
1135+ if !counted {
1136+ // Unassigned while signing in, or the g1t session ended.
1137+ return Ok(match provisioned {
1138+ Some(_) => Outcome::fail(
1139+ FailureCode::Forbidden,
1140+ format!("Your account at {slug}'s identity provider is not assigned to g1t. Ask whoever runs it for {slug} to assign you."),
1141+ ),
1142+ None => Outcome::fail(FailureCode::Unauthenticated, "Sign in to g1t again."),
1143+ });
1144+ }
11301145 let first = self
11311146 .db
11321147 .prepare("UPDATE sso_connections SET tested_at = ?, tested_by = ? WHERE id = ? AND tested_at IS NULL RETURNING id")
+9−3
55
66 use std::collections::HashMap;
77
8+use serde_json::json;
9+
810 use super::*;
911
1012 #[derive(Deserialize)]
279281 ("POST", ["Groups"]) => answer(201, Some(scim::group_accepted(&body, &a.base))),
280282 ("PUT", ["Groups", id]) => answer(200, Some(scim::group_echo(id, &body, &a.base))),
281283 ("PATCH" | "DELETE", ["Groups", _]) => answer(204, None),
282− ("GET", ["Groups", _]) => scim_error(scim::ScimError::new(404, None, "g1t keeps users, not groups.")),
284+ ("GET", ["Groups", id]) => match scim::group_named(id) {
285+ Some(name) => answer(200, Some(scim::group_echo(id, &json!({ "displayName": name }), &a.base))),
286+ None => scim_error(scim::ScimError::new(404, None, "g1t keeps users, not groups.")),
287+ },
283288 _ => scim_error(scim::ScimError::new(404, None, "No such SCIM endpoint.")),
284289 })
285290 }
441446 .db
442447 .prepare(
443448 "UPDATE scim_users SET user_name = ?, user_name_key = ?, external_id = ?, resource = ?, active = ?, user_id = ?, updated_at = ?
444− WHERE id = ? AND workspace_id = ? AND updated_at = ? AND resource = ? RETURNING id",
449+ WHERE id = ? AND workspace_id = ? AND updated_at = ? AND resource = ? AND user_id IS ? RETURNING id",
445450 )
446451 .bind(&[
447452 user.user_name.as_str().into(),
455460 at.workspace_id.into(),
456461 row.updated_at.as_str().into(),
457462 row.resource.as_str().into(),
463+ row.user_id.as_deref().map_or(JsValue::NULL, Into::into),
458464 ])?
459465 .first::<Value>(None)
460466 .await?;
565571 "UPDATE invites SET revoked_at = {SQL_NOW}, sealed_code = NULL
566572 WHERE workspace_id = ?1 AND revoked_at IS NULL AND accepted_at IS NULL
567573 AND declined_at IS NULL AND redeemed_at IS NULL
568− AND (invitee_id = ?2 OR lower(email) IN (SELECT email FROM user_emails WHERE user_id = ?2))"
574+ AND (invitee_id = ?2 OR lower(email) IN (SELECT email FROM user_emails WHERE user_id = ?2 AND verified_at IS NOT NULL))"
569575 ))
570576 .bind(&[at.workspace_id.into(), user_id.into()])?,
571577 ])
+34−7
266266
267267 /// A group a provider pushed, accepted and let go: g1t keeps users, not
268268 /// groups, but a provider that pushes them (Entra does by default) must
269−/// not fail, or it stops sending deactivations too. Its id is made from its
270−/// name, so pushing it again gives the same one.
269+/// not fail, or it stops sending deactivations too. Its id is its name,
270+/// hex-encoded (hashed when long): pushing it again gives the same one, and
271+/// reading it back gives its name.
271272 pub fn group_accepted(body: &Value, base: &str) -> Value {
272− use sha2::{Digest, Sha256};
273273 let name = text(&body["displayName"]).unwrap_or_default();
274− let id = format!("grp_{}", hex::encode(&Sha256::digest(name.as_bytes())[..12]));
274+ let id = if name.len() <= 100 {
275+ format!("grp_{}", hex::encode(name.as_bytes()))
276+ } else {
277+ use sha2::{Digest, Sha256};
278+ format!("grh_{}", hex::encode(&Sha256::digest(name.as_bytes())[..12]))
279+ };
275280 group_echo(&id, body, base)
276281 }
277282
283+/// The name in a group's id, when it holds one.
284+pub fn group_named(id: &str) -> Option<String> {
285+ if let Some(hexed) = id.strip_prefix("grp_") {
286+ return String::from_utf8(hex::decode(hexed).ok()?).ok();
287+ }
288+ id.starts_with("grh_").then(String::new)
289+}
290+
278291 /// A group as sent, under `id`, with no members kept.
279292 pub fn group_echo(id: &str, body: &Value, base: &str) -> Value {
280− json!({
293+ let mut group = json!({
281294 "schemas": [GROUP_SCHEMA],
282295 "id": id,
283296 "displayName": text(&body["displayName"]).unwrap_or_default(),
284− "externalId": body["externalId"],
285297 "members": [],
286298 "meta": { "resourceType": "Group", "location": format!("{base}/Groups/{id}") },
287− })
299+ });
300+ if let Some(external) = text(&body["externalId"]) {
301+ group["externalId"] = json!(external);
302+ }
303+ group
288304 }
289305
290306 /// What g1t can do, for `GET /ServiceProviderConfig`.
431447 }
432448
433449 #[test]
450+ fn a_pushed_group_reads_back() {
451+ let group = group_accepted(&json!({ "displayName": "Engineering", "externalId": null }), "https://api.test/scim/v2/wsp_1");
452+ let id = group["id"].as_str().unwrap();
453+ assert_eq!(group_named(id).as_deref(), Some("Engineering"));
454+ assert!(group.get("externalId").is_none());
455+ assert_eq!(group_accepted(&json!({ "displayName": "Engineering" }), "x")["id"], group["id"]);
456+ assert_eq!(group_named("grp_zz"), None);
457+ assert_eq!(group_named("scu_1"), None);
458+ }
459+
460+ #[test]
434461 fn errors_are_scim_errors() {
435462 let body = ScimError::new(409, Some("uniqueness"), "taken").body();
436463 assert_eq!(body["status"], "409");

Comments

Sign in to comment.