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: python client scope field type #223

Closed
wants to merge 1 commit into from
Closed

fix: python client scope field type #223

wants to merge 1 commit into from

Conversation

SavvasMohito
Copy link

Proposed fix for #220

As described in the issue post, after the update to version 2.0.1, the hydra python client had a misconfiguration which caused the token change flow not to work at all. This had to do with the type of the "scope" value the client expects to receive from hydra.

Based on the documentation and the OAuth2 standard, the scope field should be a string value containing space-separated entries. Instead, the new client update proposed it to have a value of int, thus causing the client not to work properly with hydra because of this type mismatch.

Tests

I have applied this proposed change on my local hydra python client and I can confirm it works as expected with hydra v2.0.1.

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).

@CLAassistant
Copy link

CLAassistant commented Nov 3, 2022

CLA assistant check
All committers have signed the CLA.

@SavvasMohito SavvasMohito changed the title fix: python client scope field type (#220) fix: python client scope field type Nov 3, 2022
@jonas-jonas
Copy link
Member

Hi @SavvasMohito, thanks for your contribution! Unfortunately, these files are automatically generated from our OpenAPI schemas. So your change would be overwritten the next time our generation is run.

We need to investigate why the generator outputs the wrong type here and adjust either 1) our schema or 2) fix the generator. My guess is, that we should be able to adjust our schema accordingly, but that needs to be done in https://github.com/ory/hydra.

The definition in hydra is here: https://github.com/ory/hydra/blob/925013e39508c22d1038560ef552a0764f2b0177/oauth2/handler.go#L827
It looks like int is the expected type, so we need to figure out why the SDK tries to input a str instead. If you're up for the task, feel free to contribute in the hydra repository. :)

@aeneasr
Copy link
Member

aeneasr commented Nov 3, 2022

Oh the problem is that

https://github.com/ory/hydra/blob/925013e39508c22d1038560ef552a0764f2b0177/oauth2/handler.go#L827

should actually be a string, not an int!

@SavvasMohito
Copy link
Author

SavvasMohito commented Nov 3, 2022

Well, I guess we finally found the root of the problem, good call @jonas-jonas! As @aeneasr said, this should definitely be a string and not an integer value. I assume you guys are going to fix this, but let me know if you'd prefer I make a PR for that as well. Thank you for looking into this.

@jonas-jonas
Copy link
Member

Feel free to open a PR in hydra! :) Contributions are always welcome.

@SavvasMohito
Copy link
Author

PR created under the hydra repo
ory/hydra#3337

aeneasr pushed a commit to ory/hydra that referenced this pull request Nov 3, 2022
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