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: Do not include nonce in ID tokens when not used #570

Merged
merged 2 commits into from
May 14, 2021

Conversation

mitar
Copy link
Contributor

@mitar mitar commented Mar 14, 2021

Proposed changes

Not sure why was this there, but it makes no sense and there is no point in making ID tokens larger just to include an empty string.

Checklist

  • I have read the contributing guidelines
    and signed the CLA.
  • 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).

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Can you please address the failing tests?

@mitar
Copy link
Contributor Author

mitar commented Apr 8, 2021

Fixed tests.

@aeneasr
Copy link
Member

aeneasr commented Apr 23, 2021

Are you still up for the changes? :) If you need any help, let us know!

@aeneasr
Copy link
Member

aeneasr commented May 11, 2021

Converting to draft due to lack of response

@mitar mitar marked this pull request as ready for review May 13, 2021 07:09
@mitar
Copy link
Contributor Author

mitar commented May 13, 2021

Addressed comments.

@mitar
Copy link
Contributor Author

mitar commented May 13, 2021

I think CI is failing because of invalid configuration, not because of something I would have introduced in this PR.

@aeneasr
Copy link
Member

aeneasr commented May 13, 2021

Possible, I restarted the CI!

@aeneasr
Copy link
Member

aeneasr commented May 13, 2021

Right, we had this issue in another PR. I create a PR in Ory Hydra which bumps the fosite dep to your fork. If the CI passes there, then the one here would also pass!

ory/hydra#2531

@mitar
Copy link
Contributor Author

mitar commented May 13, 2021

It seems it passed.

Could this be fixed in CI somehow? It is pretty ugly that (contributing to) this repository now depends in this way on Hydra's one?

@aeneasr
Copy link
Member

aeneasr commented May 14, 2021

Could this be fixed in CI somehow? It is pretty ugly that (contributing to) this repository now depends in this way on Hydra's one?

done!

@aeneasr aeneasr merged commit 795dee2 into ory:master May 14, 2021
@aeneasr
Copy link
Member

aeneasr commented May 14, 2021

😎

@mitar mitar deleted the patch-2 branch May 14, 2021 18:05
mattmoyer added a commit to mattmoyer/pinniped that referenced this pull request May 28, 2021
The new version has different behavior for the `nonce` claim, which is now omitted if it would be empty (see ory/fosite#570).

Signed-off-by: Matt Moyer <moyerm@vmware.com>
mattmoyer added a commit to mattmoyer/pinniped that referenced this pull request May 28, 2021
The new version has different behavior for the `nonce` claim, which is now omitted if it would be empty (see ory/fosite#570).

Signed-off-by: Matt Moyer <moyerm@vmware.com>
mattmoyer added a commit to mattmoyer/pinniped that referenced this pull request May 28, 2021
The new version has different behavior for the `nonce` claim, which is now omitted if it would be empty (see ory/fosite#570).

Signed-off-by: Matt Moyer <moyerm@vmware.com>
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.

2 participants