Skip to content
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.

Sign up for GitHub

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

Check subfeature state before reading rollout data #649

Merged
merged 3 commits into from
Feb 7, 2024
Merged

Check subfeature state before reading rollout data #649

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
04c0f94
Check subfeature state before reading rollout data
dus7 Feb 1, 2024
40bb18b
Add test for changing subfeature state to disabled during rollout
dus7 Feb 2, 2024
b11b744
Fix flaky test and expected value
dus7 Feb 2, 2024
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
19 changes: 8 additions & 11 deletions Sources/BrowserServicesKit/PrivacyConfig/AppPrivacyConfiguration.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,27 +195,24 @@ public struct AppPrivacyConfiguration: PrivacyConfiguration {
let subfeatures = subfeatures(for: subfeature.parent)
let subfeatureData = subfeatures[subfeature.rawValue]

// Handle Rollouts
if let rollout = subfeatureData?.rollout {
if !isRolloutEnabled(subfeature: subfeature, rolloutSteps: rollout.steps, randomizer: randomizer) {
return .disabled(.stillInRollout)
}
}

let satisfiesMinVersion = satisfiesMinVersion(subfeatureData?.minSupportedVersion, versionProvider: versionProvider)

switch subfeatureData?.state {
case PrivacyConfigurationData.State.enabled:
guard satisfiesMinVersion else { return .disabled(.appVersionNotSupported) }

return .enabled
case PrivacyConfigurationData.State.internal:
guard internalUserDecider.isInternalUser else { return .disabled(.limitedToInternalUsers) }
guard satisfiesMinVersion else { return .disabled(.appVersionNotSupported) }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably could extract this from 202 and 205 lines to 208 to simplify logic flow

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was considering this, but it means for internal user we would enable subfeature regardless of supported version and moving these two checks before state verification would result in reporting not supported version before disabled (not sure if this is a concern). I’d prefer to keep it as it is TBH.


return .enabled
default: return .disabled(.disabledInConfig)
}

// Handle Rollouts
if let rollout = subfeatureData?.rollout,
!isRolloutEnabled(subfeature: subfeature, rolloutSteps: rollout.steps, randomizer: randomizer) {
return .disabled(.stillInRollout)
}

return .enabled
}

private func subfeatures(for feature: PrivacyFeature) -> PrivacyConfigurationData.PrivacyFeature.Features {
Expand Down
71 changes: 69 additions & 2 deletions Tests/BrowserServicesKitTests/PrivacyConfig/AppPrivacyConfigurationTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -880,10 +880,77 @@ class AppPrivacyConfigurationTests: XCTestCase {
let config = manager.privacyConfig

clearRolloutData(feature: "autofill", subFeature: "accessCredentialManagement")
XCTAssertFalse(config.isSubfeatureEnabled(AutofillSubfeature.accessCredentialManagement), "Subfeature should be enabled if rollouts array is empty")
XCTAssertEqual(config.stateFor(AutofillSubfeature.accessCredentialManagement), .disabled(.stillInRollout))
XCTAssertFalse(config.isSubfeatureEnabled(AutofillSubfeature.accessCredentialManagement, randomizer: mockRandom(in:)), "Subfeature should be disabled by state setting")
XCTAssertEqual(config.stateFor(AutofillSubfeature.accessCredentialManagement, randomizer: mockRandom(in:)), .disabled(.disabledInConfig))
}

func testWhenCheckingSubfeatureStateWithRolloutsAndSubfeatureDisabledWhenPreviouslyInRollout_SubfeatureShouldBeDisabled() {
let mockEmbeddedData = MockEmbeddedDataProvider(data: exampleEnabledSubfeatureWithRollout, etag: "test")
let manager = PrivacyConfigurationManager(fetchedETag: nil,
fetchedData: nil,
embeddedDataProvider: mockEmbeddedData,
localProtection: MockDomainsProtectionStore(),
internalUserDecider: DefaultInternalUserDecider())

let config = manager.privacyConfig

clearRolloutData(feature: "autofill", subFeature: "credentialsSaving")
XCTAssertTrue(config.isSubfeatureEnabled(AutofillSubfeature.credentialsSaving), "Subfeature should be enabled with 100% rollout")
XCTAssertEqual(config.stateFor(AutofillSubfeature.credentialsSaving), .enabled)

// Update remote config
manager.reload(etag: "foo", data: exampleDisabledSubfeatureWithRollout)

let configAfterUpdate = manager.privacyConfig

XCTAssertFalse(configAfterUpdate.isSubfeatureEnabled(AutofillSubfeature.credentialsSaving), "Subfeature should be disabled")
XCTAssertEqual(configAfterUpdate.stateFor(AutofillSubfeature.credentialsSaving, randomizer: mockRandom(in:)), .disabled(.disabledInConfig))
}

let exampleEnabledSubfeatureWithRollout =
"""
{
"features": {
"autofill": {
"state": "enabled",
"exceptions": [],
"features": {
"credentialsSaving": {
"state": "enabled",
"rollout": {
"steps": [{
"percent": 100
}]
}
}
}
}
}
}
""".data(using: .utf8)!

let exampleDisabledSubfeatureWithRollout =
"""
{
"features": {
"autofill": {
"state": "enabled",
"exceptions": [],
"features": {
"credentialsSaving": {
"state": "disabled",
"rollout": {
"steps": [{
"percent": 100
}]
}
}
}
}
},
}
""".data(using: .utf8)!

func exampleTrackerAllowlistConfig(with state: String) -> Data {
return
"""
Expand Down
Loading