[PM-19305] Enforce session timeout policy #2127
Conversation
LRNcardozoWDF
requested review from
a team and
matt-livefront
as code owners
|
Claude finished @LRNcardozoWDF's task —— View job Code Review CompleteSummaryReviewed changes since 2025-11-26. The latest commit (ade0228) addressed some previous findings by adding DocC documentation to Status of Previous Findings✅ Resolved
❌ Still Unresolved
Critical Findings Requiring ActionFinding 1: All snapshot tests are disabled❌ BLOCKER All 7 snapshot tests in Location: BitwardenKit/UI/Platform/Application/Views/SettingsPickerField+SnapshotTests.swift:27-112 Required action: Remove the
Why this matters: These tests verify UI correctness across light/dark modes and accessibility settings. Disabled tests provide zero protection against visual regressions. Finding 2: Divider appears without footer textThe latest commit (ade0228) changed lines 57 and 66 from Location: BitwardenKit/UI/Platform/Application/Views/SettingsPickerField.swift:57,66 Current problematic code: if footer != nil { // Line 57: Shows divider even for empty strings
Divider()
.padding(.leading, 16)
}
// ...picker...
if footer != nil { // Line 66: Same issue
Divider()
.padding(.leading, 16)
}
if let footer { // Line 72: Only shows text when non-nil
Text(footer)
// ...
}The issue: Lines 57 and 66 check Suggested fix: if let footer, !footer.isEmpty {
Divider()
.padding(.leading, 16)
}Apply to both lines 57 and 66. This ensures dividers only appear when footer text will actually render. Additional ObservationsGood Practices
Notes💭 Spacing verification needed: Matt's comment about 8pt spacing between ContentBlocks (AccountSecurityView.swift:148) should be verified at runtime. The Action ItemsBefore merging:
Test coverage: Codecov shows 90.41% patch coverage with 95 lines missing. The disabled snapshot tests account for 81 of those missing lines - enabling them will significantly improve coverage. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| // MARK: Initialization | ||
|
|
||
| /// Initialize `EnvironmentURLData` with the specified URLs. |
There was a problem hiding this comment.
❌ Finding 1: Copy-paste error in documentation
The documentation still references EnvironmentURLData and mentions an api parameter, but this is the SessionTimeoutPolicy initializer with completely different parameters.
| /// Initialize `EnvironmentURLData` with the specified URLs. | |
| /// Initialize `SessionTimeoutPolicy` with the specified values. | |
| /// | |
| /// - Parameters: | |
| /// - timeoutAction: The action to perform on session timeout. | |
| /// - timeoutType: The type of session timeout. | |
| /// - timeoutValue: The session timeout value. |
| "ThisSettingIsManagedByYourOrganization" = "This setting is managed by your organization."; | ||
| "YourOrganizationHasSetTheDefaultSessionTimeoutToX" = "Your organization has set the default session timeout to %1$@."; | ||
| "YourOrganizationHasSetTheDefaultSessionTimeoutToXAndY" = "Your organization has set the default session timeout to %1$@ and %2$@."; | ||
| "XMinutes" = "%1$@ minute(s)"; |
There was a problem hiding this comment.
The string "XMinutes" = "%1$@ minute(s)"; uses generic "(s)" pluralization which won't work correctly for all languages. iOS supports proper pluralization via .stringsdict files.
Consider creating a Localizable.stringsdict entry:
<key>XMinutes</key>
<dict>
<key>NSStringLocalizedFormatKey</key>
<string>%#@minutes@</string>
<key>minutes</key>
<dict>
<key>NSStringFormatSpecTypeKey</key>
<string>NSStringPluralRuleType</string>
<key>NSStringFormatValueTypeKey</key>
<string>d</string>
<key>one</key>
<string>%d minute</string>
<key>other</key>
<string>%d minutes</string>
</dict>
</dict>The same applies to XHours if it exists.
| // Only update the user's stored vault timeout value if | ||
| // their stored timeout value is > the policy's timeout value. | ||
| if timeoutValue.rawValue > value || timeoutValue.rawValue < 0 { | ||
| if type == SessionTimeoutType.onAppRestart || type == SessionTimeoutType.never { |
There was a problem hiding this comment.
❌ Finding 3: Logic inconsistency bypasses policy enforcement
When the policy type is onAppRestart or never, this code sets the timeout value to whatever the user currently has stored (timeoutValue), without any validation against the policy. This means:
- If a user previously had a custom timeout that violates the new policy, it won't be corrected
- The policy value is essentially ignored for these types
The logic should validate that the user's stored value is compatible with the policy type. For example:
- If policy type is
never, user's value should be set to.never - If policy type is
onAppRestart, user's value should be set to.onAppRestart
Otherwise, the user could have a stored value of "5 minutes" when the policy says "never", which is inconsistent.
There was a problem hiding this comment.
The implemented behaviour is correct, the user's value can in those cases be different than the policy since the policy type also allows those values.
| state.policyTimeoutValue = policy.value | ||
| state.isTimeoutPolicyEnabled = true | ||
| state.policyTimeoutAction = policy.timeoutAction | ||
| state.policyTimeoutType = policy.timeoutType ?? SessionTimeoutType.custom |
There was a problem hiding this comment.
💭 Finding 4: Unclear default handling for nil policy type
When policy.timeoutType is nil (legacy servers), this defaults to SessionTimeoutType.custom. However, the logic in PolicyService.swift:284-286 suggests that nil type should be handled differently. Is custom always the correct default for legacy servers?
Consider documenting why .custom is the appropriate fallback, or if the logic should check the policy value to determine a more appropriate type (e.g., if value matches a predefined timeout like 60 minutes, should it be .predefined instead?).
There was a problem hiding this comment.
.custom is always the correct default for legacy servers since they will send the timeoutType as null.
| var policyTimeoutValue: Int = 0 { | ||
| didSet { | ||
| availableTimeoutOptions = SessionTimeoutValue.allCases | ||
| .filter { $0 != .never } | ||
| .filter { $0 != .onAppRestart } | ||
| .filter { $0.rawValue <= policyTimeoutValue } | ||
| availableTimeoutOptions = SessionTimeoutValue.allCases.filter { option in |
There was a problem hiding this comment.
🎨 Finding 7: Complex logic in property observer reduces testability
The didSet observer contains significant filtering logic (27 lines) that modifies availableTimeoutOptions based on policyTimeoutType and policyTimeoutValue. This makes the state harder to test and understand.
Consider extracting this to a computed property or dedicated method:
var policyTimeoutValue: Int = 0
var availableTimeoutOptions: [SessionTimeoutValue] {
computeAvailableTimeoutOptions(
type: policyTimeoutType,
value: policyTimeoutValue
)
}
private func computeAvailableTimeoutOptions(
type: SessionTimeoutType?,
value: Int
) -> [SessionTimeoutValue] {
// Filtering logic here
}This approach:
- Makes the logic independently testable
- Clarifies the dependency on both
policyTimeoutTypeandpolicyTimeoutValue - Removes side effects from property setters
| /// | ||
| /// - Parameter rawValue: The string representation of the type raw value. | ||
| /// | ||
| public init(rawValue: String?) { |
There was a problem hiding this comment.
This enum lacks dedicated unit tests, particularly for:
- The
init(rawValue:)method with all possible string values - The legacy "onSystemLock" → "onAppRestart" mapping (line 70)
- The default case handling (line 73) which falls back to
.custom - The
init(value:)method with all SessionTimeoutValue cases
These initialization paths are critical for policy enforcement compatibility with legacy servers. Add tests in a new file: BitwardenKit/Core/Platform/Models/Enum/SessionTimeoutTypeTests.swift
| var timeoutValue: SessionTimeoutValue? | ||
|
|
||
| for policy in policies { | ||
| // This is never null, but maybe it should be revised |
There was a problem hiding this comment.
💭 Finding 8: Comment suggests uncertainty about business logic
The comment "This is never null, but maybe it should be revised" indicates uncertainty about whether the policy's minutes value can be null. This warrants clarification:
- If it truly can never be null per the API contract, remove the guard and the comment
- If it can be null and represents a valid state, the
continueis correct but should be documented - If the server contract is unclear, add error logging before continuing
This is important for policy enforcement - skipping a policy without logging could hide configuration issues.
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2127 +/- ##
==========================================
- Coverage 85.39% 84.15% -1.24%
==========================================
Files 1731 1989 +258
Lines 145718 161985 +16267
==========================================
+ Hits 124433 136326 +11893
- Misses 21285 25659 +4374 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
LRNcardozoWDF
force-pushed
the
cmcg/pm-19305-vault-timeout-policy
branch
from
60abeb5 to
8bb7d69
Compare
| if footer != nil { | ||
| Group { | ||
| if let footer { | ||
| Text(footer) | ||
| .styleGuide(.subheadline) | ||
| .foregroundColor(Color(asset: SharedAsset.Colors.textSecondary)) | ||
| } | ||
| } | ||
| .padding(EdgeInsets(top: 12, leading: 16, bottom: 12, trailing: 16)) | ||
| } |
There was a problem hiding this comment.
🎨 You could simplify this by unwrapping footer once and removing the Group:
| if footer != nil { | |
| Group { | |
| if let footer { | |
| Text(footer) | |
| .styleGuide(.subheadline) | |
| .foregroundColor(Color(asset: SharedAsset.Colors.textSecondary)) | |
| } | |
| } | |
| .padding(EdgeInsets(top: 12, leading: 16, bottom: 12, trailing: 16)) | |
| } | |
| if let footer { | |
| Text(footer) | |
| .styleGuide(.subheadline) | |
| .foregroundColor(Color(asset: SharedAsset.Colors.textSecondary)) | |
| .padding(EdgeInsets(top: 12, leading: 16, bottom: 12, trailing: 16)) | |
| } |
| /// - Returns: The timeout value in minutes, and the action to take upon timeout. | ||
| /// | ||
| func fetchTimeoutPolicyValues() async throws -> (action: SessionTimeoutAction?, value: Int)? | ||
| func fetchTimeoutPolicyValues() async throws -> SessionTimeoutPolicy? |
There was a problem hiding this comment.
⛏️ Could you update the documentation for the return value here?
|
|
||
| try await subject.fetchSync(forceSync: false) | ||
|
|
||
| XCTAssertEqual(stateService.timeoutAction["1"], .logout) |
There was a problem hiding this comment.
🎨 Should this verify the timeout value as well?
BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityState.swift
Show resolved
Hide resolved
| !removeUnlockWithPinPolicyEnabled || isUnlockWithPINCodeOn | ||
| } | ||
|
|
||
| var customTimeoutMessage: String { |
There was a problem hiding this comment.
⛏️ Can you add docs to this and alphabetize it among the other properties?
| SectionView(Localizations.sessionTimeout, contentSpacing: 8) { | ||
| VStack(spacing: 16) { | ||
| if let policyTimeoutMessage = store.state.policyTimeoutMessage { | ||
| InfoContainer(policyTimeoutMessage) | ||
| } |
There was a problem hiding this comment.
| SessionTimeoutValue.allCases.filter { option in | ||
| switch type { | ||
| case .never: | ||
| return true | ||
| case .onAppRestart: | ||
| return option != .never | ||
| case .immediately: | ||
| return option == .immediately | ||
| case .custom: | ||
| if option.isCustomPlaceholder { return true } | ||
| guard let time = option.minutesValue else { return false } | ||
| return time <= value | ||
| case nil: | ||
| if value > 0 { | ||
| if option.isCustomPlaceholder { return true } | ||
| guard let time = option.minutesValue else { return false } | ||
| return time <= value | ||
| } else { | ||
| return true |
There was a problem hiding this comment.
🎨 This logic took me a bit to figure out, would it be good to write some tests for this?
There was a problem hiding this comment.
I test the results of the filtering here . I can also try and return the array with the right options given a type:
| SessionTimeoutValue.allCases.filter { option in | |
| switch type { | |
| case .never: | |
| return true | |
| case .onAppRestart: | |
| return option != .never | |
| case .immediately: | |
| return option == .immediately | |
| case .custom: | |
| if option.isCustomPlaceholder { return true } | |
| guard let time = option.minutesValue else { return false } | |
| return time <= value | |
| case nil: | |
| if value > 0 { | |
| if option.isCustomPlaceholder { return true } | |
| guard let time = option.minutesValue else { return false } | |
| return time <= value | |
| } else { | |
| return true | |
| var availableTimeoutOptions: [SessionTimeoutValue] { | |
| switch policyTimeoutType { | |
| case .never: | |
| SessionTimeoutValue.allCases | |
| case .onAppRestart: | |
| [ | |
| .immediately, | |
| .oneMinute, | |
| .fiveMinutes, | |
| .fifteenMinutes, | |
| .thirtyMinutes, | |
| .oneHour, | |
| .fourHours, | |
| .onAppRestart, | |
| .custom(-100), | |
| ] | |
| case .immediately: | |
| [.immediately] | |
| case .custom: | |
| SessionTimeoutValue.allCases.filter { option in | |
| option.isCustomPlaceholder || (option.minutesValue.map { $0 <= policyTimeoutValue } ?? false) | |
| } | |
| case nil: | |
| if policyTimeoutValue > 0 { | |
| SessionTimeoutValue.allCases.filter { option in | |
| option.isCustomPlaceholder || (option.minutesValue.map { $0 <= policyTimeoutValue } ?? false) | |
| } | |
| } else { | |
| SessionTimeoutValue.allCases | |
| } | |
| } | |
| } |
| send: AccountSecurityAction.customTimeoutValueSecondsChanged, | ||
| ), | ||
| hasDivider: false, | ||
| hasDivider: true, |
There was a problem hiding this comment.
🤔 Should the divider only show if there's a footer? I see it here without a footer, which I don't think is right. Since we use ContentBlock to provide the dividers now across the app, it might be easier to remove the property and just add the divider in the component if there's a footer?
| .styleGuide(.subheadline) | ||
| .foregroundColor(Color(asset: SharedAsset.Colors.textSecondary)) | ||
| .padding(EdgeInsets(top: 12, leading: 16, bottom: 12, trailing: 16)) | ||
| } |
There was a problem hiding this comment.
🎨 Finding 1: Redundant footer nil checks create visual noise
The current implementation checks footer != nil three times (lines 57, 66, 72). Matt's suggestion to simplify by unwrapping once would improve readability:
if let footer {
Text(footer)
.styleGuide(.subheadline)
.foregroundColor(Color(asset: SharedAsset.Colors.textSecondary))
.padding(EdgeInsets(top: 12, leading: 16, bottom: 12, trailing: 16))
}Then update lines 57 and 66 to simply check the unwrapped footer variable instead of footer != nil.
| ContentBlock(dividerLeadingPadding: 16) { | ||
| BitwardenMenuField( | ||
| title: Localizations.sessionTimeout, | ||
| footer: store.state.policyTimeoutMessage, |
There was a problem hiding this comment.
As noted in Matt's review, there should be 8pt of padding between the "Session timeout" and "Session timeout action" ContentBlocks. Currently, they appear directly adjacent without proper spacing.
Since you removed the VStack wrapper, the contentSpacing: 8 parameter on the SectionView (line 148) should provide the spacing. However, verify this displays correctly - you may need to test that the spacing between ContentBlocks matches the design spec.
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💭 Finding 3: Complex filtering logic in computed property has limited test coverage
The availableTimeoutOptions computed property contains 27 lines of filtering logic. While there are tests for the results in AccountSecurityProcessorTests.swift:116-125, Matt suggested adding dedicated tests for this logic.
Consider extracting this to a testable function and adding unit tests that cover all branches:
.nevertype (returns all cases).onAppRestarttype (filters out.never).immediatelytype (returns only.immediately).customtype with various valuesniltype withvalue > 0niltype withvalue == 0
This would make the logic easier to verify and maintain.
| // Only update the user's stored vault timeout value if | ||
| // their stored timeout value is > the policy's timeout value. | ||
| if timeoutValue.rawValue > value || timeoutValue.rawValue < 0 { | ||
| if type == SessionTimeoutType.onAppRestart || type == SessionTimeoutType.never { |
There was a problem hiding this comment.
When the policy type is onAppRestart or never, this code sets timeoutValue to whatever the user currently has stored, without any validation. While the author clarified this is intentional behavior, the code lacks documentation explaining why.
This could lead to confusion: if a user had a stored value of "5 minutes" and the policy changes to "never", the stored value remains "5 minutes" even though it doesn't match the policy intent.
Add a comment explaining the business logic:
// For onAppRestart and never policy types, preserve the user's current timeout value
// as these policy types don't restrict the value itself, only the behavior
| var timeoutValue: SessionTimeoutValue? | ||
|
|
||
| for policy in policies { | ||
| guard let policyTimeoutValue = policy[.minutes]?.intValue else { continue } |
There was a problem hiding this comment.
Matt noted that the return value documentation needs updating. The function now returns SessionTimeoutPolicy? instead of the previous return type, but the DocC comment above this function likely still reflects the old signature.
Update the documentation to accurately describe what the function returns and under what conditions it returns nil.
|
|
||
| try await subject.fetchSync(forceSync: false) | ||
|
|
||
| XCTAssertEqual(stateService.timeoutAction["1"], .logout) |
There was a problem hiding this comment.
💭 Finding 6: Test should verify timeout value
Matt suggested this test should verify the timeout value as well. Currently the test only checks that the sync completed successfully, but doesn't validate that the timeout policy was applied correctly.
Add an assertion to verify the expected timeout value was set by the policy.
| } | ||
|
|
||
| /// The message to display if a timeout policy is in effect for the user. | ||
| var policyTimeoutCustomMessage: String? { |
There was a problem hiding this comment.
⛏️ Finding 7: Missing documentation and alphabetization for policyTimeoutCustomMessage
Matt noted this property needs:
- DocC documentation explaining its purpose
- Alphabetization among other properties
The property should be documented with /// comments and placed in alphabetical order with the other computed properties in this section.
LRNcardozoWDF
force-pushed
the
cmcg/pm-19305-vault-timeout-policy
branch
from
fc4e4a8 to
1fde466
Compare
| .styleGuide(.subheadline) | ||
| .foregroundColor(Color(asset: SharedAsset.Colors.textSecondary)) | ||
| .padding(EdgeInsets(top: 12, leading: 16, bottom: 12, trailing: 16)) | ||
| } |
There was a problem hiding this comment.
The current logic shows dividers whenever footer != nil, but this creates the visual issue Matt identified where a divider appears without accompanying footer text.
The issue: Lines 57 and 66 check if footer != nil to show dividers, and line 72 uses if let footer to show the actual text. This means dividers render even when footer is an empty string or when the text won't display.
Suggested fix:
Since the footer text block (lines 72-77) already uses if let footer, the dividers should follow the same pattern. The divider before the picker (line 57) and after the picker (line 66) should only appear when the footer text will actually render.
Alternatively, remove the divider checks on lines 57 and 66 entirely if dividers aren't needed when there's no footer.
| } | ||
|
|
||
| /// The message to display if a timeout policy is in effect for the user. | ||
| var policyTimeoutCustomMessage: String? { |
There was a problem hiding this comment.
Per project guidelines and Matt's review comment, this computed property needs DocC documentation. The property has complex conditional logic returning different messages based on policy type.
Add documentation like:
/// The message to display in the custom timeout field when a policy is in effect.
///
/// Returns different messages based on the policy type:
/// - `.custom`: Shows the specific timeout duration from the policy
/// - `.immediately`: Indicates the setting is managed by the organization
/// - `.never`/`.onAppRestart`: Shows the policy's timeout type
/// - `nil`: Returns `nil` if no policy is in effect
///
var policyTimeoutCustomMessage: String? {
| // MARK: Tests | ||
|
|
||
| /// Test a snapshot of the picker field in light mode without footer. | ||
| func disabletest_snapshot_lightMode_noFooter() { |
There was a problem hiding this comment.
❌ Finding 1: All snapshot tests are disabled
All 7 snapshot tests have the disabletest_ prefix instead of test_, which means they never execute. This violates the testing guidelines (Testing.md:172-174) requiring snapshot tests for light mode, dark mode, and large dynamic type.
Required action: Remove the disable prefix from all test function names:
test_snapshot_lightMode_noFooter()test_snapshot_darkMode_noFooter()test_snapshot_largeDynamicType_noFooter()- etc.
| } | ||
|
|
||
| if hasDivider { | ||
| if let footer { |
There was a problem hiding this comment.
❌ Finding 2: Divider appears even when footer text won't render
This divider shows whenever footer != nil, but the actual footer text (lines 72-77) only renders when footer has content. This creates the visual issue Matt identified where a divider appears without accompanying text.
Suggested fix:
if let footer, !footer.isEmpty {
Divider()
.padding(.leading, 16)
}Apply the same fix to line 66.
|
|
||
| /// The session timeout section. | ||
| private var sessionTimeoutSection: some View { | ||
| SectionView(Localizations.sessionTimeout, contentSpacing: 8) { |
There was a problem hiding this comment.
Matt identified that there should be 8pt of padding between the "Session timeout" and "Session timeout action" blocks. While contentSpacing: 8 should provide this, please verify at runtime that the spacing displays correctly per the design spec.
| } | ||
|
|
||
| /// The message to display if a timeout policy is in effect for the user. | ||
| var policyTimeoutCustomMessage: String? { |
There was a problem hiding this comment.
⛏️ Finding 4 & 5: Missing DocC documentation and needs alphabetization
Per project guidelines (CLAUDE.md:13) and Matt's review:
- Add DocC documentation explaining the property's purpose and return values:
/// The message to display in the custom timeout field when a policy is in effect.
///
/// Returns different messages based on the policy type:
/// - `.custom`: Shows the organization's configured timeout duration
/// - `.immediately`: Indicates the setting is managed by the organization
/// - `.never`/`.onAppRestart`: Shows the policy's timeout type
/// - Returns `nil` if no policy is in effect
///
var policyTimeoutCustomMessage: String? {- Alphabetize this property among other computed properties in this section.
|
|
||
| try await subject.fetchSync(forceSync: false) | ||
|
|
||
| XCTAssertEqual(stateService.timeoutAction["1"], .logout) |
There was a problem hiding this comment.
💭 Finding 7: Test should verify timeout value
Matt suggested this test should verify the timeout value was set correctly, not just the timeout action. Consider adding:
XCTAssertEqual(stateService.vaultTimeout["1"], /* expected SessionTimeoutValue based on policy */)There was a problem hiding this comment.
Claude, I added that validation in the line below.
🎟️ Tracking
PM-19305
📔 Objective
Allow organizations to enforce session timeout policy in the iOS app.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes