-
-
Notifications
You must be signed in to change notification settings - Fork 17
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
Device Code Flow into Vapor OAuth #17
Conversation
@0xTim @marius-se Will this request be merged? Would also be great :-) |
* add PKCE parameters to AuthorizationRequestObject * Update Vapor dependency version * Add PKCE parameters to OAuthCode class * Refactor AuthorizationRequestObject initializer signature * Update CodeManager protocol to include PKCE parameters * Add PKCE validation for code challenge method * Update EmptyCodeManager to include PKCE parameters * Add token generation and retrieval methods to TokenManager protocol * Add PKCE parameters to AuthorizePostRequest * Add CryptoKit import and include code_verifier parameter for PKCE * Add PKCE parameters to code generation and validation * Use SwiftCrypto for SHA-256 hashing and PKCE validation * Refactor StaticClientRetriever to use private clients dictionary * Add Sendable conformance to AuthorizeHandler protocol * Add Sendable conformance to UserManager protocol * Add Sendable conformance to TokenManager protocol * Add Sendable protocol to ResourceServerRetriever * Add Sendable conformance to CodeManager protocol * Add Sendable conformance to ClientRetriever protocol * Update OAuthHelper to conform to Sendable * Refactor OAuthHelper for Actor-Based Concurrency * Update swift-tools-version to 5.9 * OpenID Configuration 1.0 Update and Swift Container Version Upgrade to 5.9.2-jammy (Actions) (#2) * Add OAuth 2.0 Discovery Document * Update Swift container version to 5.9.2-jammy * Update macOS build and test environment Remove swiftlint:disable comment in OAuthUser.swift * Enhance OAuth Security with JWTKit Integration and PKCE Code Challenge Handling (#4) * Add JWTKit dependency and implement IDToken protocol * Add generateAccessRefreshTokens method to TokenManager protocol * Add custom IDToken struct and update TokenManager implementations * Ensure PKCE Code Challenge is Properly Handled in Authorization Flow * Add tests for code challenge and code challenge method
Would be great if it’s merged! But is this package abandoned? |
I'll look into this early next week! Sorry for the late reply, this project is not abandoned! |
Update Vapor to 4.90.0: Fix for URI Parsing Vulnerability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't manage to go through the entire PR yet, but it's looking good so far! Just a few nits here and there :)
Sources/VaporOAuth/DefaultImplementations/EmptyCodeManager.swift
Outdated
Show resolved
Hide resolved
Sources/VaporOAuth/DefaultImplementations/StaticClientRetriever.swift
Outdated
Show resolved
Hide resolved
Sources/VaporOAuth/DefaultImplementations/StaticClientRetriever.swift
Outdated
Show resolved
Hide resolved
@@ -1,62 +1,79 @@ | |||
import Vapor | |||
|
|||
|
|||
actor RemoteTokenResponseActor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, why an actor? structs are way cheaper!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @marius-se, an actor is the best choice for our scenario, as it ensures thread-safe, concurrent access to shared mutable state, aligning well with Swift's concurrency model. While a class offers shared state, it lacks inherent thread safety. A struct, being a value type, isn't suited for shared, mutable state in concurrent contexts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, sorry my question wasn't very precise, let me rephrase it: Why do we need (shared) mutable state here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, so the shared mutable state is needed for managing OAuth token responses across concurrent operations, ensuring updates and synchronization of token data in a multi-threaded environment.
Comprehensive Refactoring of OAuth Components for Concurrency and PKCE Support
Patch Release: Refactor OAuth models to structs, extend OAuthUser, and update Vapor
@vamsii777 I haven't done a full review of this yet but I have some initial feedback/queries. First, this is a pretty huge PR and ideally would be broken up so we could discuss some of the changes in isolation. However, some of the immediate things that I've spotted:
|
@0xTim Gotcha, I'll break down the PR and YES would refactor so that you would be comfortable. |
DO NOT MERGE. Moved to #28 |
This pull request introduces the integration of the Device Code Flow into the Vapor OAuth, enhancing the existing OAuth2 Provider capabilities.
Changes:
DeviceCodeTokenHandler
to manage device code token requests.Impact:
This integration allows applications to utilize the Device Code Flow for authorization, providing a more secure and user-friendly way to authenticate devices without a browser or other user agent.
Testing:
All test cases have been updated and passed, ensuring that the new feature is working as expected and does not break existing functionality.
Please review the changes and provide feedback. Thank you!