Skip to content

feat: device authentication #42

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

Merged
merged 14 commits into from
May 26, 2025
Merged

Conversation

nkottary
Copy link
Member

@nkottary nkottary commented Apr 30, 2025

Implements device auth flow. As far as the state changes and interface is concerned, device auth flow is the same as the current browser challenge flow. The main advantage of device flow is that it avoids race conditions with the regular browser token because it is treated differently in the backend. Device token feature is not currently deployed on juliahub.com so this PR needs to be tested on development instances.

Working of this PR:

Please see the changes in auth-flow.md file.

Authenticating with device tokens:

The interface does not change for the user. PkgAuthentication.authenticate() can be called and it will seamlessly decide on what authentication mechanism to use.

@mortenpi
Copy link
Member

mortenpi commented May 6, 2025

Since it's getting a little bit complicated now, with the package supporting multiple flows, could we add a small markdown file into docs/ that summarizes things a bit? I think it should cover:

  • How device token flow works (with references to the standard if that's a thing?)
  • How the current flow works.
  • The fallback logic.
  • Assumptions around refresh tokens.

@nkottary
Copy link
Member Author

nkottary commented May 6, 2025

@mortenpi Added docs to the PR now.

@nkottary nkottary marked this pull request as ready for review May 6, 2025 10:21
nkottary added 2 commits May 22, 2025 05:13
* refactor, use dex well known conf, use env for client id
* Add docs for device flow
* Move auth_suffix under states
* tests for device auth
* Use custom config endpoint
Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly cosmetic stuff on the text, but also a little bit of bikeshedding on the configuration endpoint.

nkottary and others added 8 commits May 23, 2025 14:57
Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
@nkottary nkottary marked this pull request as ready for review May 23, 2025 15:00
@nkottary nkottary requested review from mortenpi and tanmaykm May 23, 2025 15:01
@nkottary nkottary enabled auto-merge (squash) May 23, 2025 15:01
challenge::String
response::String
response::Union{String, Dict{String, Any}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like it would have been cleaner to bifurcate the state here to a separate RequestLoginDevice state. Then we wouldn't have to overload the same state and handling method with logic from both flows. But not going to insist a change in this PR.

@@ -330,19 +441,22 @@ token, or to Failure if the polling times out, or there is an unexpected error.
"""
struct ClaimToken <: State
server::String
challenge::String
response::String
auth_suffix::String
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, a separate ClaimDeviceToken state may have been cleaner.

@nkottary nkottary merged commit acc0663 into JuliaComputing:master May 26, 2025
14 checks passed
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.

4 participants