From e27b403b69788cb2518bc18602f37d0a5de5fc83 Mon Sep 17 00:00:00 2001 From: Doug Date: Fri, 5 Aug 2022 17:38:52 +0100 Subject: [PATCH 1/2] Use the default homeserver when starting a new auth flow. And override this when a provisioning link has been set. --- .../AuthenticationCoordinator.swift | 4 +-- .../MatrixSDK/AuthenticationService.swift | 23 +++++++++++-- .../AuthenticationServiceTests.swift | 33 +++++++++++++++++++ changelog.d/6489.bugfix | 1 + 4 files changed, 56 insertions(+), 5 deletions(-) create mode 100644 changelog.d/6489.bugfix diff --git a/Riot/Modules/Authentication/AuthenticationCoordinator.swift b/Riot/Modules/Authentication/AuthenticationCoordinator.swift index f34f508391..820bbe1762 100644 --- a/Riot/Modules/Authentication/AuthenticationCoordinator.swift +++ b/Riot/Modules/Authentication/AuthenticationCoordinator.swift @@ -131,8 +131,8 @@ final class AuthenticationCoordinator: NSObject, AuthenticationCoordinatorProtoc let flow: AuthenticationFlow = initialScreen == .login ? .login : .register do { - let homeserverAddress = authenticationService.state.homeserver.addressFromUser ?? authenticationService.state.homeserver.address - try await authenticationService.startFlow(flow, for: homeserverAddress) + // Start the flow using the default server (or a provisioning link if set). + try await authenticationService.startFlow(flow) } catch { MXLog.error("[AuthenticationCoordinator] start: Failed to start") displayError(message: error.localizedDescription) diff --git a/RiotSwiftUI/Modules/Authentication/Common/Service/MatrixSDK/AuthenticationService.swift b/RiotSwiftUI/Modules/Authentication/Common/Service/MatrixSDK/AuthenticationService.swift index 0ef46245d8..eff30724ca 100644 --- a/RiotSwiftUI/Modules/Authentication/Common/Service/MatrixSDK/AuthenticationService.swift +++ b/RiotSwiftUI/Modules/Authentication/Common/Service/MatrixSDK/AuthenticationService.swift @@ -60,6 +60,8 @@ class AuthenticationService: NSObject { private(set) var loginWizard: LoginWizard? /// The current registration wizard or `nil` if `startFlow` hasn't been called for `.registration`. private(set) var registrationWizard: RegistrationWizard? + /// The provisioning link the service is currently configured with. + private(set) var provisioningLink: UniversalLink? /// The authentication service's delegate. weak var delegate: AuthenticationServiceDelegate? @@ -110,6 +112,9 @@ class AuthenticationService: NSObject { state = AuthenticationState(flow: flow, homeserverAddress: hsUrl ?? BuildSettings.serverConfigDefaultHomeserverUrlString, identityServer: isUrl ?? BuildSettings.serverConfigDefaultIdentityServerUrlString) + + // store the link to override the default homeserver address. + provisioningLink = universalLink delegate?.authenticationService(self, didUpdateStateWithLink: universalLink) } else { // logged in @@ -133,8 +138,15 @@ class AuthenticationService: NSObject { MXKAccountManager.shared().activeAccounts?.first?.mxSession } - func startFlow(_ flow: AuthenticationFlow, for homeserverAddress: String) async throws { - var (client, homeserver) = try await loginFlow(for: homeserverAddress) + /// Set up the service to start a new authentication flow. + /// - Parameters: + /// - flow: The flow to be started (login or register). + /// - homeserverAddress: The homeserver to start the flow for, or `nil` to use the default. + /// If a provisioning link has been set, it will override the default homeserver when passing `nil`. + func startFlow(_ flow: AuthenticationFlow, for homeserverAddress: String? = nil) async throws { + let address = homeserverAddress ?? provisioningLink?.homeserverUrl ?? BuildSettings.serverConfigDefaultHomeserverUrlString + + var (client, homeserver) = try await loginFlow(for: address) let loginWizard = LoginWizard(client: client, sessionCreator: sessionCreator) self.loginWizard = loginWizard @@ -179,8 +191,13 @@ class AuthenticationService: NSObject { loginWizard = nil registrationWizard = nil softLogoutCredentials = nil + + if useDefaultServer { + provisioningLink = nil + } - // The previously used homeserver is re-used as `startFlow` will be called again a replace it anyway. + // This address will be replaced when `startFlow` is called, but for + // completeness revert to the default homeserver if requested anyway. let address = useDefaultServer ? BuildSettings.serverConfigDefaultHomeserverUrlString : state.homeserver.addressFromUser ?? state.homeserver.address let identityServer = state.identityServer self.state = AuthenticationState(flow: .login, diff --git a/RiotTests/Modules/Authentication/AuthenticationServiceTests.swift b/RiotTests/Modules/Authentication/AuthenticationServiceTests.swift index ef983c7920..697573aed4 100644 --- a/RiotTests/Modules/Authentication/AuthenticationServiceTests.swift +++ b/RiotTests/Modules/Authentication/AuthenticationServiceTests.swift @@ -95,6 +95,39 @@ import XCTest "The address should reset to the value configured in the build settings.") } + func testProvisioningLink() async throws { + // Given a service that has begun login using a provisioning link. + let homeserverURL = "https://example.com" + let provisioningLink = URL(string: "app.element.io/register/?hs_url=\(homeserverURL)")! + let universalLink = UniversalLink(url: provisioningLink) + service.handleServerProvisioningLink(universalLink) + + try await service.startFlow(.login) + XCTAssertEqual(universalLink.homeserverUrl, homeserverURL) + XCTAssertNotNil(service.provisioningLink, "The provisioning link should be stored in the service.") + XCTAssertEqual(service.provisioningLink?.homeserverUrl, homeserverURL, "The provisioning link's homeserver should not change.") + XCTAssertEqual(service.state.homeserver.address, "https://matrix.example.com", "The actual homeserver address should be discovered.") + XCTAssertEqual(service.state.homeserver.addressFromUser, homeserverURL, "The address from the provisioning link should be stored.") + + // When resetting the service. + service.reset() + + // Then the link should be remembered. + XCTAssertNotNil(service.provisioningLink, "The provisioning link should not be cleared.") + XCTAssertEqual(service.provisioningLink?.homeserverUrl, homeserverURL, "The provisioning link's homeserver should not change.") + XCTAssertEqual(service.state.homeserver.address, homeserverURL, "The address from the provisioning link should be stored.") + XCTAssertNil(service.state.homeserver.addressFromUser, "There shouldn't be an address from the user after resetting the service.") + + // When resetting the service back to the default server. + service.reset(useDefaultServer: true) + + // Then the link should be forgotten. + XCTAssertNil(service.provisioningLink, "The provisioning link should be forgotten after resetting back to the default server.") + XCTAssertNil(service.state.homeserver.addressFromUser, "There shouldn't be an address from the user after resetting the service.") + XCTAssertEqual(service.state.homeserver.address, BuildSettings.serverConfigDefaultHomeserverUrlString, + "The address should reset to the value configured in the build settings.") + } + func testHomeserverState() async throws { // Given a service that has begun login for one homeserver. try await service.startFlow(.login, for: "https://example.com") diff --git a/changelog.d/6489.bugfix b/changelog.d/6489.bugfix new file mode 100644 index 0000000000..b4b416e050 --- /dev/null +++ b/changelog.d/6489.bugfix @@ -0,0 +1 @@ +Authentication: Always start a new authentication flow with the default homeserver (or the provisioning link if set). From c5105683462089a58fcd77c584b6e50f0dc9242f Mon Sep 17 00:00:00 2001 From: Doug Date: Fri, 5 Aug 2022 18:05:17 +0100 Subject: [PATCH 2/2] Replace error alert with server selection screen in startFlow. --- .../AuthenticationCoordinator.swift | 82 +++++++++---------- 1 file changed, 38 insertions(+), 44 deletions(-) diff --git a/Riot/Modules/Authentication/AuthenticationCoordinator.swift b/Riot/Modules/Authentication/AuthenticationCoordinator.swift index 820bbe1762..19316a4a13 100644 --- a/Riot/Modules/Authentication/AuthenticationCoordinator.swift +++ b/Riot/Modules/Authentication/AuthenticationCoordinator.swift @@ -134,8 +134,8 @@ final class AuthenticationCoordinator: NSObject, AuthenticationCoordinatorProtoc // Start the flow using the default server (or a provisioning link if set). try await authenticationService.startFlow(flow) } catch { - MXLog.error("[AuthenticationCoordinator] start: Failed to start") - displayError(message: error.localizedDescription) + MXLog.error("[AuthenticationCoordinator] start: Failed to start, showing server selection.") + showServerSelectionScreen(for: flow) return } @@ -155,6 +155,42 @@ final class AuthenticationCoordinator: NSObject, AuthenticationCoordinatorProtoc } } + /// Pushes the server selection screen into the flow (other screens may also present it modally later). + @MainActor private func showServerSelectionScreen(for flow: AuthenticationFlow) { + MXLog.debug("[AuthenticationCoordinator] showServerSelectionScreen") + let parameters = AuthenticationServerSelectionCoordinatorParameters(authenticationService: authenticationService, + flow: flow, + hasModalPresentation: false) + let coordinator = AuthenticationServerSelectionCoordinator(parameters: parameters) + coordinator.callback = { [weak self, weak coordinator] result in + guard let self = self, let coordinator = coordinator else { return } + self.serverSelectionCoordinator(coordinator, didCompleteWith: result, for: flow) + } + + coordinator.start() + add(childCoordinator: coordinator) + + navigationRouter.push(coordinator, animated: true) { [weak self] in + self?.remove(childCoordinator: coordinator) + } + } + + /// Shows the next screen in the flow after the server selection screen. + @MainActor private func serverSelectionCoordinator(_ coordinator: AuthenticationServerSelectionCoordinator, + didCompleteWith result: AuthenticationServerSelectionCoordinatorResult, + for flow: AuthenticationFlow) { + switch result { + case .updated: + if flow == .register { + showRegistrationScreen() + } else { + showLoginScreen() + } + case .dismiss: + MXLog.failure("[AuthenticationCoordinator] AuthenticationServerSelectionScreen is requesting dismiss when part of a stack.") + } + } + /// Presents an alert on top of the navigation router with the supplied error message. @MainActor private func displayError(message: String) { let alert = UIAlertController(title: VectorL10n.error, message: message, preferredStyle: .alert) @@ -307,48 +343,6 @@ final class AuthenticationCoordinator: NSObject, AuthenticationCoordinatorProtoc // MARK: - Registration - #warning("Unused.") - /// Pushes the server selection screen into the flow (other screens may also present it modally later). - @MainActor private func showServerSelectionScreen() { - MXLog.debug("[AuthenticationCoordinator] showServerSelectionScreen") - let parameters = AuthenticationServerSelectionCoordinatorParameters(authenticationService: authenticationService, - flow: .register, - hasModalPresentation: false) - let coordinator = AuthenticationServerSelectionCoordinator(parameters: parameters) - coordinator.callback = { [weak self, weak coordinator] result in - guard let self = self, let coordinator = coordinator else { return } - self.serverSelectionCoordinator(coordinator, didCompleteWith: result) - } - - coordinator.start() - add(childCoordinator: coordinator) - - if navigationRouter.modules.isEmpty { - navigationRouter.setRootModule(coordinator) { [weak self] in - self?.remove(childCoordinator: coordinator) - } - } else { - navigationRouter.push(coordinator, animated: true) { [weak self] in - self?.remove(childCoordinator: coordinator) - } - } - } - - /// Shows the next screen in the flow after the server selection screen. - @MainActor private func serverSelectionCoordinator(_ coordinator: AuthenticationServerSelectionCoordinator, - didCompleteWith result: AuthenticationServerSelectionCoordinatorResult) { - switch result { - case .updated: - if authenticationService.state.homeserver.needsRegistrationFallback { - showFallback(for: .register) - } else { - showRegistrationScreen() - } - case .dismiss: - MXLog.failure("[AuthenticationCoordinator] AuthenticationServerSelectionScreen is requesting dismiss when part of a stack.") - } - } - /// Shows the registration screen. @MainActor private func showRegistrationScreen() { MXLog.debug("[AuthenticationCoordinator] showRegistrationScreen")