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

Credentials Middleware tests #461

Merged
merged 14 commits into from
Dec 5, 2024
Merged

Credentials Middleware tests #461

merged 14 commits into from
Dec 5, 2024

Conversation

mwilde345
Copy link
Member

@mwilde345 mwilde345 commented Dec 4, 2024

Ticket(s): FE-6174

Problem

No tests for the credentials middleware

Solution

  1. Test that the Credentials class has the correct values set right off the bat, before any network calls, considering the args and the local filesystem
  2. Test that DB keys get refreshed correctly
  3. Test that Account keys get refreshed correctly / or you get prompted for login

Result

Test coverage

Testing

Suite passes

@mwilde345 mwilde345 changed the base branch from main to v3 December 4, 2024 18:28
@mwilde345 mwilde345 changed the title creds tests Credentials Middleware tests Dec 4, 2024
@mwilde345 mwilde345 marked this pull request as ready for review December 4, 2024 22:39
@mwilde345 mwilde345 requested a review from a team as a code owner December 4, 2024 22:39
Comment on lines +25 to +29
accountKey: {
type: "string",
description: "The account key to use when calling Fauna",
required: false,
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

when are customers going to want/need to use this flag? for operations vs frontdoor?

Copy link
Member Author

Choose a reason for hiding this comment

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

We called it out in the design phase as something that can be set by the user. I don't know the use case yet, but I know we have a UI page to allow users to create one of these keys so who knows how they might want to use those.

Copy link
Contributor

Choose a reason for hiding this comment

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

We also said we want flag parity for everything. It's probably the "right" thing to do.

It would allow you to use the cli even if frontdoor login was failing but the rest of frontdoor was okay, right?

Comment on lines +281 to +302
it("refreshes database key", async () => {
v10runQueryFromString
.onCall(0)
.rejects({
error: {},
httpStatus: 401,
})
.onCall(1)
.resolves({
data: [],
});
makeAccountRequest
.withArgs(
sinon.match({
path: sinon.match(/\/databases\/keys/),
method: "POST",
}),
)
.resolves({
secret: "new-secret",
});
await run(`query "Database.all()" -d us-std --no-color`, container);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is a pretty cool test :)

Comment on lines +251 to +253
// We need to use the original implementation of runQueryFromString to ensure it hits
// faunaClientV10.runQueryFromString which is where we force the 401 and test the refresh
// logic.
Copy link
Collaborator

Choose a reason for hiding this comment

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

good comment

Comment on lines +32 to +44
// Instead of mocking `fs` to return certain values in each test, let the middleware
// read files in the actual filesystem.
const setCredsFiles = (accessKeys, secretKeys) => {
fs.mkdirSync(credsDir, { recursive: true });
fs.writeFileSync(
`${credsDir}/access_keys`,
JSON.stringify(accessKeys, null, 2),
);
fs.writeFileSync(
`${credsDir}/secret_keys`,
JSON.stringify(secretKeys, null, 2),
);
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

:(

how much effort would it be to mock out these calls and have this function change the mock behaviors?

Copy link
Member Author

@mwilde345 mwilde345 Dec 5, 2024

Choose a reason for hiding this comment

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

Could be a decent chunk. Idea here was "set and forget" and let the middleware get and save as often as it needs to. Then at the end of the test I could assert that a certain key was used, knowing things were persisted correctly along the way. (e.g. line 311 in this file, seeing as getOrRefreshKey first goes to keyStore.get())

I wanted to avoid a function here that worries about fs.readFileSync.onCall(x) -> spit out the new key instead of old one etc.

Might be overthinking it :P

@mwilde345 mwilde345 merged commit d52a149 into v3 Dec 5, 2024
4 checks passed
@mwilde345 mwilde345 deleted the creds-tests branch December 5, 2024 18:52
This was referenced Dec 6, 2024
@cleve-fauna cleve-fauna mentioned this pull request Dec 13, 2024
@mwilde345 mwilde345 mentioned this pull request Dec 18, 2024
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