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.

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

unregister push on logout #1309

Merged
merged 3 commits into from
May 15, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ public class NotifyClient {
try await identityClient.unregister(account: account)
notifyWatcherAgreementKeysProvider.removeAgreement(account: account)
try notifyStorage.clearDatabase(account: account)
Task { try await pushClient.unregister()}
Copy link
Member

Choose a reason for hiding this comment

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

What if the unregister fails (e.g. no internet?) Will this flow be tried again or will the push registration still be present?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, I will make it throw

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chris13524 what code push api returns for already unregistered client on /unregister ?

Copy link
Member

Choose a reason for hiding this comment

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

It gives the same response code regardless if the client was already unregistered or not: 200

Copy link
Member

Choose a reason for hiding this comment

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

I will make it throw

I still think there is an issue. If it throws, what does caller do with that? If simply display an error message, the question to me is when will it do the unregister again?

Looks to me that you are clearing the notify storage database (and other things) before you unregister the push client. So this would mean if the way the unregister takes place is based on the notify storage, then the unregister won't happen again. I'm assuming that logout should be an atomic operation to prevent the receiving of push notifications it should not.

notifyAccountProvider.logout()
subscriptionWatcher.stop()
}
Expand Down
10 changes: 9 additions & 1 deletion Sources/WalletConnectPush/PushClient.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,29 @@ import Combine

public class PushClient: PushClientProtocol {
private let registerService: PushRegisterService
private let unregisterService: UnregisterService
private let logger: ConsoleLogging

public var logsPublisher: AnyPublisher<Log, Never> {
return logger.logsPublisher
}

init(registerService: PushRegisterService, logger: ConsoleLogging) {
init(registerService: PushRegisterService,
logger: ConsoleLogging,
unregisterService: UnregisterService) {
self.registerService = registerService
self.unregisterService = unregisterService
self.logger = logger
}

public func register(deviceToken: Data, enableEncrypted: Bool = false) async throws {
try await registerService.register(deviceToken: deviceToken, alwaysRaw: enableEncrypted)
}

public func unregister() async throws {
try await unregisterService.unregister()
}

#if DEBUG
public func register(deviceToken: String) async throws {
try await registerService.register(deviceToken: deviceToken, alwaysRaw: true)
Expand Down
3 changes: 2 additions & 1 deletion Sources/WalletConnectPush/PushClientFactory.swift
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ public struct PushClientFactory {
let pushAuthenticator = PushAuthenticator(clientIdStorage: clientIdStorage, pushHost: pushHost)

let registerService = PushRegisterService(httpClient: httpClient, projectId: projectId, clientIdStorage: clientIdStorage, pushAuthenticator: pushAuthenticator, logger: logger, environment: environment)
let unregisterService = UnregisterService(httpClient: httpClient, projectId: projectId, clientIdStorage: clientIdStorage, pushAuthenticator: pushAuthenticator, logger: logger, pushHost: pushHost, environment: environment)

return PushClient(registerService: registerService, logger: logger)
return PushClient(registerService: registerService, logger: logger, unregisterService: unregisterService)
}
}
55 changes: 55 additions & 0 deletions Sources/WalletConnectPush/UnregisterService.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import Foundation

actor UnregisterService {
private let httpClient: HTTPClient
private let projectId: String
private let logger: ConsoleLogging
private let environment: APNSEnvironment
private let pushAuthenticator: PushAuthenticating
private let clientIdStorage: ClientIdStoring
private let pushHost: String

init(httpClient: HTTPClient,
projectId: String,
clientIdStorage: ClientIdStoring,
pushAuthenticator: PushAuthenticating,
logger: ConsoleLogging,
pushHost: String,
environment: APNSEnvironment) {
self.httpClient = httpClient
self.clientIdStorage = clientIdStorage
self.pushAuthenticator = pushAuthenticator
self.projectId = projectId
self.logger = logger
self.pushHost = pushHost
self.environment = environment
}

func unregister() async throws {
let pushAuthToken = try pushAuthenticator.createAuthToken()
let clientId = try clientIdStorage.getClientId()

guard let url = URL(string: "https://\(pushHost)/\(projectId)/clients/\(clientId)") else {
logger.error("Invalid URL")
return
}

var request = URLRequest(url: url)
request.httpMethod = "DELETE"
request.addValue("\(pushAuthToken)", forHTTPHeaderField: "Authorization")

do {
let (_, response) = try await URLSession.shared.data(for: request)

guard let httpResponse = response as? HTTPURLResponse, httpResponse.statusCode == 200 else {
Copy link
Member

@chris13524 chris13524 Feb 22, 2024

Choose a reason for hiding this comment

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

200

Instead of checking for a specific status code, can you check if it is any 2xx status code? We may want e.g. change to 204 (no content) and all 2xx codes mean success

logger.error("Failed to unregister from Push Server")
throw NSError(domain: "UnregisterService", code: 0, userInfo: [NSLocalizedDescriptionKey: "Failed to unregister"])
}

logger.debug("Successfully unregistered from Push Server")
} catch {
logger.error("Push Server unregistration error: \(error.localizedDescription)")
throw error
}
}
}
6 changes: 3 additions & 3 deletions Sources/WalletConnectSign/RejectionReason.swift
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ public enum RejectionReason {
case unsupportedChains
case unsupportedMethods
case unsupportedAccounts
case upsupportedEvents
case unsupportedEvents
}

internal extension RejectionReason {
Expand All @@ -18,7 +18,7 @@ internal extension RejectionReason {
return SignReasonCode.unsupportedChains
case .unsupportedMethods:
return SignReasonCode.userRejectedMethods
case .upsupportedEvents:
case .unsupportedEvents:
return SignReasonCode.userRejectedEvents
case .unsupportedAccounts:
return SignReasonCode.unsupportedAccounts
Expand All @@ -36,7 +36,7 @@ public extension RejectionReason {
case .requiredMethodsNotSatisfied:
self = .unsupportedMethods
case .requiredEventsNotSatisfied:
self = .upsupportedEvents
self = .unsupportedEvents
case .emptySessionNamespacesForbidden:
self = .unsupportedAccounts
}
Expand Down
Loading