Allows for shared, default API keys. #399
Conversation
docker-compose.yml
Outdated
| @@ -26,6 +26,8 @@ services: | |||
| - DATABASE_URL=postgres://app:secret@postgres/app_development | |||
| - DEV_HOST=${DEV_HOST:-localhost} # Set if you want to use a different hostname | |||
| - ALLOWED_REQUEST_ORIGINS=${ALLOWED_REQUEST_ORIGINS} | |||
| - DEFAULT_OPENAI_KEY={DEFAULT_OPENAI_KEY} | |||
| - DEFAULT_ANTHROPIC_KEY={DEFAULT_ANTHROPIC_KEY} | |||
There was a problem hiding this comment.
I believe this needs $ it front of the curlies. At the last the one above it has that
There was a problem hiding this comment.
While you're adding these, can you do me a favor and make sure every Setting ENV is listed here? We're missing a few.
Also, add a comment such as to the YML
environment: -- Be sure to add this environment variable to options.yml
And do the reverse too, in the options.yml file on the settings line do:
settings: -- Be sure to add these ENV to docker-compose.yml
This project is getting nuanced enough it's even hard for me to remember all the subtle connections so sprinkling these hints will help future me (and you) :)
There was a problem hiding this comment.
@matthewbennink Thank you for tackling this one. I think it's sufficient for a v1 but can we implement slightly differently:
- Having the conditional sprinkled throughout the app will inevitably break, especially with the various contributors we have let's instead override it at the model level then it'll "just work". So inside user.rb do
def openai_key; self.attributes["openai_key"] || ... - The one problem with this strategy is that a user can go to their settings and they'll be able to steal the API key so the way you can fix this is simply
settings/people/formand explicitly adding the value to the text field, like this:<%= user_fields.text_field :openai_key, value: person.user.attributes["openai_key"], ...Now it will never pre-populate with the fallback value, it will only pre-populate with a user-specific value. AND users will still be able to override the default with their own key. - Let's add a Feature for this, in addition to a setting. Meaning, in the options.yml file we can call the feature boolean:
default_llm_keys, defaulting to false, and then add two Settings in here and then add the corresponding settings keys (default_openai_key, ...) - No where in the app code should you reference ENV..., instead always reference Setting.default_openai_key. This makes it so that users can configure settings with
bin/rails credentials:editor with ENV variables
While you're at it, can you do me a favor... :)
- We should follow this same pattern with ALLOWED_REQUEST_ORIGINS so add a Setting for that and update the code to use Setting.allowed_request_origins instead of ENV...
This reverts commit b201b25.
|
@krschacht - I've made the requested changes. I wasn't sure if we needed the |
| test "it treats blank String API keys as nil to allow for default keys" do | ||
| stub_features(default_llm_keys: false) do | ||
| user = users(:keith) | ||
| user.update(openai_key: '', anthropic_key: ' ') |
There was a problem hiding this comment.
@matthewbennink after merging in I noticed this is missing a bang. The update can fail and then this test mistakenly passes. Years ago I automatized almost always doing using bang on update and save since we typically expect it to succeed and want it to fail loudly if it doesn't (and controllers handle exceptions gracefully).
I'll quickly fix this in main, so just an FYI for you
If either of the default ENV keys are set, then the user will have access without needing to provide their own API key(s). I haven't added any tests and this doesn't protect anyone who adds these ENV variables while also allowing users to freely register. Is this simple enough?
Would it be worth adding anything to the README or might it be better to wait until the "feature" is more mature, perhaps allowing for the secrets to be managed within the database, for instance, since that would better support other LLMs outside of OpenAI and Anthropic.
Relates to #236.