From 21f290133765f402b21aed008f38875a2aa66247 Mon Sep 17 00:00:00 2001 From: Javier Cicchelli Date: Sun, 12 Oct 2025 21:26:55 +0200 Subject: [PATCH] Improved the AuthMiddleware type in the library target by adding input validation and by generating the authentication information once. --- .../Extensions/String+Constants.swift | 9 +- .../Public/Enumerations/AuthTransport.swift | 2 +- .../Public/Middlewares/AuthMiddleware.swift | 127 +++++---- .../Middlewares/AuthMiddlewareTests.swift | 254 +++++++++++++++++- 4 files changed, 323 insertions(+), 69 deletions(-) diff --git a/Sources/DiscogsService/Internal/Extensions/String+Constants.swift b/Sources/DiscogsService/Internal/Extensions/String+Constants.swift index 20e7bde4c..b9682eea5 100644 --- a/Sources/DiscogsService/Internal/Extensions/String+Constants.swift +++ b/Sources/DiscogsService/Internal/Extensions/String+Constants.swift @@ -26,14 +26,7 @@ extension String { static let token = "token" } /// A namespaces assigned for the formats of string values. - enum Format { - /// A format for the consumer authentication header. - static let authConsumer = "Discogs \(String.Parameter.key)=%@, \(String.Parameter.secret)=%@" - /// A format for the user authentication header. - static let authUser = "Discogs \(String.Parameter.token)=%@" - /// A format for the user agent header. - static let userAgent = "%@/%@ +%@" - } + enum Format {} /// A namespaces assigned for the formats of regular expression patterns. enum Pattern {} diff --git a/Sources/DiscogsService/Public/Enumerations/AuthTransport.swift b/Sources/DiscogsService/Public/Enumerations/AuthTransport.swift index 5e3e67b25..05f4e7328 100644 --- a/Sources/DiscogsService/Public/Enumerations/AuthTransport.swift +++ b/Sources/DiscogsService/Public/Enumerations/AuthTransport.swift @@ -13,7 +13,7 @@ // ===----------------------------------------------------------------------=== /// A representation of the available transport options to send credentials in authenticated requests. -public enum AuthTransport: Sendable { +public enum AuthTransport: CaseIterable, Sendable { /// Authentication credential are sent in a request as an `Authentication` header. /// /// This means that the header will be added to any existing header in a request, like this: diff --git a/Sources/DiscogsService/Public/Middlewares/AuthMiddleware.swift b/Sources/DiscogsService/Public/Middlewares/AuthMiddleware.swift index 4b05a7273..bc805abb5 100644 --- a/Sources/DiscogsService/Public/Middlewares/AuthMiddleware.swift +++ b/Sources/DiscogsService/Public/Middlewares/AuthMiddleware.swift @@ -19,23 +19,24 @@ import protocol OpenAPIRuntime.ClientMiddleware import struct Foundation.URL import struct Foundation.URLComponents import struct Foundation.URLQueryItem +import struct HTTPTypes.HTTPField import struct HTTPTypes.HTTPFields import struct HTTPTypes.HTTPRequest import struct HTTPTypes.HTTPResponse -/// A middleware that attaches any defined authentication credentials into the requests for the service. +/// A middleware that attaches any defined authentication credentials into the requests to the service. /// /// Please refer to the [Discogs documentation](https://www.discogs.com/developers#page:authentication) for further information. public struct AuthMiddleware { // MARK: Properties - /// A representation of an authentication method to use to authenticate requests. - private let method: AuthMethod - - /// A representation of a transport option to send credentials in requests. - private let transport: AuthTransport + /// A header field that contains the authentication information. + let authField: HTTPField? + /// A list of query items that contains the authentication information. + let authItems: [URLQueryItem]? + // MARK: Initializers /// Initializes this middleware. @@ -45,9 +46,59 @@ public struct AuthMiddleware { public init( method: AuthMethod = .none, transport: AuthTransport - ) { - self.method = method - self.transport = transport + ) throws { + switch method { + case let .consumer(key, secret): + let validateKey = ValidateInputUseCase(rules: .notNil, .notEmpty, .secure(.consumerKey)) + let validateSecret = ValidateInputUseCase(rules: .notNil, .notEmpty, .secure(.consumerSecret)) + + try validateKey(key) + try validateSecret(secret) + + self.authField = switch transport { + case .onQuery: nil + case .onHeader: .init( + name: .authorization, + value: .init(format: .Format.authConsumer, key, secret) + )} + + self.authItems = switch transport { + case .onHeader: nil + case .onQuery: [ + .init(name: .Parameter.key, value: key), + .init(name: .Parameter.secret, value: secret) + ]} + + case let .user(token): + let validateToken = ValidateInputUseCase(rules: .notNil, .notEmpty, .secure(.userToken)) + + try validateToken(token) + + self.authField = switch transport { + case .onQuery: nil + case .onHeader: .init( + name: .authorization, + value: .init(format: .Format.authUser, token) + )} + + self.authItems = switch transport { + case .onHeader: nil + case .onQuery: [ + .init(name: .Parameter.token, value: token) + ] + } + + case .none: + self.authField = nil + self.authItems = nil + } + } + + // MARK: Computed + + /// A flag that indicates whether the middleware should authenticate the intercepted request or not. + var shouldAuthenticate: Bool { + authField != nil || authItems != nil } } @@ -65,29 +116,17 @@ extension AuthMiddleware: ClientMiddleware { operationID: String, next: @Sendable (HTTPRequest, HTTPBody?, URL) async throws -> (HTTPResponse, HTTPBody?) ) async throws -> (HTTPResponse, HTTPBody?) { - guard method != .none else { + guard shouldAuthenticate else { return try await next(request, body, baseURL) } - - let headerFields = if transport == .onHeader { - authenticateHeader(request.headerFields) - } else { - request.headerFields - } - - let path = if transport == .onQuery { - authenticatePath(request.path) - } else { - request.path - } return try await next( .init( method: request.method, scheme: request.scheme, authority: request.authority, - path: path, - headerFields: headerFields + path: authenticatePath(request.path), + headerFields: authenticateHeader(request.headerFields) ), body, baseURL @@ -104,21 +143,14 @@ private extension AuthMiddleware { /// Adds an authorization header to the existing header fields. /// - Parameter fields: A set of header fields to update. - /// - Returns: An updated set of header fields. + /// - Returns: An updated set of header fields including the authorization header. func authenticateHeader(_ fields: HTTPFields) -> HTTPFields { var fields = fields - - let authorization: String = switch method { - case let .consumer(key, secret): .init(format: .Format.authConsumer, key, secret) - case let .user(token): .init(format: .Format.authUser, token) - default: .empty + + if let authField { + fields.append(authField) } - - fields.append(.init( - name: .authorization, - value: authorization - )) - + return fields } @@ -127,23 +159,13 @@ private extension AuthMiddleware { /// - Returns: An updated request path including the authentication parameters. func authenticatePath(_ path: String?) -> String? { guard + let authItems, let path, var urlComponents = URLComponents(string: path) else { return path } - - let authItems: [URLQueryItem] = switch method { - case let .consumer(key, secret): [ - .init(name: .Parameter.key, value: key), - .init(name: .Parameter.secret, value: secret) - ] - case let .user(token): [ - .init(name: .Parameter.token, value: token) - ] - default: [] - } - + var queryItems = urlComponents.queryItems ?? [] queryItems.append(contentsOf: authItems) @@ -158,3 +180,12 @@ private extension AuthMiddleware { } } + +// MARK: - Constants + +private extension String.Format { + /// A format for the consumer authentication header. + static let authConsumer = "Discogs \(String.Parameter.key)=%@, \(String.Parameter.secret)=%@" + /// A format for the user authentication header. + static let authUser = "Discogs \(String.Parameter.token)=%@" +} diff --git a/Tests/DiscogsService/Cases/Public/Middlewares/AuthMiddlewareTests.swift b/Tests/DiscogsService/Cases/Public/Middlewares/AuthMiddlewareTests.swift index a076128cf..d7abfa385 100644 --- a/Tests/DiscogsService/Cases/Public/Middlewares/AuthMiddlewareTests.swift +++ b/Tests/DiscogsService/Cases/Public/Middlewares/AuthMiddlewareTests.swift @@ -14,6 +14,8 @@ import struct Foundation.URL import struct Foundation.URLComponents +import struct Foundation.URLQueryItem +import struct HTTPTypes.HTTPField import struct HTTPTypes.HTTPFields import struct HTTPTypes.HTTPRequest import struct HTTPTypes.HTTPResponse @@ -25,6 +27,92 @@ import Testing @Suite("Auth Middleware", .tags(.middleware)) struct AuthMiddlewareTests { + // MARK: Initializers tests +#if swift(>=6.2) + @Test(arguments: Input.authMethods) + func `initialize`( + _ authMethod: AuthMethod + ) async throws { + try assertInit( + authMethod: authMethod, + authTransport: try randomTransport + ) + } + + @Test(arguments: zip( + Input.authMethodsThrows, + Output.authMethodsThrows + )) + func `initialize throws`( + _ authMethod: AuthMethod, + expects error: InputValidationError? + ) async throws { + try assertInitThrows( + authMethod: authMethod, + authTransport: try randomTransport, + expects: error + ) + } +#else + @Test("initialize", arguments: Input.authMethods) + func initialize( + _ authMethod: AuthMethod + ) throws { + try assertInit( + authMethod: authMethod, + authTransport: try randomTransport + ) + } + + @Test("initialize throws", arguments: zip( + Input.authMethodsThrows, + Output.authMethodsThrows + )) + func initializeThrows( + _ authMethod: AuthMethod, + expects error: InputValidationError? + ) throws { + assertInitThrows( + authMethod: authMethod, + authTransport: try randomTransport, + expects: error + ) + } +#endif + + // MARK: Properties tests +#if swift(>=6.2) + @Test(arguments: zip( + Input.authMethods, + Output.authMethodsShouldAuthenticate + )) + func `should authenticate`( + _ authMethod: AuthMethod, + expects flag: Bool + ) throws { + try assertShouldAuthenticate( + authMethod: authMethod, + authTransport: try randomTransport, + expects: flag + ) + } +#else + @Test("should authenticate", arguments: zip( + Input.authMethods, + Output.authMethodsShouldAuthenticate + )) + func shouldAuthenticate( + _ authMethod: AuthMethod, + expects flag: Bool + ) throws { + try assertShouldAuthenticate( + authMethod: authMethod, + authTransport: try randomTransport, + expects: flag + ) + } +#endif + // MARK: Functions tests #if swift(>=6.2) @@ -134,12 +222,94 @@ private extension AuthMiddlewareTests { // MARK: Functions + /// Asserts the initialization of the middleware, especially the assignment of its properties. + /// - Parameters: + /// - authMethod: A representation of an authentication method. + /// - authTransport: A representation of an authentication transport. + /// - Throws: an error of type ``InputValidationError`` in case of an unexpected error occurs while running test cases. + func assertInit( + authMethod: AuthMethod, + authTransport: AuthTransport, + ) throws { + // GIVEN + // WHEN + let middleware = try AuthMiddleware( + method: authMethod, + transport: authTransport + ) + + // THEN + switch (authMethod, authTransport) { + case let (.consumer(key, secret), .onHeader): + #expect(middleware.authItems == nil) + #expect(middleware.authField == .init( + name: .authorization, + value: "Discogs \(String.Parameter.key)=\(key), \(String.Parameter.secret)=\(secret)" + )) + + case let (.consumer(key, secret), .onQuery): + #expect(middleware.authField == nil) + #expect(middleware.authItems == [ + .init(name: .Parameter.key, value: key), + .init(name: .Parameter.secret, value: secret) + ]) + + case let (.user(token), .onHeader): + #expect(middleware.authItems == nil) + #expect(middleware.authField == .init( + name: .authorization, + value: "Discogs \(String.Parameter.token)=\(token)" + )) + + case let (.user(token), .onQuery): + #expect(middleware.authField == nil) + #expect(middleware.authItems == [ + .init(name: .Parameter.token, value: token) + ]) + + case (.none, _): + #expect(middleware.authField == nil) + #expect(middleware.authItems == nil) + } + } + + /// Asserts the error throwing (if justified) during the initialization of the middleware. + /// - Parameters: + /// - authMethod: A representation of an authentication method. + /// - authTransport: A representation of an authentication transport. + /// - error: An expected error of type ``InputValidationError`` during the initialization of the middleware. + func assertInitThrows( + authMethod: AuthMethod, + authTransport: AuthTransport, + expects error: InputValidationError? + ) { + // GIVEN + // WHEN + // THEN + if let error { + #expect(throws: error) { + try AuthMiddleware( + method: authMethod, + transport: authTransport + ) + } + } else { + #expect(throws: Never.self) { + try AuthMiddleware( + method: authMethod, + transport: authTransport + ) + } + } + } + /// Asserts the interception of a request to add its authentication. /// - Parameters: /// - authMethod: A representation of an authentication method. /// - authTransport: A representation of an authentication transport. /// - path: A URI path for a request. /// - headerFields: A set of header fields for a request. + /// - Throws:An error in case of an unexpected issue encountered while running a test case. func assertIntercept( authMethod: AuthMethod, authTransport: AuthTransport, @@ -147,7 +317,7 @@ private extension AuthMiddlewareTests { headerFields: HTTPFields = [:], ) async throws { // GIVEN - let middleware = AuthMiddleware( + let middleware = try AuthMiddleware( method: authMethod, transport: authTransport ) @@ -166,22 +336,24 @@ private extension AuthMiddlewareTests { ) { request, _, _ in // THEN switch (authMethod, authTransport) { - case let (.consumer(key, secret), .onHeader): + case (.consumer, .onHeader): #expect(request.path == path) #expect(request.headerFields != headerFields) - #expect(request.headerFields[.authorization] == "Discogs key=\(key), secret=\(secret)") + #expect(request.headerFields.contains(where: { $0.name == .authorization })) + case (.consumer, .onQuery): - #expect(request.path != path) - try assertAuthInPath(request.path, authMethod) #expect(request.headerFields == headerFields) - case let (.user(token), .onHeader): + try assertAuthInPath(request.path, authMethod) + + case (.user, .onHeader): #expect(request.path == path) #expect(request.headerFields != headerFields) - #expect(request.headerFields[.authorization] == "Discogs token=\(token)") + #expect(request.headerFields.contains(where: { $0.name == .authorization })) + case (.user, .onQuery): - #expect(request.path != path) - try assertAuthInPath(request.path, authMethod) #expect(request.headerFields == headerFields) + try assertAuthInPath(request.path, authMethod) + case (.none, _): #expect(request.path == path) #expect(request.headerFields == headerFields) @@ -194,10 +366,33 @@ private extension AuthMiddlewareTests { } } + /// Asserts the value of `shouldAuthenticate` flag after an initialization of a middleware. + /// - Parameters: + /// - authMethod: A representation of an authentication method. + /// - authTransport: A representation of an authentication transport. + /// - flag: An expected flag that indicates whether the middleware should authenticate its requests or not. + /// - Throws: An error of type ``InputValidationError`` in case of an unexpected issue occurs while running test cases. + func assertShouldAuthenticate( + authMethod: AuthMethod, + authTransport: AuthTransport, + expects flag: Bool + ) throws { + // GIVEN + // WHEN + let middleware = try AuthMiddleware( + method: authMethod, + transport: authTransport + ) + + // THEN + #expect(middleware.shouldAuthenticate == flag) + } + /// Asserts a request path to contain authentication parameters in its query. /// - Parameters: /// - path: A request path /// - authMethod: A representation of an authentication method. + /// - Throws:An error in case of an unexpected issue encountered while unwrapping the optionals. func assertAuthInPath( _ path: String?, _ authMethod: AuthMethod @@ -222,6 +417,19 @@ private extension AuthMiddlewareTests { // MARK: - Helpers +private extension AuthMiddlewareTests { + + // MARK: Properties + + /// Provides a random authentication transport representation. + var randomTransport: AuthTransport { + get throws { + try #require(AuthTransport.allCases.randomElement()) + } + } + +} + private extension HTTPRequest { // MARK: Initializers @@ -250,12 +458,34 @@ private extension HTTPRequest { // MARK: - Constants private extension Input { - /// A list of authentication methods for a request. + /// A list of authentication methods to use in most of the test cases. static let authMethods: [AuthMethod] = [ - .consumer(key: "SomeKey", secret: "SomeSecret"), - .user(token: "SomeToken"), + .consumer(key: "aAbBcCdDeEfFgGhHiIjJ", secret: "aAbBcCdDeEfFgGhHiIjJkKlLmMnNoOpP"), + .user(token: "aAbBcCdDeEfFgGhHiIjJkKlLmMnNoOpPqQrRsStT"), .none ] + /// A list of authentication methods to tests for the initialization throw test cases. + static let authMethodsThrows: [AuthMethod] = authMethods + [ + .consumer(key: .empty, secret: "aAbBcCdDeEfFgGhHiIjJkKlLmMnNoOpP"), + .consumer(key: "aAbBcCdDeEfFgGhHiI", secret: "aAbBcCdDeEfFgGhHiIjJkKlLmMnNoOpP"), + .consumer(key: "aAbBcCdDeEfFgGhHiIjJkK", secret: "aAbBcCdDeEfFgGhHiIjJkKlLmMnNoOpP"), + .consumer(key: "a4bBcCdDe3fFg6hH1Ij7", secret: "aAbBcCdDeEfFgGhHiIjJkKlLmMnNoOpP"), + .consumer(key: "aAbBcCdDeEfFgGhHiIjJ", secret: .empty), + .consumer(key: "aAbBcCdDeEfFgGhHiIjJ", secret: "aAbBcCdDeEfFgGhHiIjJkKlLmMnNoO"), + .consumer(key: "aAbBcCdDeEfFgGhHiIjJ", secret: "aAbBcCdDeEfFgGhHiIjJkKlLmMnNoOpPqQ"), + .consumer(key: "aAbBcCdDeEfFgGhHiIjJ", secret: "a4bBcCdDe3fFg6hH1IjJkK1LmMnNo0p9"), + .user(token: .empty), + .user(token: "aAbBcCdDeEfFgGhHiIjJkKlLmMnNoOpPqQrRsS"), + .user(token: "aAbBcCdDeEfFgGhHiIjJkKlLmMnNoOpPqQrRsStTuU"), + .user(token: "a4bBcCdDe3fFg6hH1IjJkK1LmMnNo0p9qQrRs5t7"), + ] +} + +private extension Output { + /// A list of expected input validation errors (if thrown) coming from the initialization throw test cases. + static let authMethodsThrows: [InputValidationError?] = [nil, nil, nil, .inputIsEmpty, .inputNotConsumerKey, .inputNotConsumerKey, .inputNotConsumerKey, .inputIsEmpty, .inputNotConsumerSecret, .inputNotConsumerSecret, .inputNotConsumerSecret, .inputIsEmpty, .inputNotUserToken, .inputNotUserToken, .inputNotUserToken] + /// A list of expected boolean flags coming from the should authenticate test cases. + static let authMethodsShouldAuthenticate: [Bool] = [true, true, false] } private extension String {