feat: Possibly pass client in logout request #2483

honzapatCZ · 2021-04-22T08:35:14Z

Related issue

#2468

Proposed changes

I haven't studied the api, but hopefully just extending the response model would work

Checklist

I have read the contributing 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.

CLAassistant · 2021-04-22T08:35:20Z

All committers have signed the CLA.

aeneasr · 2021-04-23T08:01:45Z

internal/httpclient/models/logout_request.go

@@ -22,6 +22,9 @@ type LogoutRequest struct {

 	// RPInitiated is set to true if the request was initiated by a Relying Party (RP), also known as an OAuth 2.0 Client.
 	RpInitiated bool `json:"rp_initiated,omitempty"`
+
+	// client, only set if rp_initiated
+	Client *OAuth2Client `json:"client,omitempty"`


These files are auto-generated, you would need to add that to the actual type ( https://github.com/ory/hydra/blob/master/consent/types.go#L398 ) and of course it also needs to be set and stored!

These files are auto-generated, you would need to add that to the actual type ( https://github.com/ory/hydra/blob/master/consent/types.go#L398 ) and of course it also needs to be set and stored!

OK, the thing is, the client is actually passed, so ory hydra works with it, it just isnt represented in the json form, so I suppose with my latest commit it works now?

No, you still need to set the client somewhere. I suggest adding a test to see if it does what you want!

But it already is set here

hydra/consent/strategy_default.go

Line 914 in 24f91ab

Client: cl,

Ah I see, my bad - these files are auto-generated! What you need to do is to change this line:

hydra/consent/types.go

Line 451 in cf2fe0c

Client *client.Client `json:"-" db:"-"`

This will then update that file automatically! The we also need a test to check if the client is actually set in JSON. For example, by doing an assertion here:

hydra/consent/strategy_logout_test.go

Line 140 in cf2fe0c

require.NotEmpty(t, *v.Payload.RedirectTo)

Ah I see, my bad - these files are auto-generated! What you need to do is to change this line:

hydra/consent/types.go

Line 451 in cf2fe0c

Client *client.Client `json:"-" db:"-"`

This will then update that file automatically! The we also need a test to check if the client is actually set in JSON. For example, by doing an assertion here:

hydra/consent/strategy_logout_test.go

Line 140 in cf2fe0c

require.NotEmpty(t, *v.Payload.RedirectTo)

I already altered the types.go, but where should I put the test? There's also no test that epxects rp_inititated to be true, which is only case where client would be set.

This test is doing rp_initiated:

hydra/consent/strategy_logout_test.go

Line 343 in cf2fe0c

t.Run("case=should pass rp-initiated flows", func(t *testing.T) {

You can add the assertion there!

This test is doing rp_initiated:

hydra/consent/strategy_logout_test.go

Line 343 in cf2fe0c

t.Run("case=should pass rp-initiated flows", func(t *testing.T) {

You can add the assertion there!

Where exactly? I dont see an assert for rp_initiated, so Iam not sure.

aeneasr

Awesome, thank you! 🎉 Your contribution makes Ory better :)

aeneasr

Thank you! This looks better! Could you please add a test to confirm that this is working? The test also must check if client_secret is omitted :) Thank you!

honzapatCZ · 2021-04-26T21:37:50Z

Thank you! This looks better! Could you please add a test to confirm that this is working? The test also must check if client_secret is omitted :) Thank you!

You know I did this change a week ago? Also where should I put the test, there is no test that actually checks for rp_initiated being true, so idk.

aeneasr · 2021-04-26T21:45:04Z

Not sure I understand? I linked the place where the tests needs to be added already, it is here:

hydra/consent/strategy_logout_test.go

Line 343 in cf2fe0c

t.Run("case=should pass rp-initiated flows", func(t *testing.T) {

That test is RP Initiated, the call here

hydra/consent/strategy_logout_test.go

Line 366 in cf2fe0c

body, res := makeLogoutRequest(t, browser, method, url.Values{

so this call

hydra/consent/strategy_logout_test.go

Line 85 in cf2fe0c

resp, err = hc.Get(publicTS.URL + "/oauth2/sessions/logout?" + values.Encode())

which ends up here

hydra/consent/strategy_logout_test.go

Line 140 in cf2fe0c

require.NotEmpty(t, *v.Payload.RedirectTo)

and there you have access to the client. I linked these lines of codes already in my comments:

Sorry, but without tests I can't merge things. This is security software and needs thorough testing and good code quality. I know this might be frustrating to keep working on this but it's just the way this repo needs to work to prevent security issues. For example, I suspect that your change would currently expose the client_secret, so we need to add a test.

Maybe I misunderstand but I believe the instructions to be correct and accurate.

honzapatCZ · 2021-05-09T17:16:53Z

Not sure I understand? I linked the place where the tests needs to be added already, it is here:

hydra/consent/strategy_logout_test.go

Line 343 in cf2fe0c

t.Run("case=should pass rp-initiated flows", func(t *testing.T) {

That test is RP Initiated, the call here

hydra/consent/strategy_logout_test.go

Line 366 in cf2fe0c

body, res := makeLogoutRequest(t, browser, method, url.Values{

so this call

hydra/consent/strategy_logout_test.go

Line 85 in cf2fe0c

resp, err = hc.Get(publicTS.URL + "/oauth2/sessions/logout?" + values.Encode())

which ends up here

hydra/consent/strategy_logout_test.go

Line 140 in cf2fe0c

require.NotEmpty(t, *v.Payload.RedirectTo)

and there you have access to the client. I linked these lines of codes already in my comments:

#2483 (comment)

#2483 (comment)

Sorry, but without tests I can't merge things. This is security software and needs thorough testing and good code quality. I know this might be frustrating to keep working on this but it's just the way this repo needs to work to prevent security issues. For example, I suspect that your change would currently expose the client_secret, so we need to add a test.

Maybe I misunderstand but I believe the instructions to be correct and accurate.

But that is for acceptLogoutRequest iam passing it in getLogoutRequest

aeneasr · 2021-05-13T12:18:48Z

@honzapatCZ I updated the code to unblock you and also help understand what I meant. Check out my commit here: 58ac49a

Hope this helps and sorry again for the frustrating contribution experience. But we got your feature merged! :) Thank you for the help

Closes ory#2468

honzapatCZ · 2021-05-13T12:22:22Z

So it's done and this can be closed?

aeneasr · 2021-05-13T13:21:21Z

No, it's not merged yet. CI is also failing. If you have appetite to look into it, I would appreciate it!

aeneasr · 2021-05-13T13:26:11Z

Found the issue I believe

aeneasr · 2021-05-14T14:03:44Z

🎉 🥳

honzapatCZ changed the title ~~Possibly pass client in logout request~~ feat: Possibly pass client in logout request Apr 22, 2021

aeneasr requested changes Apr 23, 2021

View reviewed changes

aeneasr added the needs more info label Apr 23, 2021

github-actions bot removed the needs more info label Apr 23, 2021

aeneasr approved these changes Apr 26, 2021

View reviewed changes

aeneasr requested changes Apr 26, 2021

View reviewed changes

aeneasr added the pending reply Awaiting reply of author or contributor. Issue will be closed on inactivity. label Apr 29, 2021

svrakitin mentioned this pull request May 4, 2021

fix: Valid JSON response for already handled requests #2517

Merged

7 tasks

honzapatCZ added 2 commits May 13, 2021 14:17

Possibly pass client in logout request

0a66c03

So now the client is passed, hopefully?

ff10209

feat: share client in logout request

e143a3b

Closes ory#2468

aeneasr force-pushed the patch-1 branch from 58ac49a to e143a3b Compare May 13, 2021 12:19

aeneasr removed the pending reply Awaiting reply of author or contributor. Issue will be closed on inactivity. label May 13, 2021

fix: do not panic if client is empty

ef7b15a

aeneasr merged commit 43b391d into ory:master May 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Possibly pass client in logout request #2483

feat: Possibly pass client in logout request #2483

honzapatCZ commented Apr 22, 2021

CLAassistant commented Apr 22, 2021 •

edited

Loading

aeneasr Apr 23, 2021

honzapatCZ Apr 23, 2021

aeneasr Apr 23, 2021

honzapatCZ Apr 23, 2021

aeneasr Apr 25, 2021

honzapatCZ Apr 25, 2021

aeneasr Apr 26, 2021

honzapatCZ Apr 26, 2021

aeneasr left a comment

aeneasr left a comment

honzapatCZ commented Apr 26, 2021

aeneasr commented Apr 26, 2021 •

edited

Loading

honzapatCZ commented May 9, 2021

aeneasr commented May 13, 2021

honzapatCZ commented May 13, 2021

aeneasr commented May 13, 2021

aeneasr commented May 13, 2021

aeneasr commented May 14, 2021

feat: Possibly pass client in logout request #2483

feat: Possibly pass client in logout request #2483

Conversation

honzapatCZ commented Apr 22, 2021

Related issue

Proposed changes

Checklist

CLAassistant commented Apr 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr left a comment

Choose a reason for hiding this comment

honzapatCZ commented Apr 26, 2021

aeneasr commented Apr 26, 2021 • edited Loading

honzapatCZ commented May 9, 2021

aeneasr commented May 13, 2021

honzapatCZ commented May 13, 2021

aeneasr commented May 13, 2021

aeneasr commented May 13, 2021

aeneasr commented May 14, 2021

CLAassistant commented Apr 22, 2021 •

edited

Loading

aeneasr commented Apr 26, 2021 •

edited

Loading