Skip to content
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

Open
wants to merge 32 commits into
base: main
Choose a base branch
from

Conversation

vamsii777
Copy link

This pull request introduces the integration of the Device Code Flow into the Vapor OAuth, enhancing the existing OAuth2 Provider capabilities.

Changes:

  • Added Device Code Flow support following RFC 8628.
  • Implemented DeviceCodeTokenHandler to manage device code token requests.
  • Updated test cases to ensure the correct handling of device code expiration.

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!

@mynona
Copy link

mynona commented Dec 9, 2023

@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
@Aquaa7
Copy link

Aquaa7 commented Dec 26, 2023

Would be great if it’s merged! But is this package abandoned?

@marius-se
Copy link
Contributor

I'll look into this early next week! Sorry for the late reply, this project is not abandoned!

@marius-se marius-se self-assigned this Jan 4, 2024
Copy link
Contributor

@marius-se marius-se left a 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 :)

@@ -1,62 +1,79 @@
import Vapor


actor RemoteTokenResponseActor {
Copy link
Contributor

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!

Copy link
Author

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.

Copy link
Contributor

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?

Copy link
Author

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.

Sources/VaporOAuth/Helper/OAuthHelper+remote.swift Outdated Show resolved Hide resolved
Sources/VaporOAuth/Helper/OAuthHelper+remote.swift Outdated Show resolved Hide resolved
Sources/VaporOAuth/Models/OAuthDeviceCode.swift Outdated Show resolved Hide resolved
Sources/VaporOAuth/Models/OAuthDeviceCode.swift Outdated Show resolved Hide resolved
Sources/VaporOAuth/Models/OAuthDiscoveryDocument.swift Outdated Show resolved Hide resolved
Sources/VaporOAuth/Protocols/TokenManager.swift Outdated Show resolved Hide resolved
@0xTim
Copy link
Member

0xTim commented Jan 15, 2024

@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:

  • I'm not 100% comfortable with dropping support for 5.8 and 5.7 just yet unless we have a very good reason
  • We're annotating lots of stuff with Sendable, which is good, but should turn on complete concurrency checking
  • The main issue - there's lots of stuff in the PR that's related to OpenID. That's got nothing to do with RFC 8628 and is very likely out of scope for this library. OpenID is a can of worms I don't particularly want to tackle in this library and would ideally be done with a library built on top of this one. None of it's required for device code flow right?

@vamsii777
Copy link
Author

@0xTim Gotcha, I'll break down the PR and YES would refactor so that you would be comfortable.

@vamsii777
Copy link
Author

DO NOT MERGE. Moved to #28

@vamsii777 vamsii777 reopened this Jan 31, 2024
@vamsii777 vamsii777 marked this pull request as ready for review January 31, 2024 19:35
@vamsii777 vamsii777 changed the title Integrate Device Code Flow into Vapor OAuth Device Code Flow into Vapor OAuth Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants