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

fix: Fixed the missing state check for hybrid flow #670

Merged
merged 10 commits into from
Aug 1, 2022
Merged

fix: Fixed the missing state check for hybrid flow #670

merged 10 commits into from
Aug 1, 2022

Conversation

harjoben
Copy link
Contributor

Related Issue or Design Document

#669 has the details of the bug

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 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 necessary documentation within the code base (if
    appropriate).

Further comments

@harjoben harjoben requested a review from aeneasr as a code owner May 12, 2022 17:03
@CLAassistant
Copy link

CLAassistant commented May 12, 2022

CLA assistant check
All committers have signed the CLA.

@harjoben
Copy link
Contributor Author

@aeneasr The tests here are failing because they are being run in a go1.14 environment. The Has() method for url.Values is only available from go1.17 onwards. Do the tests need to be updated to use the latest go version? Or I should update my fix to not use the Has() method?

@james-d-elliott
Copy link
Contributor

james-d-elliott commented May 13, 2022

@aeneasr The tests here are failing because they are being run in a go1.14 environment. The Has() method for url.Values is only available from go1.17 onwards. Do the tests need to be updated to use the latest go version? Or I should update my fix to not use the Has() method?

Probably the most accessible option (not requiring go 1.17) would be to do something along the lines of this since the url.Values are just a map[string][]string:

	if _, ok := resp.GetParameters()["state"]; !ok {
		resp.AddParameter("state", ar.GetState())
	}

Edit: Keep in mind I can't really speak on anyone behalf of anyone else.

@aeneasr
Copy link
Member

aeneasr commented May 14, 2022

We can move to Go 1.17 :)

@harjoben
Copy link
Contributor Author

We can move to Go 1.17 :)

@aeneasr How do we go about doing that? I don't believe I have access to update the circleci tests running for this PR.

@harjoben
Copy link
Contributor Author

harjoben commented Jun 7, 2022

@aeneasr I believe Go was upgraded already. I don't have permissions to re-run the checks for this PR. Can you help me do that?

@james-d-elliott
Copy link
Contributor

james-d-elliott commented Jun 7, 2022

@aeneasr I believe Go was upgraded already. I don't have permissions to re-run the checks for this PR. Can you help me do that?

If you merge master into your local branch and push that should trigger a rebuild.

@aeneasr
Copy link
Member

aeneasr commented Jun 21, 2022

@harjoben can you add a test that fails the current code on master and passes with the changes you've made? Thank you!

@aeneasr aeneasr self-requested a review July 4, 2022 10:44
@aeneasr
Copy link
Member

aeneasr commented Jul 4, 2022

@harjoben are you still up for the changes?

@harjoben
Copy link
Contributor Author

harjoben commented Jul 5, 2022

@harjoben are you still up for the changes?

@aeneasr Yes, I will make the changes. Give me some time. Apologies for dragging this so long

@aeneasr
Copy link
Member

aeneasr commented Jul 5, 2022

no problem :)

@harjoben
Copy link
Contributor Author

@aeneasr I have added the relevant test.

@james-d-elliott
Copy link
Contributor

james-d-elliott commented Jul 27, 2022

Can confirm the test fails before the fix (as expected), have not checked post fix as I'm assuming circleci will fail the check if it doesn't.

@aeneasr aeneasr merged commit 37f8a0a into ory:master Aug 1, 2022
@aeneasr
Copy link
Member

aeneasr commented Aug 1, 2022

thank you!

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.

None yet

4 participants