Skip to content

Commit f92e4c1

Browse files
committed
fix: address Claude review findings
1 parent 025c28f commit f92e4c1

5 files changed

Lines changed: 84 additions & 3 deletions

File tree

Sources/CodingPlanAuth/Infrastructure/Server/LocalCallbackServer.swift

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,12 +162,25 @@ actor LocalCallbackServer {
162162
private static func resolveListenPort(_ port: UInt16) throws -> UInt16 {
163163
guard port == 0 else { return port }
164164

165+
// SwiftWebServer does not expose the assigned ephemeral port before the
166+
// redirect URI is built, so this is a best-effort reservation.
165167
let socketDescriptor = socket(AF_INET, SOCK_STREAM, 0)
166168
guard socketDescriptor >= 0 else {
167169
throw AuthError.callbackServerError("Could not create socket to reserve callback port")
168170
}
169171
defer { close(socketDescriptor) }
170172

173+
var reuseAddress = 1
174+
guard setsockopt(
175+
socketDescriptor,
176+
SOL_SOCKET,
177+
SO_REUSEADDR,
178+
&reuseAddress,
179+
socklen_t(MemoryLayout.size(ofValue: reuseAddress))
180+
) == 0 else {
181+
throw AuthError.callbackServerError("Could not configure callback port socket")
182+
}
183+
171184
var address = sockaddr_in()
172185
address.sin_family = sa_family_t(AF_INET)
173186
address.sin_addr.s_addr = INADDR_ANY

Sources/CodingPlanAuth/Presentation/AuthState.swift

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,10 @@ public final class AuthState {
5252
let creds = try await service.credentials(for: providerId)
5353
update(credentials: creds)
5454
} catch {
55-
currentCredentials = nil
56-
isAuthenticated = false
55+
if shouldClearAuthentication(after: error) {
56+
currentCredentials = nil
57+
isAuthenticated = false
58+
}
5759
setError(error)
5860
}
5961
}
@@ -107,4 +109,26 @@ public final class AuthState {
107109
self.lastError = .networkError(error.localizedDescription)
108110
}
109111
}
112+
113+
private func shouldClearAuthentication(after error: any Error) -> Bool {
114+
guard let authError = error as? AuthError else { return false }
115+
switch authError {
116+
case .tokenExchangeFailed(let statusCode, let message):
117+
if statusCode == 401 || statusCode == 403 {
118+
return true
119+
}
120+
let normalizedMessage = message.lowercased()
121+
return [
122+
"invalid_grant",
123+
"invalid grant",
124+
"invalid_token",
125+
"invalid token",
126+
"no refresh token",
127+
].contains { normalizedMessage.contains($0) }
128+
case .notAuthenticated, .unsupportedProvider:
129+
return true
130+
default:
131+
return false
132+
}
133+
}
110134
}

Sources/CodingPlanAuth/Presentation/BrowserAuthSession.swift

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,11 @@ public final class BrowserAuthSession: NSObject {
3838

3939
return try await withTaskCancellationHandler {
4040
try await withCheckedThrowingContinuation { continuation in
41+
if Task.isCancelled {
42+
continuation.resume(throwing: AuthError.cancelled)
43+
return
44+
}
45+
4146
self.continuation = continuation
4247
let session = ASWebAuthenticationSession(
4348
url: url,

Tests/CodingPlanAuthTests/AuthStateTests.swift

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,4 +42,43 @@ struct AuthStateTests {
4242
#expect(state.currentCredentials == nil)
4343
#expect(state.lastError == refreshError)
4444
}
45+
46+
@Test
47+
func checkStatusKeepsAuthenticatedStateWhenRefreshNetworkFails() async throws {
48+
let storage = MockTokenStorage()
49+
let service = AuthService(storage: storage)
50+
let provider = MockAuthProvider(id: "openai", name: "OpenAI")
51+
await service.register(provider)
52+
53+
try await storage.save(
54+
credentials: Credentials(
55+
accessToken: "valid",
56+
refreshToken: "rt",
57+
expiresAt: Date().addingTimeInterval(3600)
58+
),
59+
for: "openai"
60+
)
61+
62+
let state = AuthState(service: service, providerId: "openai")
63+
await state.checkStatus()
64+
#expect(state.isAuthenticated)
65+
#expect(state.currentCredentials?.accessToken == "valid")
66+
67+
let refreshError = AuthError.networkError("offline")
68+
await provider.setShouldThrowOnRefresh(refreshError)
69+
try await storage.save(
70+
credentials: Credentials(
71+
accessToken: "expired",
72+
refreshToken: "rt",
73+
expiresAt: Date().addingTimeInterval(-100)
74+
),
75+
for: "openai"
76+
)
77+
78+
await state.checkStatus()
79+
80+
#expect(state.isAuthenticated)
81+
#expect(state.currentCredentials?.accessToken == "valid")
82+
#expect(state.lastError == refreshError)
83+
}
4584
}

Tests/CodingPlanAuthTests/Infrastructure/LocalCallbackServerTests.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ struct LocalCallbackServerTests {
99
// We test the parsing logic indirectly by creating a server and
1010
// checking it would match the callback path.
1111
let server = LocalCallbackServer(port: 0, callbackPath: "/auth/callback")
12-
// Since NWListener-based servers cannot be unit-tested in the
12+
// Since SwiftWebServer-backed servers cannot be fully unit-tested in the
1313
// Swift Testing runner on macOS due to sandboxing constraints,
1414
// we verify the configuration is correct.
1515
#expect(server.callbackPath == "/auth/callback")

0 commit comments

Comments
 (0)