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

Add support for OAUTH2 device flow (RFC 8628) #302

Closed
wants to merge 1 commit into from

Conversation

a-ilin
Copy link

@a-ilin a-ilin commented Oct 25, 2024

Hi! Thank you for the great tool!

Could you please take a look at this PR?

Description

This PR adds support for OAUTH2 device flow:

Device flow is particularly useful on headless systems, as it does not require usage of the redirect URL.

I successfully tested this PR with a free Microsoft account (outlook.com), and own application client_id.
The application (client_id) must be explicitly allowed to use the device flow in the Azure console.

Implementation

The following properties are introduced into the request propagated from OAUTH2 to the main app:

  • need_response: A boolean specifying the flow implies a response from a user to be provided to the proxy in order to retrieve the tokens. It is false for device flow, and true for others.
  • user_code: A (short) string, which should be provided to the user for authentication at the service provider. User should manually enter it at the provider's web page.

The following changes are done to the UI, both graphical and console:

  • external auth: the user code is printed to the console together with the verification URI.
  • local server auth: the user code is printed to the console together with the verification URI.
  • icon: the user code is added to the title of the authorization window.

QA

I tested the proxy on Linux systems, in container and on desktop.
The following test cases are successful:

  • external auth: the verification URI and user code are printed to the console.
  • local server auth: the verification URI and user code are printed to the console.

Attention! On my systems I cannot make GUI work at all (even without my changes). The icon in the tray is always frozen, without notifications, and without context menu.
Therefore I cannot validate that the changes are correct for GUI interaction.

@simonrob
Copy link
Owner

Thanks for the work adding this feature. I'll need time to review it - hopefully in the next few weeks.

@a-ilin
Copy link
Author

a-ilin commented Oct 28, 2024

Thanks in advance!

Soon I will add one more PR based on the changes from this one, to complete the authorization use case for headless servers.

@simonrob
Copy link
Owner

simonrob commented Nov 5, 2024

I had a look at this just now. I had to adjust your approach a little in order to maintain GUI compatibility. I also tried to stick to the current architecture of the proxy more closely where possible.

Because the commits in your branch aren't signed (so can't be merged), I made a new branch: device-authorisation-grant. Could you take a look?

@a-ilin
Copy link
Author

a-ilin commented Nov 10, 2024

Thank you for the rework! I had a look over the changed implementation, and verified the case with --no-gui and --external-auth options. The proxy works fine!

The only issue I noticed in the code is that the interval field within the authorization response should be optional, and in case of absence it should be set to 5 seconds, as per the RFC. Currently there is no fallback in get_oauth2_authorisation_tokens.

@simonrob
Copy link
Owner

Thanks for taking a look, and for spotting the optionality of the interval – ab0fbd3 re-adds this. I'll go ahead and merge this new feature now.

@simonrob
Copy link
Owner

Closing this PR as its features have been added in b62dd58. Thanks @a-ilin for the work on this.

@simonrob simonrob closed this Nov 11, 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.

2 participants