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

Improve credential handling #432

Merged
merged 25 commits into from
Nov 27, 2024
Merged

Improve credential handling #432

merged 25 commits into from
Nov 27, 2024

Conversation

mwilde345
Copy link
Member

@mwilde345 mwilde345 commented Nov 22, 2024

Ticket(s): FE-6152

Probably most helpful to look in order:

  • auth/credentials.mjs (middleware that runs buildCredentials)
  • auth/accountKeys.mjs - Makes the AccountKeyStore and adds some methods for refreshing account keys
  • auth/databaseKeys.mjs - Makes DatabaseKeyStore and has refresh methods
  • Other changes are ripping out old authNZ stuff and using the new container.resolve("credentials")

Problem

  • Existing auth middleware does too much, including a preflight api call to fauna and account api.

Solution

  • Similar to dashboard short-lived keys, make them truly just-in-time by handling 401s with a single refresh and retry attempt.

  • Build a singleton Credentials class that gets injected.

    • Tests will continue to mock filesystem and network requests, so shouldn't need any more injectables.
  • Credentials class builds AccountKeys and DatabaseKeys classes.

    • These provide methods to interact with their respective local credential files (AccountKeyStore, DatabaseKeyStore), and also provide helpers to get and refresh keys when necessary
  • Wrap up makeAccountRequest and client.query for the account api and fauna api, respectively

    • Check for 401s and refresh or prompt login

Result

  • DB keys and account keys are refreshed behind the scenes and not before every single command
  • The correct secret and account key are used, considering the various source of input
  • Simple credential argument verification (no database and secret arg together)
  • Commands don't have to worry about creds, they just init the client they need. If they want to interact with credentials, they can.

Example:

async function listDatabases(argv) {
  const logger = container.resolve("logger");

  // query the account api
  const accountClient = new FaunaAccountClient();
  const databases = await accountClient.listDatabases();
  logger.stdout(databases);

  // query the fauna api
  const dbClient = await container.resolve("getSimpleClient")(argv);
  const result = await performQuery(dbClient, "Database.all()", undefined, {
    ...argv,
    format: "json",
  });
  logger.stdout(result);

  // see what credentials are being used
  const credentials = container.resolve("credentials");
  logger.debug(
    `
    Account Key: ${credentials.accountKeys.key}\n
    Database Key: ${credentials.databaseKeys.key}`,
    "creds",
  );
}

Testing

Need to update login test and add new credentials test that verifies middleware sets things up properly

  • Manual testing for now until I refactor the old authNZ test file.
  • Did debug logging to verify that account keys and db keys are refreshed, saved, cleaned up correctly.
  • Verified we don't make duplicate credential classes

@mwilde345 mwilde345 changed the base branch from main to v3 November 22, 2024 22:01
@mwilde345 mwilde345 changed the title refresh credentials during fetch Better credential setup Nov 26, 2024
@mwilde345 mwilde345 changed the title Better credential setup Better credential handling Nov 26, 2024
@mwilde345 mwilde345 changed the title Better credential handling Improve credential handling Nov 26, 2024
@mwilde345 mwilde345 marked this pull request as ready for review November 26, 2024 17:52
@mwilde345 mwilde345 requested a review from a team as a code owner November 26, 2024 17:52
@mwilde345 mwilde345 marked this pull request as draft November 26, 2024 17:55
@mwilde345 mwilde345 requested a review from henryfauna November 26, 2024 21:21
@mwilde345 mwilde345 marked this pull request as ready for review November 26, 2024 21:21
.gitignore Outdated Show resolved Hide resolved
src/lib/auth/credentials.mjs Outdated Show resolved Hide resolved
test/schema/pull.mjs Show resolved Hide resolved
test/shell.mjs Show resolved Hide resolved
Copy link
Contributor

@ecooper ecooper left a comment

Choose a reason for hiding this comment

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

I don't think we need this .fauna files in source control unless I'm missing something.

Otherwise, the biggest thing I notice is this is OO heavy, but on the query side we're more functional + composition. I wouldn't block it now, but we should figure out what our bless pattern is here before release. Otherwise, some of the codebase will be very oo and the rest will be composition and it will be a pain later.

.fauna/credentials/access_keys Outdated Show resolved Hide resolved
src/lib/auth/accountKeys.mjs Outdated Show resolved Hide resolved
@mwilde345 mwilde345 merged commit ecc0524 into v3 Nov 27, 2024
4 checks passed
@mwilde345 mwilde345 deleted the no-preflight branch November 27, 2024 18:45
@cleve-fauna cleve-fauna mentioned this pull request Dec 5, 2024
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.

5 participants