Skip to content

Add key ID to JWT header#208

Merged
aeneasr merged 1 commit intoory:masterfrom
znorris:kid-header
Jan 8, 2024
Merged

Add key ID to JWT header#208
aeneasr merged 1 commit intoory:masterfrom
znorris:kid-header

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

aeneasr
aeneasr reviewed Sep 8, 2022
View reviewed changes
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

cli/cmd/cloudx/e2e/cypress/integration/integration.spec.js

Lines 22 to 28 in d00661e

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:

cli/.github/workflows/test-e2e.yml

Lines 41 to 49 in d00661e

- 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 •
edited
Loading

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
@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

Reviewers

@aeneasr aeneasr aeneasr approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@znorris @aeneasr @adrianrudnik