Skip to content

Cleanup old team feature storage #4470

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 11, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/5-internal/cleanup-old-features
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Remove legacy team feature storage support
8 changes: 2 additions & 6 deletions integration/test/API/GalleyInternal.hs
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,10 @@ setTeamFeatureStatus domain team featureName status = do

setTeamFeatureLockStatus :: (HasCallStack, MakesValue domain, MakesValue team) => domain -> team -> String -> String -> App ()
setTeamFeatureLockStatus domain team featureName status = do
bindResponse (setTeamFeatureLockStatusResponse domain team featureName status) $ \res ->
res.status `shouldMatchInt` 200

setTeamFeatureLockStatusResponse :: (HasCallStack, MakesValue domain, MakesValue team) => domain -> team -> String -> String -> App Response
setTeamFeatureLockStatusResponse domain team featureName status = do
tid <- asString team
req <- baseRequest domain Galley Unversioned $ joinHttpPath ["i", "teams", tid, "features", featureName, status]
submit "PUT" $ req
bindResponse (submit "PUT" $ req) $ \res ->
res.status `shouldMatchInt` 200

getFederationStatus ::
( HasCallStack,
Expand Down
21 changes: 9 additions & 12 deletions integration/test/Test/FeatureFlags.hs
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,11 @@ import SetupHelpers
import Test.FeatureFlags.Util
import Testlib.Prelude

testLimitedEventFanout :: (HasCallStack) => FeatureTable -> App ()
testLimitedEventFanout ft = do
testLimitedEventFanout :: (HasCallStack) => App ()
testLimitedEventFanout = do
let featureName = "limitedEventFanout"
(_alice, team, _) <- createTeam OwnDomain 1
updateMigrationState OwnDomain team ft
-- getTeamFeatureStatus OwnDomain team "limitedEventFanout" "enabled"
bindResponse (Internal.getTeamFeature OwnDomain team featureName) $ \resp -> do
resp.status `shouldMatchInt` 200
resp.json %. "status" `shouldMatch` "disabled"
Expand All @@ -44,10 +44,9 @@ testLimitedEventFanout ft = do

-- | Call 'GET /teams/:tid/features' and 'GET /feature-configs', and check if all
-- features are there.
testAllFeatures :: (HasCallStack) => FeatureTable -> App ()
testAllFeatures ft = do
testAllFeatures :: (HasCallStack) => App ()
testAllFeatures = do
(_, tid, m : _) <- createTeam OwnDomain 2
updateMigrationState OwnDomain tid ft
bindResponse (Public.getTeamFeatures m tid) $ \resp -> do
resp.status `shouldMatchInt` 200
defAllFeatures `shouldMatch` resp.json
Expand All @@ -71,10 +70,9 @@ testAllFeatures ft = do
resp.status `shouldMatchInt` 200
defAllFeatures `shouldMatch` resp.json

testFeatureConfigConsistency :: (HasCallStack) => FeatureTable -> App ()
testFeatureConfigConsistency ft = do
testFeatureConfigConsistency :: (HasCallStack) => App ()
testFeatureConfigConsistency = do
(_, tid, m : _) <- createTeam OwnDomain 2
updateMigrationState OwnDomain tid ft

allFeaturesRes <- Public.getFeatureConfigs m >>= parseObjectKeys

Expand All @@ -90,10 +88,9 @@ testFeatureConfigConsistency ft = do
(A.Object hm) -> pure (Set.fromList . map (show . A.toText) . KM.keys $ hm)
x -> assertFailure ("JSON was not an object, but " <> show x)

testNonMemberAccess :: (HasCallStack) => FeatureTable -> Feature -> App ()
testNonMemberAccess ft (Feature featureName) = do
testNonMemberAccess :: (HasCallStack) => Feature -> App ()
testNonMemberAccess (Feature featureName) = do
(_, tid, _) <- createTeam OwnDomain 0
updateMigrationState OwnDomain tid ft
nonMember <- randomUser OwnDomain def
Public.getTeamFeature nonMember tid featureName
>>= assertForbidden
19 changes: 7 additions & 12 deletions integration/test/Test/FeatureFlags/AppLock.hs
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@ import qualified Data.Aeson as A
import Test.FeatureFlags.Util
import Testlib.Prelude

testPatchAppLock :: (HasCallStack) => FeatureTable -> App ()
testPatchAppLock table = do
checkPatchWithTable table OwnDomain "appLock"
testPatchAppLock :: (HasCallStack) => App ()
testPatchAppLock = do
checkPatch OwnDomain "appLock"
$ object ["lockStatus" .= "locked"]
checkPatchWithTable table OwnDomain "appLock"
checkPatch OwnDomain "appLock"
$ object ["status" .= "disabled"]
checkPatchWithTable table OwnDomain "appLock"
checkPatch OwnDomain "appLock"
$ object ["lockStatus" .= "locked", "status" .= "disabled"]
checkPatchWithTable table OwnDomain "appLock"
checkPatch OwnDomain "appLock"
$ object
[ "lockStatus" .= "unlocked",
"config"
Expand All @@ -21,16 +21,11 @@ testPatchAppLock table = do
"inactivityTimeoutSecs" .= A.Number 120
]
]
checkPatchWithTable table OwnDomain "appLock"
checkPatch OwnDomain "appLock"
$ object
[ "config"
.= object
[ "enforceAppLock" .= True,
"inactivityTimeoutSecs" .= A.Number 240
]
]

testPatchAppLockReadOnly :: (HasCallStack) => App ()
testPatchAppLockReadOnly = do
checkPatchReadOnly OwnDomain "appLock"
$ object ["lockStatus" .= "locked"]
12 changes: 0 additions & 12 deletions integration/test/Test/FeatureFlags/ConferenceCalling.hs
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,3 @@ testConferenceCalling access = do
& addUpdate (confCalling def {sft = toJSON True})
& addUpdate (confCalling def {sft = toJSON False})
& addInvalidUpdate (confCalling def {sft = toJSON (0 :: Int)})

testPatchConferenceCallingReadOnly :: (HasCallStack) => App ()
testPatchConferenceCallingReadOnly = do
checkPatchReadOnly OwnDomain "conferenceCalling"
$ object ["lockStatus" .= "locked"]

testConferenceCallingReadOnlyDuringMigration :: (HasCallStack) => APIAccess -> App ()
testConferenceCallingReadOnlyDuringMigration access = do
runFeatureTestsReadOnly OwnDomain access
$ mkFeatureTests "conferenceCalling"
& addUpdate (confCalling def {sft = toJSON True})
& addUpdate (confCalling def {sft = toJSON False})
9 changes: 4 additions & 5 deletions integration/test/Test/FeatureFlags/DigitalSignatures.hs
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,12 @@ import SetupHelpers
import Test.FeatureFlags.Util
import Testlib.Prelude

testPatchDigitalSignatures :: (HasCallStack) => FeatureTable -> App ()
testPatchDigitalSignatures table = checkPatchWithTable table OwnDomain "digitalSignatures" enabled
testPatchDigitalSignatures :: (HasCallStack) => App ()
testPatchDigitalSignatures = checkPatch OwnDomain "digitalSignatures" enabled

testDigitalSignaturesInternal :: (HasCallStack) => FeatureTable -> App ()
testDigitalSignaturesInternal table = do
testDigitalSignaturesInternal :: (HasCallStack) => App ()
testDigitalSignaturesInternal = do
(alice, tid, _) <- createTeam OwnDomain 0
updateMigrationState OwnDomain tid table
withWebSocket alice $ \ws -> do
setFlag InternalAPI ws tid "digitalSignatures" disabled
setFlag InternalAPI ws tid "digitalSignatures" enabled
9 changes: 4 additions & 5 deletions integration/test/Test/FeatureFlags/DomainRegistration.hs
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,12 @@ import SetupHelpers
import Test.FeatureFlags.Util
import Testlib.Prelude

testPatchDomainRegistration :: (HasCallStack) => FeatureTable -> App ()
testPatchDomainRegistration table = checkPatchWithTable table OwnDomain "domainRegistration" enabled
testPatchDomainRegistration :: (HasCallStack) => App ()
testPatchDomainRegistration = checkPatch OwnDomain "domainRegistration" enabled

testDomainRegistrationInternal :: (HasCallStack) => FeatureTable -> App ()
testDomainRegistrationInternal table = do
testDomainRegistrationInternal :: (HasCallStack) => App ()
testDomainRegistrationInternal = do
(alice, tid, _) <- createTeam OwnDomain 0
updateMigrationState OwnDomain tid table
Internal.setTeamFeatureLockStatus alice tid "domainRegistration" "unlocked"
withWebSocket alice $ \ws -> do
setFlag InternalAPI ws tid "domainRegistration" enabled
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,21 @@ import SetupHelpers
import Test.FeatureFlags.Util
import Testlib.Prelude

testPatchEnforceFileDownloadLocation :: (HasCallStack) => FeatureTable -> App ()
testPatchEnforceFileDownloadLocation table = do
checkPatchWithTable table OwnDomain "enforceFileDownloadLocation"
testPatchEnforceFileDownloadLocation :: (HasCallStack) => App ()
testPatchEnforceFileDownloadLocation = do
checkPatch OwnDomain "enforceFileDownloadLocation"
$ object ["lockStatus" .= "unlocked"]
checkPatchWithTable table OwnDomain "enforceFileDownloadLocation"
checkPatch OwnDomain "enforceFileDownloadLocation"
$ object ["status" .= "enabled"]
checkPatchWithTable table OwnDomain "enforceFileDownloadLocation"
checkPatch OwnDomain "enforceFileDownloadLocation"
$ object ["lockStatus" .= "unlocked", "status" .= "enabled"]
checkPatchWithTable table OwnDomain "enforceFileDownloadLocation"
checkPatch OwnDomain "enforceFileDownloadLocation"
$ object ["lockStatus" .= "locked", "config" .= object []]
checkPatchWithTable table OwnDomain "enforceFileDownloadLocation"
checkPatch OwnDomain "enforceFileDownloadLocation"
$ object ["config" .= object ["enforcedDownloadLocation" .= "/tmp"]]

do
(user, tid, _) <- createTeam OwnDomain 0
updateMigrationState OwnDomain tid table
bindResponse
( Internal.patchTeamFeature
user
Expand All @@ -32,8 +31,8 @@ testPatchEnforceFileDownloadLocation table = do
resp.status `shouldMatchInt` 400
resp.json %. "label" `shouldMatch` "empty-download-location"

testEnforceDownloadLocation :: (HasCallStack) => FeatureTable -> APIAccess -> App ()
testEnforceDownloadLocation table access = do
testEnforceDownloadLocation :: (HasCallStack) => APIAccess -> App ()
testEnforceDownloadLocation access = do
mkFeatureTests
"enforceFileDownloadLocation"
& addUpdate
Expand All @@ -53,5 +52,4 @@ testEnforceDownloadLocation table access = do
]
]
)
& setTable table
& runFeatureTests OwnDomain access
9 changes: 4 additions & 5 deletions integration/test/Test/FeatureFlags/FileSharing.hs
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,12 @@ module Test.FeatureFlags.FileSharing where
import Test.FeatureFlags.Util
import Testlib.Prelude

testPatchFileSharing :: (HasCallStack) => FeatureTable -> App ()
testPatchFileSharing table = checkPatchWithTable table OwnDomain "fileSharing" disabled
testPatchFileSharing :: (HasCallStack) => App ()
testPatchFileSharing = checkPatch OwnDomain "fileSharing" disabled

testFileSharing :: (HasCallStack) => FeatureTable -> APIAccess -> App ()
testFileSharing table access =
testFileSharing :: (HasCallStack) => APIAccess -> App ()
testFileSharing access =
mkFeatureTests "fileSharing"
& addUpdate disabled
& addUpdate enabled
& setTable table
& runFeatureTests OwnDomain access
19 changes: 4 additions & 15 deletions integration/test/Test/FeatureFlags/GuestLinks.hs
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,12 @@ module Test.FeatureFlags.GuestLinks where
import Test.FeatureFlags.Util
import Testlib.Prelude

testConversationGuestLinks :: (HasCallStack) => FeatureTable -> APIAccess -> App ()
testConversationGuestLinks table access =
testConversationGuestLinks :: (HasCallStack) => APIAccess -> App ()
testConversationGuestLinks access =
mkFeatureTests "conversationGuestLinks"
& addUpdate disabled
& addUpdate enabled
& setTable table
& runFeatureTests OwnDomain access

testPatchGuestLinks :: (HasCallStack) => FeatureTable -> App ()
testPatchGuestLinks table = checkPatchWithTable table OwnDomain "conversationGuestLinks" disabled

testConversationGuestLinksReadOnly :: (HasCallStack) => APIAccess -> App ()
testConversationGuestLinksReadOnly access =
runFeatureTestsReadOnly OwnDomain access
$ mkFeatureTests "conversationGuestLinks"
& addUpdate disabled
& addUpdate enabled

testPatchGuestLinksReadOnly :: (HasCallStack) => App ()
testPatchGuestLinksReadOnly = checkPatchReadOnly OwnDomain "conversationGuestLinks" disabled
testPatchGuestLinks :: (HasCallStack) => App ()
testPatchGuestLinks = checkPatch OwnDomain "conversationGuestLinks" disabled
6 changes: 2 additions & 4 deletions integration/test/Test/FeatureFlags/Initialisation.hs
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,11 @@ import Control.Monad.Codensity
import Control.Monad.Extra
import Control.Monad.Reader
import SetupHelpers
import Test.FeatureFlags.Util
import Testlib.Prelude
import Testlib.ResourcePool

testMLSInitialisation :: (HasCallStack) => FeatureTable -> App ()
testMLSInitialisation table = do
testMLSInitialisation :: (HasCallStack) => App ()
testMLSInitialisation = do
let override =
def
{ galleyCfg =
Expand Down Expand Up @@ -43,7 +42,6 @@ testMLSInitialisation table = do
(alice, tid, _) <- createTeam domain 0
feat <- getTeamFeature alice tid "mls" >>= getJSON 200
feat %. "config.defaultProtocol" `shouldMatch` "proteus"
updateMigrationState domain tid table
pure (alice, tid)

lift $ lowerCodensity do
Expand Down
21 changes: 8 additions & 13 deletions integration/test/Test/FeatureFlags/LegalHold.hs
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,15 @@ import Test.FeatureFlags.Util
import Testlib.Prelude
import Testlib.ResourcePool (acquireResources)

testLegalholdDisabledByDefault :: (HasCallStack) => FeatureTable -> App ()
testLegalholdDisabledByDefault table = do
testLegalholdDisabledByDefault :: (HasCallStack) => App ()
testLegalholdDisabledByDefault = do
let put uid tid st = Internal.setTeamFeatureConfig uid tid "legalhold" (object ["status" .= st]) >>= assertSuccess
let patch uid tid st = Internal.setTeamFeatureStatus uid tid "legalhold" st >>= assertSuccess
forM_ [put, patch] $ \setFeatureStatus -> do
withModifiedBackend
def {galleyCfg = setField "settings.featureFlags.legalhold" "disabled-by-default"}
$ \domain -> do
(owner, tid, m : _) <- createTeam domain 2
updateMigrationState domain tid table
nonMember <- randomUser domain def
assertForbidden =<< Public.getTeamFeature nonMember tid "legalhold"
-- Test default
Expand All @@ -30,8 +29,8 @@ testLegalholdDisabledByDefault table = do
checkFeature "legalhold" owner tid disabled

-- always disabled
testLegalholdDisabledPermanently :: (HasCallStack) => FeatureTable -> App ()
testLegalholdDisabledPermanently table = do
testLegalholdDisabledPermanently :: (HasCallStack) => App ()
testLegalholdDisabledPermanently = do
let cfgLhDisabledPermanently =
def
{ galleyCfg = setField "settings.featureFlags.legalhold" "disabled-permanently"
Expand All @@ -47,7 +46,6 @@ testLegalholdDisabledPermanently table = do
-- Happy case: DB has no config for the team
runCodensity (startDynamicBackend testBackend cfgLhDisabledPermanently) $ \_ -> do
(owner, tid, _) <- createTeam domain 1
updateMigrationState domain tid table
checkFeature "legalhold" owner tid disabled
assertStatus 403 =<< Internal.setTeamFeatureStatus domain tid "legalhold" "enabled"
assertStatus 403 =<< Internal.setTeamFeatureConfig domain tid "legalhold" (object ["status" .= "enabled"])
Expand All @@ -56,7 +54,6 @@ testLegalholdDisabledPermanently table = do
-- changed to disabled-permanently
(owner, tid) <- runCodensity (startDynamicBackend testBackend cfgLhDisabledByDefault) $ \_ -> do
(owner, tid, _) <- createTeam domain 1
updateMigrationState domain tid table
checkFeature "legalhold" owner tid disabled
assertSuccess =<< Internal.setTeamFeatureStatus domain tid "legalhold" "enabled"
checkFeature "legalhold" owner tid enabled
Expand All @@ -66,8 +63,8 @@ testLegalholdDisabledPermanently table = do
checkFeature "legalhold" owner tid disabled

-- enabled if team is allow listed, disabled in any other case
testLegalholdWhitelistTeamsAndImplicitConsent :: (HasCallStack) => FeatureTable -> App ()
testLegalholdWhitelistTeamsAndImplicitConsent table = do
testLegalholdWhitelistTeamsAndImplicitConsent :: (HasCallStack) => App ()
testLegalholdWhitelistTeamsAndImplicitConsent = do
let cfgLhWhitelistTeamsAndImplicitConsent =
def
{ galleyCfg = setField "settings.featureFlags.legalhold" "whitelist-teams-and-implicit-consent"
Expand All @@ -83,7 +80,6 @@ testLegalholdWhitelistTeamsAndImplicitConsent table = do
-- Happy case: DB has no config for the team
(owner, tid) <- runCodensity (startDynamicBackend testBackend cfgLhWhitelistTeamsAndImplicitConsent) $ \_ -> do
(owner, tid, _) <- createTeam domain 1
updateMigrationState domain tid table
checkFeature "legalhold" owner tid disabled
Internal.legalholdWhitelistTeam tid owner >>= assertSuccess
checkFeature "legalhold" owner tid enabled
Expand All @@ -105,8 +101,8 @@ testLegalholdWhitelistTeamsAndImplicitConsent table = do
runCodensity (startDynamicBackend testBackend cfgLhWhitelistTeamsAndImplicitConsent) $ \_ -> do
checkFeature "legalhold" owner tid enabled

testExposeInvitationURLsToTeamAdminConfig :: (HasCallStack) => FeatureTable -> App ()
testExposeInvitationURLsToTeamAdminConfig table = do
testExposeInvitationURLsToTeamAdminConfig :: (HasCallStack) => App ()
testExposeInvitationURLsToTeamAdminConfig = do
let cfgExposeInvitationURLsTeamAllowlist tids =
def
{ galleyCfg = setField "settings.exposeInvitationURLsTeamAllowlist" tids
Expand All @@ -118,7 +114,6 @@ testExposeInvitationURLsToTeamAdminConfig table = do
testNoAllowlistEntry :: (HasCallStack) => App (Value, String)
testNoAllowlistEntry = runCodensity (startDynamicBackend testBackend $ cfgExposeInvitationURLsTeamAllowlist ([] :: [String])) $ \_ -> do
(owner, tid, _) <- createTeam domain 1
updateMigrationState domain tid table
checkFeature "exposeInvitationURLsToTeamAdmin" owner tid disabledLocked
-- here we get a response with HTTP status 200 and feature status unchanged (disabled), which we find weird, but we're just testing the current behavior
-- a team that is not in the allow list cannot enable the feature, it will always be disabled and locked
Expand Down
Loading