From 4db8c373afec0ec8b111a3a7870c77bb249f4852 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 2 Jun 2026 01:23:52 -0400 Subject: [PATCH] =?UTF-8?q?fix(auth):=20ScadaBridge=201.2=20review=20fixes?= =?UTF-8?q?=20=E2=80=94=20secret-test=20repoint,=20checklist,=20Scope=20gu?= =?UTF-8?q?ard,=200.1.1=20pin?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Directory.Packages.props | 8 ++++---- docs/deployment/production-checklist.md | 7 ++++--- docs/plans/2026-05-24-second-environment.md | 16 ++++++++------- .../Auth/AuthEndpoints.cs | 20 +++++++++++++++++-- .../ConfigSecretsTests.cs | 12 ++++++++--- 5 files changed, 44 insertions(+), 19 deletions(-) diff --git a/Directory.Packages.props b/Directory.Packages.props index 9127b928..a81b4c54 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -80,10 +80,10 @@ - - - - + + + + diff --git a/docs/deployment/production-checklist.md b/docs/deployment/production-checklist.md index 17b6ca88..b5ef6c62 100644 --- a/docs/deployment/production-checklist.md +++ b/docs/deployment/production-checklist.md @@ -18,9 +18,10 @@ - [ ] EF Core migrations have been applied (SQL script reviewed and executed) - [ ] `ScadaBridge:Security:JwtSigningKey` is at least 32 characters, randomly generated - [ ] **Both central nodes use the same JwtSigningKey** (required for JWT failover) -- [ ] `ScadaBridge:Security:LdapServer` points to the production LDAP/AD server -- [ ] `ScadaBridge:Security:LdapUseTls` is `true` (LDAPS required in production) -- [ ] `ScadaBridge:Security:AllowInsecureLdap` is `false` +- [ ] `ScadaBridge:Security:Ldap:Server` points to the production LDAP/AD server +- [ ] `ScadaBridge:Security:Ldap:Transport` is `Ldaps` (LDAPS required in production) +- [ ] `ScadaBridge:Security:Ldap:AllowInsecure` is `false` +- [ ] LDAP service-account password supplied via env var `ScadaBridge__Security__Ldap__ServiceAccountPassword` (renamed from `ScadaBridge__Security__LdapServiceAccountPassword` in the Task 1.4 nested-config cutover) - [ ] LDAP search base DN is correct for the organization - [ ] LDAP group-to-role mappings are configured - [ ] Load balancer is configured in front of central UI (sticky sessions not required) diff --git a/docs/plans/2026-05-24-second-environment.md b/docs/plans/2026-05-24-second-environment.md index e863019b..67fdcc0a 100644 --- a/docs/plans/2026-05-24-second-environment.md +++ b/docs/plans/2026-05-24-second-environment.md @@ -246,13 +246,15 @@ These are clones of `docker/central-node-a/appsettings.Central.json` and `docker "MachineDataDb": "Server=scadabridge-mssql,1433;Database=ScadaBridgeMachineData2;User Id=scadabridge_app;Password=ScadaBridge_Dev1#;TrustServerCertificate=true" }, "Security": { - "LdapServer": "scadabridge-ldap", - "LdapPort": 3893, - "LdapUseTls": false, - "AllowInsecureLdap": true, - "LdapSearchBase": "dc=scadabridge,dc=local", - "LdapServiceAccountDn": "cn=admin,dc=scadabridge,dc=local", - "LdapServiceAccountPassword": "password", + "Ldap": { + "Server": "scadabridge-ldap", + "Port": 3893, + "Transport": "None", + "AllowInsecure": true, + "SearchBase": "dc=scadabridge,dc=local", + "ServiceAccountDn": "cn=admin,dc=scadabridge,dc=local", + "ServiceAccountPassword": "password" + }, "JwtSigningKey": "scadabridge-env2-dev-jwt-signing-key-must-be-at-least-32-characters-long", "JwtExpiryMinutes": 15, "IdleTimeoutMinutes": 30, diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Auth/AuthEndpoints.cs b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Auth/AuthEndpoints.cs index 60173be2..33e3174c 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Auth/AuthEndpoints.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Auth/AuthEndpoints.cs @@ -51,7 +51,17 @@ public static class AuthEndpoints // system-wide flag — is carried in the mapping's opaque Scope so the // site-scope→SiteId claims below are built exactly as before. var roleMapping = await roleMapper.MapAsync(authResult.Groups, context.RequestAborted); - var scope = (RoleMappingResult)roleMapping.Scope!; + + // The ScadaBridge mapper carries the full RoleMappingResult in the seam's + // opaque Scope (see ScadaBridgeGroupRoleMapper). Guard the unwrap (review I4): + // a future/alternate IGroupRoleMapper could leave Scope null or set a + // different type. Rather than throw InvalidCastException mid-login, fall back to + // the most restrictive interpretation — not a system-wide deployment and no + // permitted sites — so no SiteId claims are stamped (deny-by-omission). The real + // ScadaBridge mapper always supplies a RoleMappingResult, so behaviour is unchanged. + var scope = roleMapping.Scope is RoleMappingResult mapped + ? mapped + : new RoleMappingResult(roleMapping.Roles, [], IsSystemWideDeployment: false); // Build claims from LDAP auth + role mapping. // CentralUI-005: no fixed "expires_at" absolute-cap claim is stamped @@ -116,7 +126,13 @@ public static class AuthEndpoints } var roleMapping = await roleMapper.MapAsync(authResult.Groups, context.RequestAborted); - var scope = (RoleMappingResult)roleMapping.Scope!; + + // Guard the opaque-Scope unwrap (review I4); see the matching note on + // /auth/login. Fall back to no site-scope rather than throwing if a future + // mapper leaves Scope null or sets a different type. + var scope = roleMapping.Scope is RoleMappingResult mapped + ? mapped + : new RoleMappingResult(roleMapping.Roles, [], IsSystemWideDeployment: false); var displayName = string.IsNullOrEmpty(authResult.DisplayName) ? username : authResult.DisplayName; var resolvedUsername = string.IsNullOrEmpty(authResult.Username) ? username : authResult.Username; diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ConfigSecretsTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ConfigSecretsTests.cs index d01249fa..0dc510ad 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ConfigSecretsTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ConfigSecretsTests.cs @@ -61,14 +61,20 @@ public class ConfigSecretsTests [Fact] public void CentralConfig_LdapServiceAccountPassword_IsNotCommitted() { + // Task 1.4 cutover: the LDAP service-account password moved out of the flat + // Security:LdapServiceAccountPassword key into the nested Security:Ldap + // sub-section (Security:Ldap:ServiceAccountPassword), bound to the shared + // ZB.MOM.WW.Auth LdapOptions. Walk into Security:Ldap and guard the nested + // key — checking the deleted flat key would pass vacuously. var security = ScadaBridgeSection().GetProperty("Security"); - if (security.TryGetProperty("LdapServiceAccountPassword", out var pw)) + var ldap = security.GetProperty("Ldap"); + if (ldap.TryGetProperty("ServiceAccountPassword", out var pw)) { var value = pw.GetString() ?? string.Empty; Assert.True( value.Length == 0 || value.Contains('{') || value.Contains('$'), - $"appsettings.Central.json carries a plaintext LdapServiceAccountPassword '{value}'. " + - "Move it to an environment variable."); + $"appsettings.Central.json carries a plaintext Security:Ldap:ServiceAccountPassword '{value}'. " + + "Move it to an environment variable (ScadaBridge__Security__Ldap__ServiceAccountPassword)."); } }