From 57f43f822aedebfa717e3b6a891ac895dbd5bedb Mon Sep 17 00:00:00 2001 From: Bryan Frimin Date: Thu, 30 Oct 2025 16:30:47 +0100 Subject: [PATCH] Style Signed-off-by: Bryan Frimin --- .../settings/SAMLSettingsTab.tsx | 107 ++++++++---------- pkg/auth/access.go | 20 +--- pkg/auth/saml_cleanup.go | 39 ++++--- pkg/coredata/file.go | 49 -------- pkg/server/api/console/v1/v1_resolver.go | 2 +- 5 files changed, 74 insertions(+), 143 deletions(-) diff --git a/apps/console/src/pages/organizations/settings/SAMLSettingsTab.tsx b/apps/console/src/pages/organizations/settings/SAMLSettingsTab.tsx index 0682daa6d..fdffe2c5c 100644 --- a/apps/console/src/pages/organizations/settings/SAMLSettingsTab.tsx +++ b/apps/console/src/pages/organizations/settings/SAMLSettingsTab.tsx @@ -4,13 +4,13 @@ import { useFragment, graphql } from "react-relay"; import { Controller } from "react-hook-form"; import { z } from "zod"; import { + Breadcrumb, Button, Card, Checkbox, Dialog, DialogContent, DialogFooter, - DialogTitle, Field, Label, Option, @@ -491,16 +491,23 @@ export default function SAMLSettingsTab() { )} - - - - {currentStep === "initiate" && __("Step 1: Register Domain")} - {currentStep === "verify" && __("Step 2: Verify Domain Ownership")} - {currentStep === "configure" && (editingConfig?.domainVerified ? __("Configure SAML") : __("Step 3: Configure SAML"))} - - - {currentStep === "initiate" && ( -
+ + } + > + {currentStep === "initiate" && ( + +

{__("Register Your Domain")} @@ -520,11 +527,18 @@ export default function SAMLSettingsTab() {

- - )} + + + + + + )} - {currentStep === "verify" && ( -
+ {currentStep === "verify" && ( + <> +

{__("Verify Domain Ownership")} @@ -566,12 +580,19 @@ export default function SAMLSettingsTab() {

- - )} +
+ + + + + )} - {currentStep === "configure" && ( -
-
+ {currentStep === "configure" && ( + + +

{__("Basic Configuration")}

@@ -710,48 +731,14 @@ export default function SAMLSettingsTab() { )} />
- - - )} - - - - - {currentStep === "initiate" && ( - - )} - - {currentStep === "verify" && ( - - )} - - {currentStep === "configure" && ( - - )} - - + + + )}
); diff --git a/pkg/auth/access.go b/pkg/auth/access.go index 586769bac..9cbd883fd 100644 --- a/pkg/auth/access.go +++ b/pkg/auth/access.go @@ -13,28 +13,23 @@ type AuthMethod int const ( AuthMethodPassword AuthMethod = iota AuthMethodSAML - AuthMethodAny // Used when either password or SAML would work + AuthMethodAny ) -// OrgAuthRequirement encapsulates the authentication requirements for accessing an organization type OrgAuthRequirement struct { OrganizationID gid.GID EmailDomain string - SAMLConfig *coredata.SAMLConfiguration // nil if no SAML config applies to this org+domain + SAMLConfig *coredata.SAMLConfiguration } -// AccessResult represents the result of an organization access check type AccessResult struct { OrganizationID gid.GID Allowed bool - MissingAuth AuthMethod // Which auth method is missing (if not allowed) - SAMLConfig *coredata.SAMLConfiguration // The SAML config involved (if any) + MissingAuth AuthMethod + SAMLConfig *coredata.SAMLConfiguration } -// Check performs the access control decision based on session state -// This is pure business logic with no side effects - easily testable func (r OrgAuthRequirement) Check(session coredata.SessionData) AccessResult { - // No SAML config or disabled → requires password authentication if r.SAMLConfig == nil || !r.SAMLConfig.Enabled || !r.SAMLConfig.DomainVerified { return AccessResult{ OrganizationID: r.OrganizationID, @@ -44,11 +39,9 @@ func (r OrgAuthRequirement) Check(session coredata.SessionData) AccessResult { } } - // Check if user has SAML authentication for this specific organization orgKey := r.OrganizationID.String() _, hasSAML := session.SAMLAuthenticatedOrgs[orgKey] - // SAML enforcement: REQUIRED → must have SAML auth for this org if r.SAMLConfig.EnforcementPolicy == coredata.SAMLEnforcementPolicyRequired { return AccessResult{ OrganizationID: r.OrganizationID, @@ -58,11 +51,10 @@ func (r OrgAuthRequirement) Check(session coredata.SessionData) AccessResult { } } - // SAML enforcement: OPTIONAL → needs either password (global) OR SAML (for this org) hasAnyAuth := session.PasswordAuthenticated || hasSAML missingAuth := AuthMethodAny if hasAnyAuth { - missingAuth = AuthMethodPassword // Not actually missing, but need a value + missingAuth = AuthMethodPassword } return AccessResult{ @@ -73,8 +65,6 @@ func (r OrgAuthRequirement) Check(session coredata.SessionData) AccessResult { } } -// ToError converts an AccessResult to an error if access is denied -// This handles the presentation layer concern of generating appropriate errors and redirect URLs func (r AccessResult) ToError(baseURL string) error { if r.Allowed { return nil diff --git a/pkg/auth/saml_cleanup.go b/pkg/auth/saml_cleanup.go index 40bd742b0..d60520dd6 100644 --- a/pkg/auth/saml_cleanup.go +++ b/pkg/auth/saml_cleanup.go @@ -76,27 +76,30 @@ func (c *Cleaner) Run(ctx context.Context) error { func (c *Cleaner) cleanup(ctx context.Context) error { var assertionsDeleted, requestsDeleted, relayStatesDeleted int64 - err := c.pg.WithConn(ctx, func(conn pg.Conn) error { - count, err := CleanupExpiredAssertions(ctx, conn) - if err != nil { - return err - } - assertionsDeleted = count + err := c.pg.WithConn( + ctx, + func(conn pg.Conn) error { + count, err := CleanupExpiredAssertions(ctx, conn) + if err != nil { + return err + } + assertionsDeleted = count - count, err = CleanupExpiredRequests(ctx, conn) - if err != nil { - return err - } - requestsDeleted = count + count, err = CleanupExpiredRequests(ctx, conn) + if err != nil { + return err + } + requestsDeleted = count - count, err = CleanupExpiredRelayStates(ctx, conn) - if err != nil { - return err - } - relayStatesDeleted = count + count, err = CleanupExpiredRelayStates(ctx, conn) + if err != nil { + return err + } + relayStatesDeleted = count - return nil - }) + return nil + }, + ) if err != nil { return err diff --git a/pkg/coredata/file.go b/pkg/coredata/file.go index ac926fdf5..b8a519b25 100644 --- a/pkg/coredata/file.go +++ b/pkg/coredata/file.go @@ -153,52 +153,3 @@ WHERE %s return err } - -// LoadFilesByIDs loads multiple files by their IDs in a single query -// Returns a map of file ID to File for efficient lookup -func LoadFilesByIDs( - ctx context.Context, - conn pg.Conn, - fileIDs []gid.GID, -) (map[gid.GID]*File, error) { - if len(fileIDs) == 0 { - return make(map[gid.GID]*File), nil - } - - q := ` -SELECT - id, - bucket_name, - mime_type, - file_name, - file_key, - file_size, - created_at, - updated_at, - deleted_at -FROM - files -WHERE - id = ANY(@file_ids) - AND deleted_at IS NULL -` - - args := pgx.StrictNamedArgs{"file_ids": fileIDs} - - rows, err := conn.Query(ctx, q, args) - if err != nil { - return nil, fmt.Errorf("cannot query files: %w", err) - } - - files, err := pgx.CollectRows(rows, pgx.RowToStructByName[File]) - if err != nil { - return nil, fmt.Errorf("cannot collect files: %w", err) - } - - result := make(map[gid.GID]*File, len(files)) - for i := range files { - result[files[i].ID] = &files[i] - } - - return result, nil -} diff --git a/pkg/server/api/console/v1/v1_resolver.go b/pkg/server/api/console/v1/v1_resolver.go index 9bbf8c849..eb74ba23e 100644 --- a/pkg/server/api/console/v1/v1_resolver.go +++ b/pkg/server/api/console/v1/v1_resolver.go @@ -1080,7 +1080,7 @@ func (r *membershipResolver) AuthMethod(ctx context.Context, obj *types.Membersh authMethod, err := auth.GetUserAuthMethod(ctx, obj.UserID, obj.OrganizationID, session) if err != nil { - return "", fmt.Errorf("cannot get user auth method: %w", err) + panic(fmt.Errorf("cannot get user auth method: %w", err)) } return authMethod, nil }