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

Add key ID to JWT header #208

Merged
merged 1 commit into from
Jan 8, 2024
Merged

Add key ID to JWT header #208

merged 1 commit into from
Jan 8, 2024

Conversation

znorris
Copy link
Contributor

@znorris znorris commented Sep 7, 2022

Related Issue or Design Document

Relates to issue #144. Being able to lookup the signing key by the key ID is very helpful when using a keystore. This change was originally accepted in PR #145 but was (accidentally?) dropped during some later code reshuffling.

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

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! Sorry that this got lost. If you want, you can also add a test for this so that it never regresses again!

@znorris
Copy link
Contributor Author

znorris commented Sep 8, 2022

That's a good idea. @aeneasr Is there an existing test you might point me to as an example?

@aeneasr
Copy link
Member

aeneasr commented Sep 9, 2022

expect(res.body.headers["Authorization"]).to.not.be.empty
cy.task(
"verify",
res.body.headers["Authorization"].replace(/bearer /gi, ""),
).then((decoded) => {
expect(decoded.session.identity.traits.email).to.equal(email)
})

Has the code which verifies the JWT, in here you could probably add an assertion for it :)

Here's the script to run the tests:

- run: |
cd cmd/cloudx/e2e
npm ci
go build -tags sqlite -o ory .
- name: Run Proxy E2E Tests
run: |
cd cmd/cloudx/e2e
./ory proxy https://httpbin.org/anything --project playground &
npm run test

@adrianrudnik
Copy link

adrianrudnik commented Apr 8, 2023

Just tried to do the tests in another branch, as this seems to be a stale PR right now, but the feature is still missing.

Failed to go any further: Seems like I need a project set up for tests only and npm run test just fails with You passed the --record flag but did not provide us your Record Key.. Digging myself through the github workflows did not help either. Could not find any page dedicated on test setup or any Makefile step, so you could just do a local test run to confirm the changes, not depending on anything.

Any advice?

@aeneasr aeneasr merged commit dcdc666 into ory:master Jan 8, 2024
1 check passed
@znorris znorris deleted the kid-header branch April 10, 2024 04:53
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.

3 participants