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

Remove VPN metadata admin check #2127

Merged
merged 2 commits into from
Jan 29, 2024
Merged

Remove VPN metadata admin check #2127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1b066ab
Remove admin user check.
samsymons Jan 27, 2024
dbf8293
Update tests.
samsymons Jan 27, 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
58 changes: 0 additions & 58 deletions DuckDuckGo/VPNFeedbackForm/VPNMetadataCollector.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ struct VPNMetadata: Encodable {
let appVersion: String
let lastVersionRun: String
let isInternalUser: Bool
let isAdminUser: String
let isInApplicationsDirectory: Bool
}

Expand Down Expand Up @@ -157,14 +156,12 @@ final class DefaultVPNMetadataCollector: VPNMetadataCollector {
let appVersion = AppVersion.shared.versionAndBuildNumber
let versionStore = NetworkProtectionLastVersionRunStore()
let isInternalUser = NSApp.delegateTyped.internalUserDecider.isInternalUser
let isAdminUser = isAdminUser()
let isInApplicationsDirectory = Bundle.main.isInApplicationsDirectory

return .init(
appVersion: appVersion,
lastVersionRun: versionStore.lastVersionRun ?? "Unknown",
isInternalUser: isInternalUser,
isAdminUser: isAdminUser,
isInApplicationsDirectory: isInApplicationsDirectory
)
}
Expand Down Expand Up @@ -276,59 +273,4 @@ final class DefaultVPNMetadataCollector: VPNMetadataCollector {

}

// MARK: - Admin User

private enum AdminQueryError: Error {
case queryExecutionFailed
case queriedWithoutResult
}

extension VPNMetadataCollector {

private func getUser() throws -> CSIdentity? {
let query = CSIdentityQueryCreateForCurrentUser(kCFAllocatorDefault).takeRetainedValue()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This function is being a real pain. Changing it to takeUnretainedValue solves the problem, but leaks the query instances. Since we get error information about lack of privileges elsewhere, I think this one is a little redundant anyway, so let's just remove it and keep things simple.

let flags = CSIdentityQueryFlags()

guard CSIdentityQueryExecute(query, flags, nil) else {
throw AdminQueryError.queryExecutionFailed
}

let users = CSIdentityQueryCopyResults(query).takeRetainedValue() as? [CSIdentity]
return users?.first
}

private func getAdminGroup() throws -> CSIdentity {
let privilegeGroup = "admin" as CFString
let authority = CSGetDefaultIdentityAuthority().takeRetainedValue()
let query = CSIdentityQueryCreateForName(kCFAllocatorDefault,
privilegeGroup,
kCSIdentityQueryStringEquals,
kCSIdentityClassGroup,
authority).takeRetainedValue()
let flags = CSIdentityQueryFlags()

guard CSIdentityQueryExecute(query, flags, nil) else { throw AdminQueryError.queryExecutionFailed }
let groups = CSIdentityQueryCopyResults(query).takeRetainedValue() as? [CSIdentity]

guard let adminGroup = groups?.first else {
throw AdminQueryError.queriedWithoutResult
}

return adminGroup
}

fileprivate func isAdminUser() -> String {
do {
let user = try self.getUser()
let group = try self.getAdminGroup()

let isAdmin = CSIdentityIsMemberOfGroup(user, group)
return String(describing: isAdmin)
} catch {
return "error checking status: \(error.localizedDescription)"
}
}

}

#endif
1 change: 0 additions & 1 deletion UnitTests/VPNFeedbackForm/VPNFeedbackFormViewModelTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ private class MockVPNMetadataCollector: VPNMetadataCollector {
appVersion: "1.2.3",
lastVersionRun: "1.2.3",
isInternalUser: false,
isAdminUser: "true",
isInApplicationsDirectory: true
)

Expand Down
Loading