diff --git a/changelog.d/3-bug-fixes/WBP-8790 b/changelog.d/3-bug-fixes/WBP-8790 new file mode 100644 index 00000000000..76b0c27b8a6 --- /dev/null +++ b/changelog.d/3-bug-fixes/WBP-8790 @@ -0,0 +1 @@ +Fix handling of defaults of `mlsE2EID` feature config diff --git a/hack/helm_vars/wire-server/values.yaml.gotmpl b/hack/helm_vars/wire-server/values.yaml.gotmpl index 2ed14739f79..0e1b9604a64 100644 --- a/hack/helm_vars/wire-server/values.yaml.gotmpl +++ b/hack/helm_vars/wire-server/values.yaml.gotmpl @@ -278,6 +278,14 @@ galley: usersThreshold: 100 clientsThreshold: 50 lockStatus: locked + mlsE2EId: + defaults: + status: disabled + config: + verificationExpiration: 86400 + acmeDiscoveryUrl: null + crlProxy: https://crlproxy.example.com + lockStatus: unlocked limitedEventFanout: defaults: status: disabled diff --git a/integration/test/Test/FeatureFlags.hs b/integration/test/Test/FeatureFlags.hs index 4b285e6bef2..6d088b1ce84 100644 --- a/integration/test/Test/FeatureFlags.hs +++ b/integration/test/Test/FeatureFlags.hs @@ -228,7 +228,11 @@ testMlsE2EConfigCrlProxyNotRequiredInV5 = do resp.status `shouldMatchInt` 200 -- Assert that the feature config got updated correctly - expectedResponse <- configWithoutCrlProxy & setField "lockStatus" "unlocked" & setField "ttl" "unlimited" + expectedResponse <- + configWithoutCrlProxy + & setField "lockStatus" "unlocked" + & setField "ttl" "unlimited" + & setField "config.crlProxy" "https://crlproxy.example.com" checkFeature "mlsE2EId" owner tid expectedResponse testSSODisabledByDefault :: (HasCallStack) => App () @@ -462,7 +466,8 @@ testAllFeatures = do "config" .= object [ "verificationExpiration" .= A.Number 86400, - "useProxyOnMobile" .= False + "useProxyOnMobile" .= False, + "crlProxy" .= "https://crlproxy.example.com" ] ], "mlsMigration" @@ -747,7 +752,8 @@ mlsE2EIdConfig = do "config" .= object [ "verificationExpiration" .= A.Number 86400, - "useProxyOnMobile" .= False + "useProxyOnMobile" .= False, + "crlProxy" .= "https://crlproxy.example.com" ] ] mlsE2EIdConfig1 :: Value @@ -1028,7 +1034,8 @@ testPatchE2EId = do "config" .= object [ "verificationExpiration" .= A.Number 86400, - "useProxyOnMobile" .= False + "useProxyOnMobile" .= False, + "crlProxy" .= "https://crlproxy.example.com" ] ] _testPatch "mlsE2EId" True defCfg (object ["lockStatus" .= "locked"]) diff --git a/services/galley/galley.integration.yaml b/services/galley/galley.integration.yaml index 6439e5ba7de..c07a9c78056 100644 --- a/services/galley/galley.integration.yaml +++ b/services/galley/galley.integration.yaml @@ -93,6 +93,7 @@ settings: config: verificationExpiration: 86400 acmeDiscoveryUrl: null + crlProxy: https://crlproxy.example.com lockStatus: unlocked mlsMigration: defaults: diff --git a/services/galley/src/Galley/Cassandra/MakeFeature.hs b/services/galley/src/Galley/Cassandra/MakeFeature.hs index 2db777f2521..c090a97bdb0 100644 --- a/services/galley/src/Galley/Cassandra/MakeFeature.hs +++ b/services/galley/src/Galley/Cassandra/MakeFeature.hs @@ -23,6 +23,20 @@ import Wire.API.Conversation.Protocol (ProtocolTag) import Wire.API.MLS.CipherSuite import Wire.API.Team.Feature +-- [Note: default values for configuration fields] +-- +-- When reading values for configuration types with multiple fields, we fall +-- back to default values for each field independently, instead of treating the +-- whole configuration as a single value that can be set or not. +-- +-- In most cases, either strategy would produce the same result, because there +-- is no way to set only *some* fields using the public API. However, that can +-- happen when a feature flag changes over time and gains new fields, as it has +-- been the case for mlsE2EId. +-- +-- Therefore, we use the first strategy consistently for all feature flags, +-- even when it does not matter. + -- | This is necessary in order to convert an @NP f xs@ type to something that -- CQL can understand. -- @@ -90,7 +104,13 @@ instance MakeFeature AppLockConfig where rowToFeature (status :* enforce :* timeout :* Nil) = foldMap dbFeatureStatus status - <> foldMap dbFeatureConfig (AppLockConfig <$> enforce <*> timeout) + -- [Note: default values for configuration fields] + <> dbFeatureModConfig + ( \defCfg -> + AppLockConfig + (fromMaybe defCfg.applockEnforceAppLock enforce) + (fromMaybe defCfg.applockInactivityTimeoutSecs timeout) + ) featureToRow feat = Just feat.status @@ -226,13 +246,34 @@ instance MakeFeature MLSConfig where ) = foldMap dbFeatureLockStatus lockStatus <> foldMap dbFeatureStatus status - <> foldMap - dbFeatureConfig - ( MLSConfig (foldMap C.fromSet toggleUsers) - <$> defProto - <*> pure (foldMap C.fromSet ciphersuites) - <*> defCiphersuite - <*> pure (foldMap C.fromSet supportedProtos) + <> dbFeatureModConfig + ( \defCfg -> + -- [Note: default values for configuration fields] + -- + -- This case is a bit special, because Cassandra sets do not + -- distinguish between 'null' and 'empty'. To differentiate + -- between these cases, we use the `mls_default_protocol` field: + -- if set, we interpret null sets as empty, otherwise we use the + -- default. + let configIsSet = isJust defProto + in MLSConfig + ( maybe + (if configIsSet then [] else defCfg.mlsProtocolToggleUsers) + C.fromSet + toggleUsers + ) + (fromMaybe defCfg.mlsDefaultProtocol defProto) + ( maybe + (if configIsSet then [] else defCfg.mlsAllowedCipherSuites) + C.fromSet + ciphersuites + ) + (fromMaybe defCfg.mlsDefaultCipherSuite defCiphersuite) + ( maybe + (if configIsSet then [] else defCfg.mlsSupportedProtocols) + C.fromSet + supportedProtos + ) ) featureToRow feat = @@ -280,8 +321,8 @@ instance MakeFeature MlsE2EIdConfig where defCfg { verificationExpiration = maybe defCfg.verificationExpiration fromIntegral gracePeriod, - acmeDiscoveryUrl = acmeDiscoveryUrl, - crlProxy = crlProxy, + acmeDiscoveryUrl = acmeDiscoveryUrl <|> defCfg.acmeDiscoveryUrl, + crlProxy = crlProxy <|> defCfg.crlProxy, useProxyOnMobile = fromMaybe defCfg.useProxyOnMobile useProxyOnMobile } ) @@ -310,6 +351,7 @@ instance MakeFeature MlsMigrationConfig where rowToFeature (lockStatus :* status :* startTime :* finalizeAfter :* Nil) = foldMap dbFeatureLockStatus lockStatus <> foldMap dbFeatureStatus status + -- FUTUREWORK: allow using the default <> dbFeatureConfig (MlsMigrationConfig startTime finalizeAfter) featureToRow feat = @@ -331,6 +373,7 @@ instance MakeFeature EnforceFileDownloadLocationConfig where rowToFeature (lockStatus :* status :* location :* Nil) = foldMap dbFeatureLockStatus lockStatus <> foldMap dbFeatureStatus status + -- FUTUREWORK: allow using the default <> dbFeatureConfig (EnforceFileDownloadLocationConfig location) featureToRow feat = Just feat.lockStatus