Refactor Authentication Flows for consistency and customization #214

mikenachbaur-okta · 2024-12-10T01:23:55Z

This introduces a generic authentication flow configuration protocol that various authentication flows can support.
It refactors all authentication flows for consistency and customization by:

Introducing the concept of an Authentication Context, which is used to supply flow-specific capabilities to adapt the flows to specific use-cases.
Normalizes the coalescence of values supplied to OAuth2Client configuration, auth flow context, and user-supplied information.
Introduces support for ACR Values
Paves the way for refactoring these objects and types to support actors and @Sendable

This introduces a generic authentication flow configuration protocol that various authenetication flows can support. Concrete implementations for other OAuth2 flows to follow.

…requests

…ation views for OOB factors

- Introduces the concept of an Authentication Context, which is used to supply flow-specific capabilities to adapt the flows to specific use-cases. - Normalize the coalescence of values supplied to OAuth2Client configuration, auth flow context, and user-supplied information. - Introduce support for ACR Values - Pave the way for refactoring these objects and types to support actors and @sendable

…Token validator

bmitchelmore

Changes look okay to me, just some nits and quibbles.

bmitchelmore · 2025-02-03T20:47:08Z

Sources/AuthFoundation/OAuth2/Authentication.swift

    /// Indicates if this flow is currently authenticating.
    var isAuthenticating: Bool { get }

+    /// Optional request parameters to be added to requests made from this flow.
+    var additionalParameters: [String: APIRequestArgument]? { get }


I don't see the need to have this as optional unless we have some special behaviour in mind for the empty dictionary case.

My feeling is that since the property is optional, it makes sense for it to actually be an Optional, particularly since it feels odd to say something is optional on one had while requiring a result on the other.

Additionally, most developers using this won't see the API documentation (or, if they're attentive, they might see the inline docs during code completion in Xcode). So in general, if something is optional, the API contract should establish that in a self-documenting fashion.

Sources/AuthFoundation/OAuth2/Authentication.swift

bmitchelmore · 2025-02-03T20:53:37Z

Sources/AuthFoundation/OAuth2/Configuration.swift


        /// The list of OAuth2 scopes requested for this client.
-        public let scopes: String
+        public var scope: String


I guess the spec refers to this as scope even though it's almost universally treated as a space separated list of scopes? Given that we're already changing the public api, is it worth providing a scopes: [String] (or scopes: Set<String>) variable that automatically updates scope with a space separated list of values?

Yes, I'm trying to keep the naming as consistent with the specs as possible. And I agree, the space-separated list encoded as a string, as opposed to an array, does feel "non-swifty". And since acrValues is also a space-separated list in the spec and I'm representing it as an array, it makes sense to align both properties for consistency.

I'm not sure about making it a Set though. While the spec doesn't mention it, I don't want to remove the ability for a developer to specify duplicate values if they choose to do so. Plus, since one of the goals of this SDK is to have architectural and functional parity across different languages, I don't want to make it difficult for Javascript to adopt later.

I agree with @bmitchelmore that if we're going to change it, wouldn't [String] would be better? Then we can handle the details of string concatenation for them

@stevelind-okta I'm discussing this with the other SDK engineers. A change like this should be applied consistently to the other SDKs since this is an API spec change that's applicable to the other languages. I agree that we should be consistent in how whitespace-separated collections are handled in the SDK, but the question remains whether or not we should process the responses/request arguments from the Authorization Server for developer convenience, or if we should stick to the spec.

Update: I've discussed this with the other SDK engineers, and we've decided to adopt [String] across the board for these sorts of properties. I've updated the design guidelines with this decision, and will update this PR to make this consistent in similar values.

@stevelind-okta & @bmitchelmore I'll be adding these updates in another PR to keep the scope of this one down. But suffice to say, scope will become an array, just the same as acrValues.

bmitchelmore · 2025-02-04T04:47:04Z

Sources/AuthFoundation/OAuth2/Configuration.swift

+        if let container = try? decoder.container(keyedBy: CodingKeysV1.self),
+           container.allKeys.contains(.baseURL)
+        {
+            self.init(issuerURL: try container.decode(URL.self, forKey: .baseURL),


My personal preference for a situation like this would be to have two separate inits that each take a Decoder and each one attempts decoding using the different coding keys set, then this init would just delegate to those inits rather than baking it all into this one init

I like the one method which steps through the different decoding strategies because not all version migration features are as straightforward as this one. For example, in Token+Initialization.swift, adapting between v1 and v2 isn't as straightforward, particularly since it uses JSONEncoder.userInfo to pull in context from outside the encoded data. So having both init(from:) initializers follow the same patterns is useful for consistency.

bmitchelmore · 2025-02-04T04:54:02Z

Sources/AuthFoundation/Requests/Token+Requests.swift

+        if let scope = scope {
+            result["scope"] = scope
+        } else {
+            result.removeValue(forKey: "scope")


Was it possible for clientConfiguration.authentication to contain a scope value previously? Or for clientConfiguration.parameters(for: category) to contain a scope value now? Is there a reason to explicitly disallow the introduction of a scope via that mechanism? Is it documented?

This is more about the spec's definition around refreshing access tokens than client configuration.

A scope value is optional when making a refresh request, because when this value is omitted it means the Authorization Server should return a new access token with the same set of scopes that was originally returned. However, if a client explicitly includes a set of scopes when refreshing an access token, this is a way for the client to request a "downscope", constraining the resulting access token to a subset of the original scopes. This is a useful feature which is something that'll be added in a subsequent PR.

For completeness sake, the value returned from the if let scope = scope line is the explicit scope requested when performing a refresh, and not the scope implicitly referenced in the client configuration. So if a developer isn't explicitly asking for a particular set of scopes during the refresh request, we're following the spec and are omitting the value from the request.

bmitchelmore · 2025-02-04T05:46:26Z

Sources/OktaOAuth2/Internal/Extensions/AuthorizationCodeFlow+InternalExtensions.swift

+        parameters.merge(client.configuration.parameters(for: .authorization))
+        parameters.merge(context.parameters(for: .authorization))
+
+        components.percentEncodedQuery = parameters


Why are we manually percent encoding here? Can't URLComponents do the url encoding for you if you give it a queryItems array?

That's what this code used to do, except the default encoding strategy improperly uses %20 instead of +, which causes problems with handling of scope values on some authorization servers. #99

Sources/OktaOAuth2/Internal/Requests/DeviceAuthorizeFlow+Requests.swift

bmitchelmore · 2025-02-04T05:50:04Z

Sources/OktaOAuth2/Logout/SessionLogoutFlow.swift

        inProgress = true

        client.openIdConfiguration { result in
-            defer { self.reset() }


Seems like you call reset at the end of every code path, was defer not working as expected?

If an error is thrown, the reset() function shouldn't be called, so I moved it out of the defer block and opted to call reset explicitly.

Hmm, but you're still calling reset inside your catch block?

Hmm good catch 👍

The goal of this behavior is to differentiate between a server-generated error, or a local SDK failure. In the event the developer did something wrong, or if there's a spurious error, the context shouldn't be automatically reset, enabling them to debug & diagnose the issue.

I'll look into this one ...

Tests/OktaOAuth2Tests/OAuth2ClientTests.swift

bmitchelmore · 2025-02-04T05:59:49Z

Sources/AuthFoundation/Utilities/String+Extensions.swift

+        let regex = try! NSRegularExpression(pattern: "([a-z])([A-Z])")
+        let range = NSRange(location: 0, length: self.count)
+        let snakeCased = regex.stringByReplacingMatches(in: self, options: [], range: range, withTemplate: "$1_$2")
+        return snakeCased.lowercased()


This will map isHTTPResponse to is_httpresponse right? Acceptable?

Correct. There are more complete solutions that would map to is_http_response for example, but for the purposes of this SDK (where the aim is to map keys used in the OAuth2 specs) this seems fine, and easier to read. However @bmitchelmore, if you feel like it would make sense, I could change the implementation to iterate over the characters similar to this.

I think if you create a public extension, you introduce the possibility of someone else using it not aware of that specific limitation and suffering the consequences. So it might be worth it to either document the limitation or use an alternate implementation, but I leave that up to you.

@bmitchelmore I updated the implementation to be more "feature complete". It's still probably faster than using a regular expression, but it does look a bit more verbose. 🤷🏼‍♀️

jaredperreault-okta · 2025-02-04T15:00:11Z

Sources/AuthFoundation/Network/APIRequest.swift

@@ -148,7 +148,22 @@ public protocol APIParsingContext {
 }

 extension APIParsingContext {
+    @_documentation(visibility: private)
+    public var codingUserInfo: [CodingUserInfoKey: Any]? {


UserInfo may be confused with OIDC userInfo

UserInfo is actually a common property name used throughout Apple's frameworks. In this case, this corresponds to the JSONEncoder's userInfo property, which is used to pass additional information needed to properly encode/decode data.

jaredperreault-okta · 2025-02-04T15:12:54Z

Sources/AuthFoundation/Token Management/Token.swift

-                                                    "client_id": client.configuration.clientId,
-                                                    "scope": client.configuration.scopes
-                                                ])
+                                                   scope: nil,


Why is scope: nil?

Because it's inheriting the scopes from the initial token exchange. But this argument is being exposed to the refresh function for future expansion, when I introduce support for downscoping refresh tokens. I just didn't want to overly conflate this PR with additional user-facing features.

Samples/DirectAuthSignIn/DirectAuthSignIn/ContinuationView.swift

Samples/Shared/Common/Testing/Screen+Extensions.swift

rajdeepnanua-okta · 2025-02-06T20:20:21Z

Sources/AuthFoundation/Debugging/APIRequestObserver.swift

This is awesome!

Sources/AuthFoundation/Security/SecurityUtilities.swift

Sources/AuthFoundation/Utilities/String+Extensions.swift

rajdeepnanua-okta · 2025-02-07T17:09:38Z

Sources/OktaDirectAuth/Internal/Requests/OOBAuthenticateRequest.swift

@@ -39,17 +33,31 @@ struct OOBResponse: Codable, HasTokenParameters {
    func tokenParameters(currentStatus: DirectAuthenticationFlow.Status?) -> [String: APIRequestArgument] {
        ["oob_code": oobCode]
    }
+
+    static func == (lhs: OOBResponse, rhs: OOBResponse) -> Bool {


Is bindingCode not important for comparison here?

Great catch! 👍 Thanks, I'll make sure to add this in my next PR.

Sources/OktaOAuth2/Authentication/AuthorizationCodeFlow.swift

rajdeepnanua-okta · 2025-02-07T17:16:58Z

Sources/OktaOAuth2/Authentication/AuthorizationCodeFlow.swift

    ///   - context: Optional context to provide when customizing the state parameter.
-    ///   - additionalParameters: Optional additional query string parameters you would like to supply to the authorization server.
+    ///   - options: Options to customize this authentication flow.
    /// - Returns: The URL a user should be presented with within a browser, to continue authorization.
-    public func start(with context: Context? = nil, additionalParameters: [String: String]? = nil) async throws -> URL {
+    public func start(with context: Context = .init()) async throws -> URL {


I don't see options in the function signature?

Again, good catch! Thanks! 👍

rajdeepnanua-okta

This PR looks good to me from SDK and Oauth side. Please don't merge until other Swift devs give their approval

bmitchelmore

Big PR with lots of changes. Hard to be utterly confident, but it looks ok to me.

mikenachbaur-okta added 11 commits December 9, 2024 17:15

Accept ACR Values to authentication flows, Part 1

adc1a42

This introduces a generic authentication flow configuration protocol that various authenetication flows can support. Concrete implementations for other OAuth2 flows to follow.

Ensure the authentication flow config is included in all appropriate …

7817737

…requests

Update tests

655064a

Added a API debugging convenience class, and fixed DirectAuth continu…

eeaf5fe

…ation views for OOB factors

Fix lint error

0046217

Fix some test failures

828b975

Update minimum Swift version to 5.10

a4727c9

Update and add tests, and establish an explicit connection to the ID …

2bc85ec

…Token validator

More tests, and fixes for localization key collisions

7c54596

Update documentation & function signatures

e724f7b

mikenachbaur-okta changed the title ~~Accept ACR Values to authentication flows, Part 1~~ Refactor Authentication Flows for consistency and customization Jan 31, 2025

mikenachbaur-okta added 2 commits January 31, 2025 14:08

Remove an Xcode 16 hint that breaks in Xcode 15.4

fa5a0b6

Fix semgrep-scan job, and tidy up some things

d3f4c61

mikenachbaur-okta mentioned this pull request Jan 31, 2025

Expose a public initializer for the WebAuthn response object, and clean up API doc errors #213

Closed

mikenachbaur-okta requested review from jaredperreault-okta and rajdeepnanua-okta January 31, 2025 23:55

mikenachbaur-okta and others added 4 commits January 31, 2025 16:04

Use a different orb version

0ac0075

Update platform helper orb

562fc26

More updates for semgrep scan

7104297

Fix back snyk

59454a9

bmitchelmore reviewed Feb 4, 2025

View reviewed changes

jaredperreault-okta reviewed Feb 4, 2025

View reviewed changes

mikenachbaur-okta added 7 commits February 4, 2025 12:49

Fix tests & respond to PR feedback

1a20b80

Fix some automation tests

148314c

Refactor string-to-snake case conversion for a more complete solution

a702c3a

Fix a lint warning that behaves differently in CI

b6f858c

Correct some unit test expectations

0aebc26

Fix problems where Xcode 14.x cannot infer the type of XCUIKeyboardKey

027d66f

Another attempt at fixing a CI UI test issue

0a6f3c7

mikenachbaur-okta requested review from jaredperreault-okta and bmitchelmore February 6, 2025 17:12

rajdeepnanua-okta reviewed Feb 7, 2025

View reviewed changes

rajdeepnanua-okta approved these changes Feb 11, 2025

View reviewed changes

bmitchelmore approved these changes Feb 12, 2025

View reviewed changes

mikenachbaur-okta merged commit b0f51af into master Feb 13, 2025
14 checks passed

mikenachbaur-okta deleted the OKTA-663108-ACRValues branch February 13, 2025 00:24

mikenachbaur-okta mentioned this pull request Feb 13, 2025

Change scope to be an array of strings for developer convenience and consistency #217

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Authentication Flows for consistency and customization #214

Refactor Authentication Flows for consistency and customization #214

mikenachbaur-okta commented Dec 10, 2024 •

edited

Loading

bmitchelmore left a comment

bmitchelmore Feb 3, 2025

mikenachbaur-okta Feb 4, 2025

bmitchelmore Feb 3, 2025

mikenachbaur-okta Feb 4, 2025

stevelind-okta Feb 6, 2025

mikenachbaur-okta Feb 7, 2025

mikenachbaur-okta Feb 7, 2025 •

edited

Loading

mikenachbaur-okta Feb 11, 2025

bmitchelmore Feb 4, 2025

mikenachbaur-okta Feb 4, 2025

bmitchelmore Feb 4, 2025

mikenachbaur-okta Feb 4, 2025

bmitchelmore Feb 4, 2025

mikenachbaur-okta Feb 4, 2025

bmitchelmore Feb 4, 2025

mikenachbaur-okta Feb 4, 2025

bmitchelmore Feb 6, 2025

mikenachbaur-okta Feb 7, 2025

bmitchelmore Feb 4, 2025

mikenachbaur-okta Feb 4, 2025

bmitchelmore Feb 4, 2025

mikenachbaur-okta Feb 5, 2025

jaredperreault-okta Feb 4, 2025

mikenachbaur-okta Feb 4, 2025

jaredperreault-okta Feb 4, 2025

mikenachbaur-okta Feb 4, 2025

rajdeepnanua-okta Feb 6, 2025

rajdeepnanua-okta Feb 7, 2025

mikenachbaur-okta Feb 11, 2025

rajdeepnanua-okta Feb 7, 2025

mikenachbaur-okta Feb 11, 2025

rajdeepnanua-okta left a comment

bmitchelmore left a comment

Refactor Authentication Flows for consistency and customization #214

Refactor Authentication Flows for consistency and customization #214

Conversation

mikenachbaur-okta commented Dec 10, 2024 • edited Loading

bmitchelmore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikenachbaur-okta Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajdeepnanua-okta left a comment

Choose a reason for hiding this comment

bmitchelmore left a comment

Choose a reason for hiding this comment

mikenachbaur-okta commented Dec 10, 2024 •

edited

Loading

mikenachbaur-okta Feb 7, 2025 •

edited

Loading