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 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 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!

aeneasr · 2024-10-09T12:10:26Z

go.mod

-go 1.20
+go 1.21
+
+toolchain go1.21.4


Is 1.21 needed?

It was required for the update of ory/x, I will roll it back to the lateset version that supports 1.20.

fosite.go

aeneasr · 2024-10-09T12:20:59Z

handler/rfc8628/strategy_hmacsha.go

+	key := code + "_limiter"
+
+	keyBytes := []byte(key)
+	object, err := h.RateLimiterCache.Get(keyBytes)


This won't work in real world scenarios. The cache is in memory which means:

It won't work if more than one instance (which is almost always the case) of the library/hydra running

It won't persist across restarts

In my view, rate limiting should be the concern of the firewall/ingress/reverse proxy and I think we could simplify this PR a bit by removing this part or making it optional and/or more generic

Agreed on all your points. The reason we included this change is to be spec compliant and the reason we didn't implement a more sophisticated solution (which would probably require an external caching system) is that rate limiting should be handled by the ingress/firewall. But looking back at it, it is probably better to leave it unhandled, instead of having partly correct logic.

It is probably better to remove this and leave it as future work.

aeneasr · 2024-10-09T12:22:22Z

handler/oauth2/flow_authorize_code_token.go

@@ -5,197 +5,99 @@ package oauth2



This diff is quite challenging to read, would it be possible to reduce the refactoring and separating into different files? Since this is security relevant code, the smaller the diff the faster the review :)

We will try to revert the refactored code and push the changes in the coming days

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

zepatrik · 2024-10-21T15:39:53Z

device_write_test.go

+	resp.SetUserCode("AAAA")
+	resp.SetDeviceCode("BBBB")
+	resp.SetInterval(int(
+		oauth2.Config.GetDeviceAuthTokenPollingInterval(context.TODO()).Round(time.Second).Seconds(),


I'd suggest to add a simple

ctx := context.Background()

in the beginning of each test, and use that as the context everywhere.

device_write_test.go

handler/rfc8628/auth_handler.go

handler/rfc8628/storage.go

handler/openid/flow_device_auth.go

… auth/device code is expired

…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!

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

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

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 25 commits January 7, 2025 16:41

fix: passing the correct authorization request when validating if the…

3dfb6bd

… auth/device code is expired

feat: error handling for authorization pending in device flow

d4c0bc4

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

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 Oct 9, 2024

nsklikas Oct 17, 2024

aeneasr Oct 9, 2024

nsklikas Oct 14, 2024

aeneasr Oct 9, 2024

nsklikas Oct 14, 2024

aeneasr commented Oct 9, 2024

zepatrik left a comment

nsklikas commented Oct 18, 2024

zepatrik Oct 21, 2024

eseliger commented Jan 15, 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 Oct 9, 2024

Choose a reason for hiding this comment

nsklikas Oct 17, 2024

Choose a reason for hiding this comment

aeneasr Oct 9, 2024

Choose a reason for hiding this comment

nsklikas Oct 14, 2024

Choose a reason for hiding this comment

aeneasr Oct 9, 2024

Choose a reason for hiding this comment

nsklikas Oct 14, 2024

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

zepatrik Oct 21, 2024

Choose a reason for hiding this comment

eseliger commented Jan 15, 2025

nsklikas commented Sep 30, 2024 •

edited

Loading

CLAassistant commented Sep 30, 2024 •

edited

Loading