FE-6296 Enable scoped database and role with user-provided keys#558
Merged
ptpaterson merged 12 commits intomainfrom Jan 17, 2025
Merged
FE-6296 Enable scoped database and role with user-provided keys#558ptpaterson merged 12 commits intomainfrom
ptpaterson merged 12 commits intomainfrom
Conversation
ecooper
reviewed
Jan 13, 2025
ecooper
approved these changes
Jan 14, 2025
wildemat
approved these changes
Jan 17, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
You can provide a scoped key by specifying
--secret some_key:child/db:role, but sometimes it is more convenient to split up the auth config into--secret some_key -d child/db -r role. This is especially true if you have a config profile that provides a secret that you sometimes want to scope down, e.g.-p test-db -d child.Solution
Add middleware that mutates a secret provided with
--secret(or whatever secret is set if added by prior middleware, such are --local) to scope the key with the provided database and/or role.In the case of using
--localor thelocalcommand, we can remove the special handing of secret, role and database there, since it will be handled for all cases by the new middleware.Result
For all commands that accept
--secret,--databaseand--roleoptions can now also be provided.Use of credentials files
Using the user-provided secret will now avoid digging through file-based database keys before falling back on the provided secret. To do this, I updated the
getSecret()function to acceptargvand short circuit ifargv.secretalready exists. It was brought up that thegetSecret()function is already a bit of a hack, but I kept this function in place to reduce code duplication: the alternative I saw was to replacegetSecret()withargv.secret ?? (await getSecret())everywhere, and that seemed to add more complexity. I still moved the key-refreshing bit into a class method to keep those details out of the logic that forks at provided key vs credentials file key.The
Credentialssingletons are still constructed with the container initialization, so we are not avoiding it entirely. Additionally, the check for whether or not to retry 401s with a refreshed key goes throughCredentials(seeDatabaseKeys.onInvalidCreds()method). A future optimization might be to not initialize the credentials files with the container, but perform the file I/O when called for and memoize for additional calls. But I was trying to avoid anything that invasive with this PR.Example
Testing
Added some new tests to cover the
scopeSecretmiddleware. This is really just taking over the existing tests that were covered byapplyLocalArgmiddleware before.