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: create an admin return all agents route #1620

Merged
merged 3 commits into from
Aug 13, 2024

Conversation

4shub
Copy link
Collaborator

@4shub 4shub commented Aug 6, 2024

Please describe the purpose of this pull request.
Attempts to address this issue by adding a new endpoint that will return all agents, regardless of user behind an /api/admin endpoint.

How to test
You can pull down the code and run this command to test:

curl http://localhost:4200/api/admin/agents  -H "Authorization: Bearer $ADMIN_PASSWORD"

Have you tested this PR?
Yes

Related issues or PRs
[Please link any related GitHub issues or PRs.
](#1522)

Is your PR over 500 lines of code?
No

Additional context
Add any other context or screenshots about the PR here.

@4shub 4shub marked this pull request as ready for review August 6, 2024 19:07
@cpacker cpacker self-requested a review August 7, 2024 04:05
Copy link
Collaborator

@cpacker cpacker left a comment

Choose a reason for hiding this comment

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

LGTM

@cpacker cpacker self-requested a review August 7, 2024 04:06
Copy link
Collaborator

@cpacker cpacker left a comment

Choose a reason for hiding this comment

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

Actually sorry can you run the linter code first? Looks like that test is failing, otherwise the main logic LGTM though.

You can run the linter via precommit, see OP in this PR:
#1300

@norton120
Copy link
Collaborator

design note - typically the way you'd want to handle this is via auth. an admin auth token would return all agents on list, whereas a normal user auth token would return the agents scoped to them. then for list queries if it takes query strings the admin would be able to do query?user=user-xxxx-xxxx-xxxx-xxxx in the get request and scope it to the user. this keeps your API to CRUD + list instead of building one-off endpoints for use cases.

so maybe a one-off for now, but should definitely consider auth-scoped list endpoints to replace it

@4shub
Copy link
Collaborator Author

4shub commented Aug 7, 2024

design note - typically the way you'd want to handle this is via auth. an admin auth token would return all agents on list, whereas a normal user auth token would return the agents scoped to them. then for list queries if it takes query strings the admin would be able to do query?user=user-xxxx-xxxx-xxxx-xxxx in the get request and scope it to the user. this keeps your API to CRUD + list instead of building one-off endpoints for use cases.

so maybe a one-off for now, but should definitely consider auth-scoped list endpoints to replace it

I am not sure if I agree here; the /api/admin/agents route while returning a list of all agents, is providing a different expected result than /api/agents based on your authentication token. If a non-admin user were to access /api/agents and were to receive nothing or a 403 but receive data when providing their own user id; then it's a standard crud either as the effect changes based on properties of header and not the url value itself.

If you wanted an alternative crud solution I'd say you want to map your endpoints like this:

/api/(<user-id> | self)/agents to return agents of a specific user
/api/admin/agents to return all agents, this endpoint only available to admin accounts

Then we can add our query params to the end of it for filtering

I think overall we should probably do a bit of restful improvement here though I agree!

@cpacker cpacker self-requested a review August 13, 2024 18:59
@cpacker cpacker merged commit 39db293 into letta-ai:main Aug 13, 2024
9 of 11 checks passed
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.

3 participants