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(platform): List and revoke credentials in user profile #8207

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

kcze
Copy link
Contributor

@kcze kcze commented Sep 27, 2024

Background

Follow up to #8044
Resolves:

Changes 🏗️

Display existing credentials (OAuth and API keys) for all current providers: Google, Github, Notion and allow user to remove them (OAuth is also revoked at providers for Google and GitHub).

  • Add credentials list and Remove button in app/profile/page.tsx
  • Add revoke_tokens abstract method to BaseOAuthHandler and implement it in each provider
  • Revoke OAuth tokens for providers on DELETE /{provider}/credentials/{cred_id}
  • Update autogpt-server-api/baseClient.ts:_request to properly handle empty server responses
  • Ignore google and notion providers at profile until implemented (added TODO)

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/frontend AutoGPT Platform - Front end platform/backend AutoGPT Platform - Back end size/l labels Sep 27, 2024
Copy link

netlify bot commented Sep 27, 2024

Deploy Preview for auto-gpt-docs canceled.

Name Link
🔨 Latest commit f930bf9
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/66f6b7db7d1d120008147f81

Copy link

netlify bot commented Sep 27, 2024

Deploy Preview for auto-gpt-docs canceled.

Name Link
🔨 Latest commit 454b680
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/66fd6b892b7556000870c60a

@CLAassistant
Copy link

CLAassistant commented Sep 30, 2024

CLA assistant check
All committers have signed the CLA.

@kcze kcze marked this pull request as ready for review September 30, 2024 18:32
@kcze kcze requested a review from a team as a code owner September 30, 2024 18:32
@kcze kcze requested review from Torantulino, aarushik93, Pwuts and majdyz and removed request for a team, Torantulino and aarushik93 September 30, 2024 18:32
Copy link
Member

Choose a reason for hiding this comment

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

This will need to be integrated with the credentials manager and its locking mechanism introduced in #8191, depending on which PR gets merged first.

Comment on lines 267 to 263
response_data.detail,
response,
response.status,
response.statusText,
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

afaik response_data.detail == response.statusText for non-ok responses. I've changed it to print response only - that contains everything and const response_data = await response.json(); isn't needed.

Comment on lines 59 to 61
//TODO: Remove this once we have more providers
delete providers["notion"];
delete providers["google"];
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

Comment on lines 68 to 79
{/* <Alert className="mb-2 mt-2">
<AlertDescription>Heads up!</AlertDescription>
<AlertDescription>
<p>
You need to manually remove credentials from the Notion after
deleting them here, see{" "}
</p>
<a href="https://www.notion.so/help/add-and-manage-connections-with-the-api#manage-connections-in-your-workspace">
Notion documentation
</a>
</AlertDescription>
</Alert> */}
Copy link
Member

@Pwuts Pwuts Oct 1, 2024

Choose a reason for hiding this comment

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

I don't think it's scalable to do this here for every single provider that doesn't have a revocation endpoint. A general note saying "Hey, you may also have to delete the connection with AutoGPT in your {provider} settings" would be plenty. Could be here or in the documentation for the platform.

Or raise HTTP 501 in the handler or the endpoint and get the message from there.

Bottom line: minimal differentiation per provider in the front end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I return 501 and displayed information in toast.

Comment on lines +253 to +254
...(hasRequestBody && { "Content-Type": "application/json" }),
...(token && { Authorization: `Bearer ${token}` }),
Copy link
Member

Choose a reason for hiding this comment

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

I didn't know you could do this :)

@kcze kcze requested a review from Pwuts October 2, 2024 15:50
@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label Oct 3, 2024
Copy link
Contributor

github-actions bot commented Oct 3, 2024

This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conflicts Automatically applied to PRs with merge conflicts platform/backend AutoGPT Platform - Back end platform/frontend AutoGPT Platform - Front end size/l
Projects
Status: 🆕 Needs initial review
Development

Successfully merging this pull request may close these issues.

3 participants