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

feat(backend): credential swap to api keys in cred store #8403

Merged

Conversation

ntindle
Copy link
Member

@ntindle ntindle commented Oct 23, 2024

Background

Many of our blocks use api keys to authenticate with various services. We want those to no longer be stored in the graph on a block level, but as a reference to a credential

Changes 🏗️

//Backend

  • Swaps many blocks, but not all (LLM blocks mostly due to needing to select from many valid credentials of different providers), to the new format of credentials
  • Drops discord continuous mode as a side effect, approved by Toran
  • uses the store to load default set credentials for API keys injected by the system
  • Uses the credit system to detect and charge for injected API keys

// Frontend

  • Updates the way we handle providers in the front end to deduplicate the keys for providers we have

Testing 🔍

Note

Only for the new autogpt platform, currently in autogpt_platform/

  • Create from scratch and execute an agent with at least 3 blocks
  • Import an agent from file upload, and confirm it executes correctly
  • Upload agent to marketplace
  • Import an agent from marketplace and confirm it executes correctly
  • Edit an agent from monitor, and confirm it executes correctly

@github-actions github-actions bot added platform/backend AutoGPT Platform - Back end platform/blocks size/l and removed platform/backend AutoGPT Platform - Back end platform/blocks labels Oct 23, 2024
@github-actions github-actions bot added platform/backend AutoGPT Platform - Back end platform/blocks labels Oct 23, 2024
@github-actions github-actions bot added the platform/frontend AutoGPT Platform - Front end label Oct 23, 2024
@ntindle ntindle marked this pull request as ready for review October 23, 2024 19:37
@ntindle ntindle requested a review from a team as a code owner October 23, 2024 19:37
@ntindle ntindle requested review from Pwuts and aarushik93 and removed request for a team October 23, 2024 19:37
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 Security concerns

Sensitive information exposure:
In several files, the API keys are being converted from SecretStr to string when used in API calls. For example, in 'autogpt_platform/backend/backend/blocks/ai_shortform_video_block.py' on line 211, api_key.get_secret_value() is used. This could potentially expose the API key if not handled carefully. It's important to ensure that these secret values are not logged or exposed in any way throughout the application.

⚡ Recommended focus areas for review

Potential Security Risk
The API key is now stored as a SecretStr, but it's being converted to a string when used. This could potentially expose the key if not handled carefully.

Unused Import
The BlockSecret and SecretField are imported but not used in the updated code. These imports should be removed.

Incomplete Icon Mapping
Some providers are using a fallback icon. Consider adding specific icons for these providers to improve user experience.

@ntindle ntindle requested a review from Bentlybro October 24, 2024 16:02
@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label Oct 24, 2024
@aarushik93 aarushik93 requested a review from a team as a code owner October 31, 2024 02:24
this is only for making sure stuff keeps wokrin
@aarushik93 aarushik93 enabled auto-merge (squash) October 31, 2024 02:25
…entials' of https://github.com/Significant-Gravitas/AutoGPT into ntindle/secrt-938-update-all-api-keys-in-blocks-to-credentials
aarushik93
aarushik93 previously approved these changes Oct 31, 2024
@aarushik93 aarushik93 merged commit 3aebed6 into dev Oct 31, 2024
11 checks passed
@aarushik93 aarushik93 deleted the ntindle/secrt-938-update-all-api-keys-in-blocks-to-credentials branch October 31, 2024 02:36
github-merge-queue bot pushed a commit that referenced this pull request Nov 29, 2024
…kend (#8648)

- Move `autogpt_libs.supabase_integration_credentials_store` into
`backend`
   - `.store` -> `backend.integrations.credentials_store`
   - `.types` -> added to `backend.data.model`
- Rename `SupabaseIntegrationCredentialsStore` to
`IntegrationCredentialsStore`

We wanted to get a few security things in quickly in #8403 and had to
make some compromises to do so. This picks those up and fixes them.

- Resolves #8540

### Checklist 📋

#### For code changes:
- [x] I have clearly listed my changes in the PR description
- [x] I have made a test plan
- [x] I have tested my changes according to the test plan:
  <!-- Put your test plan here: -->

---------

Co-authored-by: Reinier van der Leer <[email protected]>
Co-authored-by: Aarushi <[email protected]>
aarushik93 added a commit that referenced this pull request Dec 1, 2024
…kend (#8648)

- Move `autogpt_libs.supabase_integration_credentials_store` into
`backend`
   - `.store` -> `backend.integrations.credentials_store`
   - `.types` -> added to `backend.data.model`
- Rename `SupabaseIntegrationCredentialsStore` to
`IntegrationCredentialsStore`

We wanted to get a few security things in quickly in #8403 and had to
make some compromises to do so. This picks those up and fixes them.

- Resolves #8540

### Checklist 📋

#### For code changes:
- [x] I have clearly listed my changes in the PR description
- [x] I have made a test plan
- [x] I have tested my changes according to the test plan:
  <!-- Put your test plan here: -->

---------

Co-authored-by: Reinier van der Leer <[email protected]>
Co-authored-by: Aarushi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants