Implement RFC 8628 #826

nsklikas · 2024-09-30T06:50:13Z

BREAKING CHANGES: This patch breaks up `OAuth2AuthorizeExplicitFactory` into
`OAuth2AuthorizeExplicitAuthFactory` and `Oauth2AuthorizeExplicitTokenFactory`

Checklist

I have read the contributing guidelines and signed the CLA.
I have referenced an issue containing the design document if my change introduces a new feature.
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 approval (please contact [email protected]) 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 the necessary documentation within the code base (if appropriate).

Further comments

This PR is based on the work done on #701, by @supercairos and @BuzzBumbleBee. That PR was based on an older version of fosite and was missing some features/tests.

Comments:

Due to some dependency issues we had to switch to go.uber.org/mock/gomock (github.com/golang/mock/gomock is deprecated)
We had to do a small refactor of the AuthorizeExplicitGrantHandler to generalize it to reduce code duplication

CLAassistant · 2024-09-30T06:50:20Z

All committers have signed the CLA.

nsklikas · 2024-09-30T07:14:18Z

Looks like the conformity tests are failing because it tries to use this version of fosite on hydra master. But they pass on the hydra PR with this version of fosite. Changes are needed on the hydra config DefaultProvider object, how should we fix this?

aeneasr

Thank you very much for this massive PR! There's still thousands of lines to review, but here are some first remarks.

Primarily, some of the diffs are hard to read because they re-organize code into different files. Would it be possible to focus the changes on what's required to change and leave the refactoring out? We can always re-organize the code later, but given the sensitivity of the files touched (core token methods), it will be significantly easier to review those changes. Thank you!

go.mod

fosite.go

handler/rfc8628/strategy_hmacsha.go

handler/oauth2/flow_authorize_code_token.go

aeneasr · 2024-10-09T12:46:48Z

Looks like the conformity tests are failing because it tries to use this version of fosite on hydra master. But they pass on the hydra PR with this version of fosite. Changes are needed on the hydra config DefaultProvider object, how should we fix this?

You should be able to change the target commit in the CI file to target your PR in hydra, which should make the tests pass

zepatrik

This looks great already, however due to the security implications I agree with @aeneasr that we should pull out some of the unrelated refactoring into separate PRs to reduce the size and complexity of this one, especially the migration from github.com/golang/mock/gomock to go.uber.org/mock/gomock and maybe also some other code style changes.
I did not yet fully review all parts, but these are some preliminary suggestions already.

device_request_handler.go

device_request_handler_test.go

device_request_test.go

device_response.go

errors.go

handler/oauth2/flow_authorize_code_token.go

handler/rfc8628/strategy_hmacsha.go

handler/oauth2/flow_generic_code_token.go

nsklikas · 2024-10-18T12:13:46Z

Thank you for the reviews.

I reverted the refactor on the oauth2 handlers and tried to address all the comments. Also pinned the ci to use hydra from the PR branch and now the tests pass. Please have another go.

handler/rfc7523/handler_test.go

device_write_test.go

handler/rfc8628/auth_handler.go

handler/rfc8628/storage.go

handler/openid/flow_device_auth.go

…ssertion

fix: add the OIDC handler for device flow

* fix: fix the refresh token issue * fix: fix the OIDC token missing issue

eseliger · 2025-01-15T13:14:32Z

Just wanted to leave a big thank you for the massive work you put into this @nsklikas!

aeneasr

Amazing work, there is one comment left around DeleteOpenIDSession, otherwise this is good to merge IMO

Next step is to get the hydra changes reviewed and then we can merge everything

aeneasr · 2025-01-16T11:12:56Z

handler/rfc8628/strategy_hmacsha.go

+}
+
+// ValidateUserCode validates a user_code
+func (h *DefaultDeviceStrategy) ValidateUserCode(ctx context.Context, r fosite.DeviceRequester, code string) error {


code is not being validated here?

aeneasr · 2025-01-16T11:14:36Z

handler/rfc8628/strategy_hmacsha.go

+	if !exp.IsZero() && exp.Before(time.Now().UTC()) {
+		return errorsx.WithStack(fosite.ErrDeviceExpiredToken.WithHintf("User code expired at '%s'.", exp))
+	}
+	return nil


Actually, I think you forgot to validate the code here

nsklikas requested a review from aeneasr as a code owner September 30, 2024 06:50

nsklikas force-pushed the canonical-master branch from 7589ecc to 73d570f Compare September 30, 2024 06:54

nsklikas mentioned this pull request Sep 30, 2024

feat: implement RFC 8628 ory/hydra#3851

Open

7 tasks

aeneasr reviewed Oct 9, 2024

View reviewed changes

go.mod Outdated Show resolved Hide resolved

fosite.go Show resolved Hide resolved

handler/rfc8628/strategy_hmacsha.go Outdated Show resolved Hide resolved

handler/oauth2/flow_authorize_code_token.go Outdated Show resolved Hide resolved

zepatrik requested changes Oct 14, 2024

View reviewed changes

zepatrik reviewed Oct 14, 2024

View reviewed changes

handler/rfc8628/strategy_hmacsha.go Outdated Show resolved Hide resolved

handler/oauth2/flow_generic_code_token.go Outdated Show resolved Hide resolved

nsklikas force-pushed the canonical-master branch 3 times, most recently from 24db6b9 to aa568e6 Compare October 18, 2024 10:09

nsklikas commented Oct 18, 2024

View reviewed changes

handler/rfc7523/handler_test.go Show resolved Hide resolved

nsklikas force-pushed the canonical-master branch from aa568e6 to bf057ec Compare October 18, 2024 14:12

zepatrik reviewed Oct 22, 2024

View reviewed changes

device_write_test.go Outdated Show resolved Hide resolved

device_write_test.go Outdated Show resolved Hide resolved

handler/rfc8628/auth_handler.go Show resolved Hide resolved

handler/rfc8628/storage.go Outdated Show resolved Hide resolved

nsklikas force-pushed the canonical-master branch from bf057ec to 35c89bf Compare October 23, 2024 12:02

nsklikas requested review from aeneasr, J-tt and zepatrik October 29, 2024 08:47

nsklikas force-pushed the canonical-master branch 2 times, most recently from bfa2f4e to 904e20c Compare November 6, 2024 14:17

nsklikas force-pushed the canonical-master branch 2 times, most recently from e5035e8 to 94653ee Compare November 13, 2024 10:57

nsklikas commented Nov 15, 2024

View reviewed changes

handler/openid/flow_device_auth.go Outdated Show resolved Hide resolved

nsklikas force-pushed the canonical-master branch 2 times, most recently from c778674 to ae6e4e3 Compare November 18, 2024 09:33

nsklikas force-pushed the canonical-master branch from ae6e4e3 to a5ad00e Compare December 6, 2024 12:00

nsklikas mentioned this pull request Dec 18, 2024

feat: implement RFC 8628 ory/hydra#3912

Open

7 tasks

nsklikas force-pushed the canonical-master branch from a5ad00e to acb3361 Compare December 18, 2024 11:15

wood-push-melon and others added 23 commits January 7, 2025 16:41

test: reorganize the testcases

3c0e8ae

chore: resolve comments

dcdb5fb

fix: fix oauth2 core storage interface and device flow session type a…

04b0555

…ssertion

fix: implement rate limiting

5999abc

fix: do not validate request when creating response

afcf75a

fix: add the OIDC handler for device flow (#13)

9f547ec

fix: add the OIDC handler for device flow

fix: fix the refresh token issue (#14)

6909e46

* fix: fix the refresh token issue * fix: fix the OIDC token missing issue

fix: use correct grant lifespan to issue tokens

70f92a4

fix: handle the user code generation duplication

66f3167

chore: migrate to uber/gomock

eee50de

refactor: revert oauth handler changes

a215367

ci: use hydra from branch

42b749b

fix: remove rate limiting implementation

75edaee

fix: make user code creation configurable

2fbf264

refactor: simplify handler and test logic

f6778c1

refactor: merge user and device code storage

0d42e95

refactor: enhance deviceRequest struct

c0edacf

fix: do not create openid session on device auth request

7326f8e

feat: implement the access token handling for device authorization flow

aa91dc9

test: check for id and refresh token

1f62d18

chore: migrate to uber/gomock

a59bd1c

fix: delete oidc session when used

c952760

fix: remove rate limiting implementation

9d5065e

nsklikas force-pushed the canonical-master branch from 7309753 to 9d5065e Compare January 7, 2025 15:45

nsklikas requested a review from a team as a code owner January 7, 2025 15:45

chore: update copyright date

349723b

nsklikas requested a review from aeneasr January 7, 2025 16:58

aeneasr reviewed Jan 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement RFC 8628 #826

Implement RFC 8628 #826

nsklikas commented Sep 30, 2024 •

edited

Loading

CLAassistant commented Sep 30, 2024 •

edited

Loading

nsklikas commented Sep 30, 2024

aeneasr left a comment

aeneasr commented Oct 9, 2024

zepatrik left a comment

nsklikas commented Oct 18, 2024

eseliger commented Jan 15, 2025

aeneasr left a comment

aeneasr Jan 16, 2025

aeneasr Jan 16, 2025

Implement RFC 8628 #826

Are you sure you want to change the base?

Implement RFC 8628 #826

Conversation

nsklikas commented Sep 30, 2024 • edited Loading

Related Design Document

Checklist

Further comments

CLAassistant commented Sep 30, 2024 • edited Loading

nsklikas commented Sep 30, 2024

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr commented Oct 9, 2024

zepatrik left a comment

Choose a reason for hiding this comment

nsklikas commented Oct 18, 2024

eseliger commented Jan 15, 2025

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr Jan 16, 2025

Choose a reason for hiding this comment

aeneasr Jan 16, 2025

Choose a reason for hiding this comment

nsklikas commented Sep 30, 2024 •

edited

Loading

CLAassistant commented Sep 30, 2024 •

edited

Loading