-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat(api): Check if JWT is revoked before allowing access #4353
Conversation
70614c7
to
3666c12
Compare
Removed vultr server and associated DNS entries |
91bf522
to
5c58be0
Compare
5c58be0
to
56afe71
Compare
This is required as there's now an addtional query via .client.request which wasn't previously mocked
56afe71
to
b9151e0
Compare
@@ -12,13 +13,18 @@ const mockGenerateCSVData = vi.fn().mockResolvedValue([ | |||
metadata: {}, | |||
}, | |||
]); | |||
vi.mock("@opensystemslab/planx-core", () => { | |||
|
|||
vi.mock("@opensystemslab/planx-core", async (importOriginal) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern is repeated a few times here, grouped in b1d6037
Now that JWT verification needs to make a round trip to the DB, this test needs to access a GraphQL client to do this ($api.client.request
) which is not mocked. Using importOriginal()
we can only mock the function we want to mock in this test suite and keep the rest of the original behaviour. In this case it's this.export.csvData
.
We have an open issue to consolidate these mocks and to make them easier and simpler to manage - the pattern here could be DRY'ed up a but into a setupPlanXCoreMock()
function called by vitest setup. I've not tackled this here in order to make process and keep things separate. The issue is here - #2404
.then((res) => { | ||
expect(res.body).toEqual({ | ||
error: "No authorization token was found", | ||
describe("authentication and error handling", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another common pattern here is just tighter closures for mocks - a lot were failing because we were relying on queryMock.reset()
to manage scope instead of describe()
blocks - this meant that the mock query for a non-revoked JWT was then being cleared, and the tests were failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for super clear testing instructions here, all works as expected for me repeated over a couple times 🙌
|
||
beforeEach(() => { | ||
queryMock.setup(process.env.HASURA_GRAPHQL_URL!); | ||
queryMock.mockQuery(mockIsTokenRevokedQuery); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
beautiful! such thorough work on the tests.
also I tested as you suggested and it works like a dream! ✨
What does this PR do?
revoked_tokens
table by using theisRevoked
callback fromexpress-jwt
queryMock
is instantiated means we don't have to add this in multiple places.describe()
blocks - please see comment on PR belowNot yet covered
Requests made directly to Hasura with an existing token (not proxied via API).
To test...
revoked_tokens
tableScreen.Recording.2025-02-21.at.13.16.13.mov