feat: OAuth 2.0 Device Authorization Grant #2416 by supercairos · Pull Request #3252 · ory/hydra

supercairos · 2022-09-08T23:16:05Z

This PR adds the Device Auth Grant to Ory/Hydra
RFC: https://www.rfc-editor.org/rfc/rfc8628

Related issue(s)

This PR is related to #2416

See also fosite PR:
https://github.com/ory/fosite/pull/701/files

Checklist

I have read the contributing guidelines.
I have referenced an issue containing the design document if my change
introduces a new feature.
I am following the
contributing code guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added or changed the documentation.

BREAKING CHANGE: SDK object `PatchDocument` was renamed to `JsonPatchDocument`.

Closes ory#3142

…nerator

Co-authored-by: Andreas Krause <a.krause@celonis.com>

BuzzBumbleBee · 2023-11-24T18:04:57Z

@aeneasr by the way of introduction I am the cloud native product manager at Canonical (the publisher of Ubuntu) We use Hydra internally and are also interested in this feature landing in the product. Is there anything we can do to help getting the work over the line? (We have an engineering team focused on identity management which could provide support)

If their is a chance to get this merged I can put some time into cleaning up my original implementation, up to now I think @supercairos has been keeping the MR alive as they are using this feature now.

I can also run you or other devs through the implementation

alnr · 2023-11-24T19:00:41Z

Just a quick update: this is still on our plate and we want to move this forward. Since we’ll own the code after it’s merged, we must allocate resources to really make sure we’re completely confident in how the feature integrates in hydra.

I’d expect some movement here soon.

Thanks massively again for the contribution and your patience!

shipperizer · 2023-11-24T19:26:22Z

@aeneasr by the way of introduction I am the cloud native product manager at Canonical (the publisher of Ubuntu) We use Hydra internally and are also interested in this feature landing in the product. Is there anything we can do to help getting the work over the line? (We have an engineering team focused on identity management which could provide support)

@alnr just to reinforce the point above, i'm the lead engineer of the aforementioned IAM team at canonical, we are more than happy to divert some resources and help on the effort

If their is a chance to get this merged I can put some time into cleaning up my original implementation, up to now I think @supercairos has been keeping the MR alive as they are using this feature now.

I can also run you or other devs through the implementation

@BuzzBumbleBee yes please, that would be great, feel free to get in touch, here's my email alessandro.cabbia@canonical.com and we can take it from there

supercairos · 2023-11-24T19:49:39Z

Hi all, would love to see this merged!
And would be happy to help on the effort :)
I hope on Monday to port our patch on the latest Hydra HEAD commit.

supercairos · 2023-11-27T09:11:41Z

Feel free to check the sister PR also @Fosite:
ory/fosite#701

supercairos · 2023-11-27T09:50:51Z

this code is missing quite a few tests but I can definitly work on this if it helps having this PR merged :)

shipperizer · 2023-11-27T11:14:14Z

@supercairos happy to contribute if help is needed, just need a bit of direction on how and where

hperl

Thank you very much for all the hard work! I finally managed to do a pass over the Hydra and Fosite PRs. I have left some comments, questions and ideas to improve this. Thank you for looking into this!

oauth2/handler.go

hperl · 2023-11-27T11:14:52Z

consent/strategy_default.go

 	return ls
 }
+
+func (s *DefaultStrategy) requestDevice(ctx context.Context, w http.ResponseWriter, r *http.Request, req fosite.Requester) error {


Confusing name. Are we requesting a device?

I guess it makes sense now, we ask the user to either enter the user_code or confirm the code sent in the query param.

hperl · 2023-11-27T11:48:02Z

oauth2/handler.go

 	DeleteTokensPath = "/oauth2/tokens" // #nosec G101
+
+	// Device Grant Handler
+	DeviceAuthPath = "/oauth2/device/auth"


Should we have two different paths, one for the user and one for the device?

E.g., /oauth/device/auth for the POST from the device, and /oauth/device/verify for the user to verify the device?

Additionally, I think it might be useful to handle user POST against the verification_url (https://datatracker.ietf.org/doc/html/rfc8628#section-3.3) so that a UI can render a simple form that will submit the user_code against the same endpoint.

Additionally, I think it might be useful to be able to configure a shorter version of this URL. /oauth2/device/auth is already quite long if you have to type it on a mobile, plus some origin. How about /device (as in the spec), or just /dev, or /oauth2/dev if you want to stick with the namespacing? But really, it should be configurable IMHO.

WDYT?

Yes, that's one of the missing feature of this PR.
It shoudn't be hard to implemenent but I had to revert this particular feature because it was not working after the 2.1.1 merge.

We (at shadow) use a UI trick on the TV side to render a shorter URL (using a minified URL) but yes it should be configurable 👍

I can work on that after having fixed all the other PR reviews :)

Additionally, I think it might be useful to handle user POST against the verification_url (https://datatracker.ietf.org/doc/html/rfc8628#section-3.3) so that a UI can render a simple form that will submit the user_code against the same endpoint.

Please ignore this remark, I now understand your approach with the device verifier, which is using a POST to the UI and then an admin API to bind the device to the flow.

hperl · 2023-11-27T12:48:00Z

oauth2/handler.go

+}
+
+// FIXME: Add Doc
+func (h *Handler) performOAuth2DeviceAuthorizationFlow(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {


Sure it's missing quite a lots of tests :)

I'll add them asap :)

hperl · 2023-11-27T12:48:09Z

oauth2/handler.go

+//	Responses:
+//	  200: deviceAuthorization
+//	  default: errorOAuth2
+func (h *Handler) performOAuth2DeviceFlow(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {


hperl · 2023-11-27T12:48:51Z

persistence/sql/persister_consent.go

 	return f.GetConsentRequest(), nil
 }

+func (p *Persister) CreateDeviceGrantRequest(ctx context.Context, req *flow.DeviceGrantRequest) error {


Remove these if you agree we can use user_code.

Otherwise, we'll need tests ;).

OK, now I know we need this.

hperl · 2023-11-27T12:49:54Z

persistence/sql/persister_oauth2.go

 	)
 }
+
+func (p *Persister) CreateDeviceCodeSession(ctx context.Context, signature string, requester fosite.Requester) (err error) {


Tests for all new methods in this file please.

hperl · 2023-11-27T12:50:43Z

persistence/sql/persister_oauth2.go

+	sqlTableRefresh    tableName = "refresh"
+	sqlTableCode       tableName = "code"
+	sqlTablePKCE       tableName = "pkce"
+	sqlTableDeviceCode tableName = "device_code"


Please check if we really need two tables here :).

In fact, the best would be to use the flow table as it can be seen as part of a login flow but since this MR was done before the 2.x we never took the time to update the database scheme.

But yes i believe there can be only one DB for both device_code and user_code as they are tied together

supercairos · 2023-11-27T20:07:55Z

Here are some tools help develop this feature:

A fake client to test the DAG: https://github.com/supercairos/oauth-device-flow-client-sample
A Hydra-UI that implement the DAG: https://github.com/supercairos/hydra-login-consent-node

lltr · 2023-12-08T18:05:50Z

Looking forward to this being out! Can't find any other open source provider supporting this.

RomanHotsiy · 2024-02-26T08:29:32Z

Hey folks 👋. Any plans to get it moving? Do you need any help?

aeneasr and others added 30 commits September 5, 2022 16:11

fix: improve lazy initialization of JWKs

392d4c5

feat: allow config context

5de9fcb

fix: cockroach migration fixes

dd02c7b

fix: lazy load PKI

18b74bd

test: update fixtures

41283d5

fix: improve jwk generator defaults

8c13153

chore: format

2dfe393

refactor(sdk): rename PatchDocument to JsonPatchDocument

7c134b7

BREAKING CHANGE: SDK object `PatchDocument` was renamed to `JsonPatchDocument`.

fix(sdk): resolve type issues and regenerate SDK

719db10

chore(sdk): remove obsolete template

5161259

feat(sdk): add missing bearer security definition

1a3f2c2

chore: styles

f6cb9f4

chore: fix compile and lint issues

85e3eae

fix(sdk): remove pattern from scope parameter

26d752a

Closes ory#3142

fix: resolve test issues and regressions introduced by the new JWK ge…

9a8a7ab

…nerator

fix: regression in database layer

364ba11

fix(hsm): public key extraction

fcd53d2

fix(hsm): public key extraction everywhere

b8fb997

chore: upgrade golangci-lint

8484d1e

fix: compile issue

0311aea

test(e2e): respect metadata

762e3eb

fix(jwk): expose correct metadata algorithms

0496a32

test(hsm): resolve test issues

19bfc53

chore: format

7461b13

test(e2e): upgrade jwks-rsa

1a6c5e7

test(e2e): upgrade cypress

81f8a81

fix: default back to RS256 keys

774a158

chore: format

406a091

test: update snapshots

f72e695

fix: missing data in JWT grant (ory#3143)

f90079a

Co-authored-by: Andreas Krause <a.krause@celonis.com>

Merge branch 'master' into feat_dev_grants_2x

3711af2

supercairos requested a review from hperl as a code owner November 24, 2023 17:22

supercairos added 3 commits November 25, 2023 01:31

Fix test error

662801a

Fix tests

aed80e9

Fix tests

25346c6

hperl requested changes Nov 27, 2023

View reviewed changes

supercairos mentioned this pull request Nov 27, 2023

Device grant flow (migrate to master) ory/fosite#701

Closed

6 tasks

Fix route spec

44dd093

Update struc name

b8c598a

supercairos force-pushed the feat_dev_grants_2x branch from 22a2817 to b8c598a Compare January 4, 2024 16:02

supercairos added 3 commits January 4, 2024 17:04

Update pkgs

d25a3a8

Merge branch 'master' into feat_dev_grants_2x

5191714

Start working on persister tests

edf0738

This was referenced Feb 9, 2024

Implement the device authorization endpoint #3714

Closed

Implement the device authorization endpoint canonical/hydra#1

Merged

nsklikas mentioned this pull request Sep 30, 2024

feat: implement RFC 8628 #3851

Closed

7 tasks

nsklikas mentioned this pull request Dec 18, 2024

feat: implement RFC 8628 #3912

Merged

7 tasks

aeneasr closed this in #3912 Feb 26, 2025

aeneasr closed this in 5215d24 Feb 26, 2025

Conversation

supercairos commented Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related issue(s)

Checklist

Uh oh!

BuzzBumbleBee commented Nov 24, 2023

Uh oh!

alnr commented Nov 24, 2023

Uh oh!

shipperizer commented Nov 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

supercairos commented Nov 24, 2023

Uh oh!

supercairos commented Nov 27, 2023

Uh oh!

supercairos commented Nov 27, 2023

Uh oh!

shipperizer commented Nov 27, 2023

Uh oh!

hperl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

supercairos commented Nov 27, 2023

Uh oh!

lltr commented Dec 8, 2023

Uh oh!

RomanHotsiy commented Feb 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

16 participants

supercairos commented Sep 8, 2022 •

edited

Loading

shipperizer commented Nov 24, 2023 •

edited

Loading