[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 CompleteSummary of Changes Since Last ReviewThe latest commits (b2acdf2, 9a80efe, 69a91ff) addressed several findings from the previous review:
Findings 3, 4, and 8 were marked as working as intended by the author with valid explanations. Changes OverviewThis PR implements organization-enforced session timeout policies across 17 files (693 additions, 96 deletions). The implementation correctly follows the established architecture patterns with proper separation between Core and UI layers. Critical IssuesNone found. The previous critical issues have been addressed. Suggested ImprovementsFinding 9: Potential nil handling inconsistency in SyncService💭 In if type == SessionTimeoutType.onAppRestart || type == SessionTimeoutType.never {
try await stateService.setVaultTimeout(
value: timeoutValue, // User's current value, unchanged
)
}The call to
RationaleSetting a value to itself can confuse future maintainers. If this is intentional for triggering side effects (e.g., observers, notifications), document it clearly. Otherwise, eliminate the no-op. Finding 10: Missing documentation for public property
/// Whether or not to show the date picker.
@SwiftUI.State var showTimePicker = falseWhile this has a comment on line 26, it should use Finding 11: Test coverage gaps in SettingsPickerField♻️
Consider adding tests in
Finding 12: Incomplete edge case handling in policy enforcement💭 Recommendation: Add logging when skipping a policy to aid debugging: guard let policyTimeoutValue = policy[.minutes]?.intValue else {
services.errorReporter.log(error: PolicyServiceError.invalidPolicyData(
message: "Policy minutes value is nil for policy: \(policy.id)"
))
continue
}This helps diagnose misconfigured policies without failing the sync. Finding 13: Potential integer overflow in timeout calculations
/// The policy's timeout value in hours.
var policyTimeoutHours: Int {
policyTimeoutValue / 60 // Could overflow if policyTimeoutValue is Int.max
}While unlikely in practice (timeout values are constrained), defensive programming suggests: var policyTimeoutHours: Int {
guard policyTimeoutValue > 0 else { return 0 }
return policyTimeoutValue / 60
}Good Practices Observed
Action Items
Overall AssessmentStrong implementation that correctly enforces session timeout policies while maintaining backward compatibility. The latest commits successfully addressed all critical findings from the previous review. The remaining suggestions are minor improvements for code clarity, documentation completeness, and test coverage. The architecture is sound, following established patterns with proper layer separation. Security considerations are appropriately handled through the existing policy enforcement mechanisms. |
![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.38% 83.98% -1.41%
==========================================
Files 1725 1982 +257
Lines 145595 161404 +15809
==========================================
+ Hits 124316 135554 +11238
- Misses 21279 25850 +4571 ☔ 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
🎟️ 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