From 6443dbc6ea42ac0638abc4d5f07aaeccada1eb60 Mon Sep 17 00:00:00 2001 From: Francesco Paolo Severino <96546612+fpseverino@users.noreply.github.com> Date: Wed, 1 Jan 2025 23:10:30 +0100 Subject: [PATCH] Fix path components parsing (#116) * Adopt VaporTesting * Try fix for #115 * Make the linter happy * Use `pathSegments` also for `authURL` * Update GoogleJWT audience claim to use `accessTokenURL` * Remove redundant test * Update Imperial version in README to `2.0.0-beta.2` --- Package.swift | 6 +- README.md | 2 +- .../ImperialCore/FederatedServiceRouter.swift | 4 +- .../ImperialCore/String+pathSegments.swift | 18 ++++++ .../ImperialGoogle/JWT/GoogleJWTRouter.swift | 2 +- .../Standard/GoogleRouter.swift | 1 - Tests/ImperialTests/ImperialTests.swift | 61 +++++++++++-------- Tests/ImperialTests/ShopifyTests.swift | 6 +- Tests/ImperialTests/withApp.swift | 7 +-- 9 files changed, 63 insertions(+), 44 deletions(-) create mode 100644 Sources/ImperialCore/String+pathSegments.swift diff --git a/Package.swift b/Package.swift index 627aa432..67391756 100755 --- a/Package.swift +++ b/Package.swift @@ -42,8 +42,8 @@ let package = Package( ), ], dependencies: [ - .package(url: "https://github.com/vapor/vapor.git", from: "4.0.0"), - .package(url: "https://github.com/vapor/jwt-kit.git", from: "5.0.0"), + .package(url: "https://github.com/vapor/vapor.git", from: "4.110.1"), + .package(url: "https://github.com/vapor/jwt-kit.git", from: "5.1.1"), ], targets: [ .target( @@ -84,7 +84,7 @@ let package = Package( .target(name: "ImperialMicrosoft"), .target(name: "ImperialMixcloud"), .target(name: "ImperialShopify"), - .product(name: "XCTVapor", package: "vapor"), + .product(name: "VaporTesting", package: "vapor"), ], swiftSettings: swiftSettings ), diff --git a/README.md b/README.md index 1927876e..09b40434 100644 --- a/README.md +++ b/README.md @@ -25,7 +25,7 @@ Use the SPM string to easily include the dependendency in your `Package.swift` file ```swift -.package(url: "https://github.com/vapor-community/Imperial.git", from: "2.0.0-beta.1") +.package(url: "https://github.com/vapor-community/Imperial.git", from: "2.0.0-beta.2") ``` and then add the desired provider to your target's dependencies: diff --git a/Sources/ImperialCore/FederatedServiceRouter.swift b/Sources/ImperialCore/FederatedServiceRouter.swift index 439fa0b2..1335b21c 100644 --- a/Sources/ImperialCore/FederatedServiceRouter.swift +++ b/Sources/ImperialCore/FederatedServiceRouter.swift @@ -85,8 +85,8 @@ extension FederatedServiceRouter { package func configureRoutes( withAuthURL authURL: String, authenticateCallback: (@Sendable (Request) async throws -> Void)?, on router: some RoutesBuilder ) throws { - router.get(callbackURL.pathComponents, use: callback) - router.get(authURL.pathComponents) { req async throws -> Response in + router.get(callbackURL.pathSegments, use: callback) + router.get(authURL.pathSegments) { req async throws -> Response in let redirect: Response = req.redirect(to: try self.authURL(req)) guard let authenticateCallback else { return redirect diff --git a/Sources/ImperialCore/String+pathSegments.swift b/Sources/ImperialCore/String+pathSegments.swift new file mode 100644 index 00000000..e1675167 --- /dev/null +++ b/Sources/ImperialCore/String+pathSegments.swift @@ -0,0 +1,18 @@ +import Foundation +import RoutingKit + +extension String { + var pathSegments: [PathComponent] { + if let components = URL(string: self)?.pathComponents { + var pathSegments: [PathComponent] = [] + + for component in components where component != "/" { + pathSegments.append(PathComponent(stringLiteral: component)) + } + + return pathSegments + } else { + return self.pathComponents + } + } +} diff --git a/Sources/ImperialGoogle/JWT/GoogleJWTRouter.swift b/Sources/ImperialGoogle/JWT/GoogleJWTRouter.swift index 330f1c47..4f691dc5 100644 --- a/Sources/ImperialGoogle/JWT/GoogleJWTRouter.swift +++ b/Sources/ImperialGoogle/JWT/GoogleJWTRouter.swift @@ -53,7 +53,7 @@ struct GoogleJWTRouter: FederatedServiceRouter { let payload = GoogleJWTPayload( iss: IssuerClaim(value: self.tokens.clientID), scope: self.scope.joined(separator: " "), - aud: AudienceClaim(value: "https://www.googleapis.com/oauth2/v4/token"), + aud: AudienceClaim(value: self.accessTokenURL), iat: IssuedAtClaim(value: Date()), exp: ExpirationClaim(value: Date().addingTimeInterval(3600)) ) diff --git a/Sources/ImperialGoogle/Standard/GoogleRouter.swift b/Sources/ImperialGoogle/Standard/GoogleRouter.swift index 406c093a..dd38ef8c 100644 --- a/Sources/ImperialGoogle/Standard/GoogleRouter.swift +++ b/Sources/ImperialGoogle/Standard/GoogleRouter.swift @@ -49,5 +49,4 @@ struct GoogleRouter: FederatedServiceRouter { redirectURI: callbackURL ) } - } diff --git a/Tests/ImperialTests/ImperialTests.swift b/Tests/ImperialTests/ImperialTests.swift index b77cb5fa..c352b6fd 100644 --- a/Tests/ImperialTests/ImperialTests.swift +++ b/Tests/ImperialTests/ImperialTests.swift @@ -11,7 +11,7 @@ import ImperialKeycloak import ImperialMicrosoft import ImperialMixcloud import Testing -import XCTVapor +import VaporTesting @testable import ImperialCore @@ -21,14 +21,14 @@ struct ImperialTests { func auth0Route() async throws { try await withApp(service: Auth0.self) { app in try await app.test( - .GET, apiAuthURL, + .GET, authURL, afterResponse: { res async throws in #expect(res.status == .seeOther) } ) try await app.test( - .GET, "\(apiCallbackURL)?code=123", + .GET, "\(callbackURL)?code=123", afterResponse: { res async throws in // A real Auth0 domain is needed to test this route #expect(res.status == .internalServerError) @@ -41,14 +41,14 @@ struct ImperialTests { func deviantArtRoute() async throws { try await withApp(service: DeviantArt.self) { app in try await app.test( - .GET, apiAuthURL, + .GET, authURL, afterResponse: { res async throws in #expect(res.status == .seeOther) } ) try await app.test( - .GET, "\(apiCallbackURL)?code=123", + .GET, "\(callbackURL)?code=123", afterResponse: { res async throws in // TODO: test this route #expect(res.status != .notFound) @@ -61,14 +61,14 @@ struct ImperialTests { func discordRoute() async throws { try await withApp(service: Discord.self) { app in try await app.test( - .GET, apiAuthURL, + .GET, authURL, afterResponse: { res async throws in #expect(res.status == .seeOther) } ) try await app.test( - .GET, "\(apiCallbackURL)?code=123", + .GET, "\(callbackURL)?code=123", afterResponse: { res async throws in // Discord returns a 400 Bad Request error when the code is invalid with a JSON error message #expect(res.status == .badRequest) @@ -81,14 +81,14 @@ struct ImperialTests { func dropboxRoute() async throws { try await withApp(service: Dropbox.self) { app in try await app.test( - .GET, apiAuthURL, + .GET, authURL, afterResponse: { res async throws in #expect(res.status == .seeOther) } ) try await app.test( - .GET, "\(apiCallbackURL)?code=123", + .GET, "\(callbackURL)?code=123", afterResponse: { res async throws in // Dropbox returns a 400 Bad Request error when the code is invalid with a JSON error message #expect(res.status == .badRequest) @@ -101,14 +101,14 @@ struct ImperialTests { func facebookRoute() async throws { try await withApp(service: Facebook.self) { app in try await app.test( - .GET, apiAuthURL, + .GET, authURL, afterResponse: { res async throws in #expect(res.status == .seeOther) } ) try await app.test( - .GET, "\(apiCallbackURL)?code=123", + .GET, "\(callbackURL)?code=123", afterResponse: { res async throws in // The response is an JS, signaling an error with `redirect_uri` #expect(res.status == .unsupportedMediaType) @@ -121,14 +121,14 @@ struct ImperialTests { func githubRoute() async throws { try await withApp(service: GitHub.self) { app in try await app.test( - .GET, apiAuthURL, + .GET, authURL, afterResponse: { res async throws in #expect(res.status == .seeOther) } ) try await app.test( - .GET, "\(apiCallbackURL)?code=123", + .GET, "\(callbackURL)?code=123", afterResponse: { res async throws in // The response is an HTML page likely signaling an error #expect(res.status == .unsupportedMediaType) @@ -141,14 +141,14 @@ struct ImperialTests { func gitlabRoute() async throws { try await withApp(service: Gitlab.self) { app in try await app.test( - .GET, apiAuthURL, + .GET, authURL, afterResponse: { res async throws in #expect(res.status == .seeOther) } ) try await app.test( - .GET, "\(apiCallbackURL)?code=123", + .GET, "\(callbackURL)?code=123", afterResponse: { res async throws in // Gitlab returns a 400 Bad Request error when the code is invalid with a JSON error message #expect(res.status == .badRequest) @@ -161,14 +161,14 @@ struct ImperialTests { func googleRoute() async throws { try await withApp(service: Google.self) { app in try await app.test( - .GET, apiAuthURL, + .GET, authURL, afterResponse: { res async throws in #expect(res.status == .seeOther) } ) try await app.test( - .GET, "\(apiCallbackURL)?code=123", + .GET, "\(callbackURL)?code=123", afterResponse: { res async throws in // Google returns a 400 Bad Request error when the code is invalid with a JSON error message #expect(res.status == .badRequest) @@ -181,14 +181,14 @@ struct ImperialTests { func googleJWTRoute() async throws { try await withApp(service: GoogleJWT.self) { app in try await app.test( - .GET, apiAuthURL, + .GET, authURL, afterResponse: { res async throws in #expect(res.status == .seeOther) } ) try await app.test( - .GET, apiCallbackURL, + .GET, callbackURL, afterResponse: { res async throws in // We don't have a valid key to sign the JWT #expect(res.status == .internalServerError) @@ -201,14 +201,14 @@ struct ImperialTests { func imgurRoute() async throws { try await withApp(service: Imgur.self) { app in try await app.test( - .GET, apiAuthURL, + .GET, authURL, afterResponse: { res async throws in #expect(res.status == .seeOther) } ) try await app.test( - .GET, "\(apiCallbackURL)?code=123", + .GET, "\(callbackURL)?code=123", afterResponse: { res async throws in // TODO: test this route #expect(res.status != .notFound) @@ -221,14 +221,14 @@ struct ImperialTests { func keycloakRoute() async throws { try await withApp(service: Keycloak.self) { app in try await app.test( - .GET, apiAuthURL, + .GET, authURL, afterResponse: { res async throws in #expect(res.status == .seeOther) } ) try await app.test( - .GET, "\(apiCallbackURL)?code=123", + .GET, "\(callbackURL)?code=123", afterResponse: { res async throws in // The post request fails #expect(res.status == .internalServerError) @@ -241,14 +241,14 @@ struct ImperialTests { func microsoftRoute() async throws { try await withApp(service: Microsoft.self) { app in try await app.test( - .GET, apiAuthURL, + .GET, authURL, afterResponse: { res async throws in #expect(res.status == .seeOther) } ) try await app.test( - .GET, "\(apiCallbackURL)?code=123", + .GET, "\(callbackURL)?code=123", afterResponse: { res async throws in // Microsoft returns a 400 Bad Request, signaling an error with `redirect_uri` #expect(res.status == .badRequest) @@ -261,14 +261,14 @@ struct ImperialTests { func mixcloudRoute() async throws { try await withApp(service: Mixcloud.self) { app in try await app.test( - .GET, apiAuthURL, + .GET, authURL, afterResponse: { res async throws in #expect(res.status == .seeOther) } ) try await app.test( - .GET, "\(apiCallbackURL)?code=123", + .GET, "\(callbackURL)?code=123", afterResponse: { res async throws in // TODO: test this route #expect(res.status != .notFound) @@ -277,6 +277,13 @@ struct ImperialTests { } } + @Test("Path Segments") + func pathSegments() { + let url = "https://hello.world.example.com:8080/api/oauth/service-auth-complete" + #expect(url.pathSegments == ["api", "oauth", "service-auth-complete"]) + #expect(url.pathSegments.string == "api/oauth/service-auth-complete") + } + @Test("ImperialError") func imperialError() { let variable = "test" diff --git a/Tests/ImperialTests/ShopifyTests.swift b/Tests/ImperialTests/ShopifyTests.swift index dfbc3783..f26ab057 100644 --- a/Tests/ImperialTests/ShopifyTests.swift +++ b/Tests/ImperialTests/ShopifyTests.swift @@ -1,6 +1,6 @@ import Foundation import Testing -import XCTVapor +import VaporTesting @testable import ImperialShopify @@ -9,7 +9,7 @@ struct ShopifyTests { @Test("Shopify Route") func shopifyRoute() async throws { try await withApp(service: Shopify.self) { app in try await app.test( - .GET, "\(apiAuthURL)?shop=some-shop.myshopify.com", + .GET, "\(authURL)?shop=some-shop.myshopify.com", afterResponse: { res async throws in #expect(res.status == .seeOther) } @@ -17,7 +17,7 @@ struct ShopifyTests { try await app.test( .GET, - "\(apiCallbackURL)?" + "\(callbackURL)?" + "code=0907a61c0c8d55e99db179b68161bc00&" + "hmac=700e2dadb827fcc8609e9d5ce208b2e9cdaab9df07390d2cbca10d7c328fc4bf&" + "shop=some-shop.myshopify.com&" diff --git a/Tests/ImperialTests/withApp.swift b/Tests/ImperialTests/withApp.swift index 48a48693..472f281e 100644 --- a/Tests/ImperialTests/withApp.swift +++ b/Tests/ImperialTests/withApp.swift @@ -4,9 +4,6 @@ import Vapor let authURL = "service" let callbackURL = "service-auth-complete" -let apiGroup = "api" -let apiAuthURL = "/\(apiGroup)/\(authURL)" -let apiCallbackURL = "/\(apiGroup)/\(callbackURL)" func withApp( service: OAuthProvider.Type, @@ -16,9 +13,7 @@ func withApp( try #require(isLoggingConfigured) do { app.middleware.use(app.sessions.middleware) - // Test for https://github.com/vapor-community/Imperial/issues/43 - let grouped = app.grouped(PathComponent(stringLiteral: apiGroup)) - try grouped.oAuth(from: service.self, authenticate: authURL, callback: callbackURL, redirect: "/") + try app.oAuth(from: service.self, authenticate: authURL, callback: callbackURL, redirect: "/") try await test(app) } catch { try await app.asyncShutdown()