-
-
Notifications
You must be signed in to change notification settings - Fork 983
Improve login to use ASWebAuthenticationSession with integrated browser #3938
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
base: master
Are you sure you want to change the base?
Changes from all commits
2149dba
ca83d0a
a931ab9
1d716cb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -8,7 +8,7 @@ import UIKit | |||||||
| import NextcloudKit | ||||||||
| import SwiftEntryKit | ||||||||
| import SwiftUI | ||||||||
| import SafariServices | ||||||||
| import AuthenticationServices | ||||||||
|
|
||||||||
| class NCLogin: UIViewController, UITextFieldDelegate, NCLoginQRCodeDelegate { | ||||||||
| @IBOutlet weak var imageBrand: UIImageView! | ||||||||
|
|
@@ -28,6 +28,8 @@ class NCLogin: UIViewController, UITextFieldDelegate, NCLoginQRCodeDelegate { | |||||||
| private var activeTextField = UITextField() | ||||||||
|
|
||||||||
| private var shareAccounts: [NKShareAccounts.DataAccounts]? | ||||||||
| private let loginFlowPoller = NCLoginFlowPoller() | ||||||||
| private var authenticationSession: ASWebAuthenticationSession? | ||||||||
|
|
||||||||
| /// Controller | ||||||||
| var controller: NCMainTabBarController? | ||||||||
|
|
@@ -197,6 +199,16 @@ class NCLogin: UIViewController, UITextFieldDelegate, NCLoginQRCodeDelegate { | |||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| override func viewDidDisappear(_ animated: Bool) { | ||||||||
| super.viewDidDisappear(animated) | ||||||||
|
|
||||||||
| if isMovingFromParent || isBeingDismissed { | ||||||||
| loginFlowPoller.cancel() | ||||||||
| authenticationSession?.cancel() | ||||||||
| authenticationSession = nil | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| private func handleLoginWithAppConfig() { | ||||||||
| let accountCount = NCManageDatabase.shared.getAccounts()?.count ?? 0 | ||||||||
|
|
||||||||
|
|
@@ -330,14 +342,9 @@ class NCLogin: UIViewController, UITextFieldDelegate, NCLoginQRCodeDelegate { | |||||||
| let loginOptions = NKRequestOptions(customUserAgent: userAgent) | ||||||||
| NextcloudKit.shared.getLoginFlowV2(serverUrl: url, options: loginOptions) { [self] token, endpoint, login, _, error in | ||||||||
| // Login Flow V2 | ||||||||
| if error == .success, let token, let endpoint, let login { | ||||||||
| if error == .success, let token, let endpoint, let login, let loginURL = URL(string: login) { | ||||||||
| nkLog(debug: "Successfully received login flow information.") | ||||||||
| let safariVC = NCLoginProvider() | ||||||||
| safariVC.initialURLString = login | ||||||||
| safariVC.uiColor = textColor | ||||||||
| safariVC.delegate = self | ||||||||
| safariVC.startPolling(loginFlowV2Token: token, loginFlowV2Endpoint: endpoint, loginFlowV2Login: login) | ||||||||
| navigationController?.pushViewController(safariVC, animated: true) | ||||||||
| startASWebAuthenticationSession(loginURL: loginURL, token: token, endpoint: endpoint) | ||||||||
| } | ||||||||
| } | ||||||||
| case .failure(let error): | ||||||||
|
|
@@ -371,6 +378,71 @@ class NCLogin: UIViewController, UITextFieldDelegate, NCLoginQRCodeDelegate { | |||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| private func startLoginFlowPolling(token: String, endpoint: String) { | ||||||||
| loginFlowPoller.start(token: token, endpoint: endpoint) { [weak self] grant in | ||||||||
| // Finish login v2 flow | ||||||||
| await self?.handleLoginGrant(grant) | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| private func startASWebAuthenticationSession(loginURL: URL, token: String, endpoint: String) { | ||||||||
| let callbackScheme = URL(string: NCBrandOptions.shared.webLoginAutenticationProtocol)?.scheme ?? loginURL.scheme | ||||||||
|
|
||||||||
| let session = ASWebAuthenticationSession(url: loginURL, | ||||||||
| callbackURLScheme: callbackScheme, | ||||||||
| completionHandler: handleAuthenticationSessionCompletion(callbackURL:error:)) | ||||||||
|
|
||||||||
| session.presentationContextProvider = self | ||||||||
| session.prefersEphemeralWebBrowserSession = true | ||||||||
|
|
||||||||
| authenticationSession = session | ||||||||
|
|
||||||||
| if session.start() { | ||||||||
| startLoginFlowPolling(token: token, endpoint: endpoint) | ||||||||
| } else { | ||||||||
| authenticationSession = nil | ||||||||
| loginFlowPoller.cancel() | ||||||||
| let error = NKError(errorCode: NCGlobal.shared.errorInternalError, errorDescription: "_error_something_wrong_") | ||||||||
| NCContentPresenter().showError(error: error, priority: .max) | ||||||||
| } | ||||||||
|
Comment on lines
+400
to
+407
|
||||||||
| } | ||||||||
|
|
||||||||
| @MainActor | ||||||||
| private func handleLoginGrant(_ grant: NCLoginGrant) async { | ||||||||
| authenticationSession?.cancel() | ||||||||
| authenticationSession = nil | ||||||||
| createAccount(urlBase: grant.urlBase, user: grant.loginName, password: grant.appPassword) | ||||||||
| } | ||||||||
|
|
||||||||
| @MainActor | ||||||||
|
||||||||
| @MainActor |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a potential race condition where the polling task could complete and call handleLoginGrant (line 384) at the same time as the authentication session completion handler calls it (line 442). Both paths could execute simultaneously if the user completes the login in the browser right when polling succeeds. Consider adding a flag or synchronization mechanism to ensure the account is only created once.
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The onBack() function cancels both the login flow poller and authentication session, but it doesn't clean up the state in NCLogin like viewDidDisappear does. If the user navigates back from the WebView login provider, the authentication session and poller references in NCLogin remain active. Consider adding similar cleanup logic here or consolidating the cleanup into a shared method.
| loginFlowPoller.cancel() | |
| loginFlowPoller.cancel() | |
| loginFlowPoller = nil |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| // SPDX-FileCopyrightText: Nextcloud GmbH | ||
| // SPDX-License-Identifier: GPL-3.0-or-later | ||
|
|
||
| import Foundation | ||
| import NextcloudKit | ||
|
|
||
| struct NCLoginGrant { | ||
| let urlBase: String | ||
| let loginName: String | ||
|
||
| let appPassword: String | ||
| } | ||
|
|
||
| final class NCLoginFlowPoller { | ||
| private var pollingTask: Task<Void, Never>? | ||
|
|
||
| func start(token: String, endpoint: String, onGrant: @escaping @MainActor (NCLoginGrant) async -> Void) { | ||
| cancel() | ||
|
|
||
| let options = NKRequestOptions(customUserAgent: userAgent) | ||
| nkLog(start: "Starting polling at \(endpoint) with token \(token)") | ||
|
|
||
| pollingTask = Task { [weak self] in | ||
| guard let self else { return } | ||
| defer { self.pollingTask = nil } | ||
|
|
||
| guard let grantValues = await self.waitForGrant(token: token, endpoint: endpoint, options: options) else { | ||
| return | ||
| } | ||
|
|
||
| await onGrant(grantValues) | ||
| nkLog(debug: "Login flow polling task completed.") | ||
| } | ||
|
|
||
| nkLog(debug: "Login flow polling task created.") | ||
| } | ||
|
|
||
| func cancel() { | ||
| guard pollingTask != nil else { | ||
| return | ||
| } | ||
|
|
||
| nkLog(debug: "Cancelling login polling task...") | ||
| pollingTask?.cancel() | ||
| pollingTask = nil | ||
| } | ||
|
|
||
| private func waitForGrant(token: String, endpoint: String, options: NKRequestOptions) async -> NCLoginGrant? { | ||
| var grantValues: NCLoginGrant? | ||
|
|
||
| repeat { | ||
| guard !Task.isCancelled else { | ||
| nkLog(debug: "Login polling task cancelled before receiving grant values.") | ||
| return nil | ||
| } | ||
|
|
||
| grantValues = await pollOnce(token: token, endpoint: endpoint, options: options) | ||
| if grantValues == nil { | ||
| try? await Task.sleep(nanoseconds: 1_000_000_000) // .seconds() is not supported on iOS 15 yet. | ||
| } | ||
| } while grantValues == nil | ||
|
|
||
| return grantValues | ||
| } | ||
|
|
||
| private func pollOnce(token: String, endpoint: String, options: NKRequestOptions) async -> NCLoginGrant? { | ||
| await withCheckedContinuation { continuation in | ||
| NextcloudKit.shared.getLoginFlowV2Poll(token: token, endpoint: endpoint, options: options) { server, loginName, appPassword, _, error in | ||
|
|
||
| guard error == .success else { | ||
| nkLog(error: "Login poll result for token \"\(token)\" is not successful!") | ||
| continuation.resume(returning: nil) | ||
| return | ||
| } | ||
|
|
||
| guard let urlBase = server else { | ||
| nkLog(error: "Login poll response field for server for token \"\(token)\" is nil!") | ||
| continuation.resume(returning: nil) | ||
| return | ||
| } | ||
|
|
||
| guard let user = loginName else { | ||
| nkLog(error: "Login poll response field for user name for token \"\(token)\" is nil!") | ||
| continuation.resume(returning: nil) | ||
| return | ||
| } | ||
|
|
||
| guard let password = appPassword else { | ||
| nkLog(error: "Login poll response field for app password for token \"\(token)\" is nil!") | ||
| continuation.resume(returning: nil) | ||
| return | ||
| } | ||
|
|
||
| nkLog(debug: "Returning login poll response for \"\(user)\" on \"\(urlBase)\" for token \"\(token)\".") | ||
| continuation.resume(returning: NCLoginGrant(urlBase: urlBase, loginName: user, appPassword: password)) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,38 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // SPDX-FileCopyrightText: Nextcloud GmbH | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // SPDX-FileCopyrightText: 2025 Iva Horn | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // SPDX-FileCopyrightText: 2025 Milen Pivchev | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // SPDX-License-Identifier: GPL-3.0-or-later | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import Foundation | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| struct NCProviderLoginHandler { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| static func handle(callbackURL: URL) -> (urlBase: String, user: String, password: String)? { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let callbackURLString = callbackURL.absoluteString.lowercased() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let protocolPrefix = NCBrandOptions.shared.webLoginAutenticationProtocol.lowercased() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| guard callbackURLString.hasPrefix(protocolPrefix), callbackURLString.contains("login") else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+10
to
+15
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let callbackURLString = callbackURL.absoluteString.lowercased() | |
| let protocolPrefix = NCBrandOptions.shared.webLoginAutenticationProtocol.lowercased() | |
| guard callbackURLString.hasPrefix(protocolPrefix), callbackURLString.contains("login") else { | |
| return nil | |
| } | |
| let callbackURLString = callbackURL.absoluteString | |
| let protocolPrefix = NCBrandOptions.shared.webLoginAutenticationProtocol | |
| guard callbackURLString.range(of: protocolPrefix, options: [.caseInsensitive, .anchored]) != nil, | |
| callbackURLString.range(of: "login", options: .caseInsensitive) != nil else { | |
| return nil |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parsing logic checks if values contain the prefixes "server:", "user:", "password:" but this could match false positives. For example, if a password contains the string "user:", it could be incorrectly assigned to the user variable. Consider using hasPrefix instead of contains to be more precise, or use a more robust URL component parsing approach.
| if value.contains("server:") { server = value } | |
| if value.contains("user:") { user = value } | |
| if value.contains("password:") { password = value } | |
| if value.hasPrefix("/server:") { server = value } | |
| if value.hasPrefix("user:") { user = value } | |
| if value.hasPrefix("password:") { password = value } |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The replacement logic on line 32 only replaces "/server:" but the check on line 23 uses "server:" without the leading slash. This inconsistency could lead to the prefix not being properly removed if the format doesn't match expectations. Ensure the format is consistent or handle both cases.
| let serverClean = server.replacingOccurrences(of: "/server:", with: "") | |
| let serverClean = server | |
| .replacingOccurrences(of: "/server:", with: "") | |
| .replacingOccurrences(of: "server:", with: "") |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The login flow here relies on a custom URL scheme (configured via webLoginAutenticationProtocol, e.g. "nc://") that carries the server URL, username, and app password directly in the deep-link and parses them from callbackURL. On iOS, custom URL schemes are not exclusive to a single app, so any malicious app that registers the same scheme (e.g. nc) can intercept these callback URLs from Safari or ASWebAuthenticationSession and exfiltrate the cleartext credentials, leading to full account compromise. To harden this, avoid embedding credentials in the deep-link and switch to an HTTPS/universal-link based callback that returns only a short-lived authorization code or flow token, which the app then exchanges over TLS for credentials, ensuring only this app can complete the flow.
| let callbackURLString = callbackURL.absoluteString.lowercased() | |
| let protocolPrefix = NCBrandOptions.shared.webLoginAutenticationProtocol.lowercased() | |
| guard callbackURLString.hasPrefix(protocolPrefix), callbackURLString.contains("login") else { | |
| return nil | |
| } | |
| var server: String = "" | |
| var user: String = "" | |
| var password: String = "" | |
| let keyValue = callbackURL.path.components(separatedBy: "&") | |
| for value in keyValue { | |
| if value.contains("server:") { server = value } | |
| if value.contains("user:") { user = value } | |
| if value.contains("password:") { password = value } | |
| } | |
| guard !server.isEmpty, !user.isEmpty, !password.isEmpty else { | |
| return nil | |
| } | |
| let serverClean = server.replacingOccurrences(of: "/server:", with: "") | |
| let username = user.replacingOccurrences(of: "user:", with: "").replacingOccurrences(of: "+", with: " ") | |
| let passwordClean = password.replacingOccurrences(of: "password:", with: "") | |
| return (serverClean, username, passwordClean) | |
| // Expect an HTTPS / universal-link callback and avoid custom URL schemes | |
| let protocolPrefix = NCBrandOptions.shared.webLoginAutenticationProtocol.lowercased() | |
| let callbackURLString = callbackURL.absoluteString.lowercased() | |
| // Ensure the callback originates from the expected base URL and contains "login" | |
| guard callbackURLString.hasPrefix(protocolPrefix), callbackURLString.contains("login") else { | |
| return nil | |
| } | |
| // Parse query items instead of embedding credentials in the path | |
| guard let components = URLComponents(url: callbackURL, resolvingAgainstBaseURL: false) else { | |
| return nil | |
| } | |
| let queryItems = components.queryItems ?? [] | |
| func value(for name: String) -> String? { | |
| return queryItems.first(where: { $0.name == name })?.value | |
| } | |
| guard | |
| let server = value(for: "server"), | |
| let user = value(for: "user"), | |
| let code = value(for: "code") | |
| else { | |
| return nil | |
| } | |
| // The third tuple element is now a short-lived authorization code, not a long-lived password. | |
| return (server, user, code) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fallback to
loginURL.schemeon line 389 may not be appropriate. IfNCBrandOptions.shared.webLoginAutenticationProtocolcannot be parsed as a URL, using the loginURL scheme (likely "https") as the callback scheme will cause the authentication session to fail since the app won't receive callbacks on the https scheme. Consider logging this case or using a more explicit error handling approach.