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

Enhance API Key Validation and Admin Access Control #224

Merged
merged 8 commits into from
Feb 3, 2025

Conversation

movchan74
Copy link
Contributor

@movchan74 movchan74 commented Jan 29, 2025

  • Improved API key validation with better error handling
  • Introduced admin access requirements for certain endpoints
  • Introduced two new options for user-defined endpoints:
    • admin_required: Flag indicating if the endpoint requires admin access.
    • always_defer: Flag indicating if the endpoint should always defer execution to the task queue.
  • Added configuration options for enabling or disabling the OpenAI-compatible chat endpoint.

Aleksandr Movchan added 7 commits January 29, 2025 13:42
- Added ApiKeyValidationFailed exception for better error handling during API key validation.
- Updated api_key_check middleware to exclude certain paths from API key checks.
- Modified status endpoint to require admin access.
- Introduced is_admin field in ApiKeyEntity to manage admin user permissions.
- Introduced admin_required property for user defined endpoints.
@movchan74 movchan74 changed the base branch from api_service_task_queue to main January 29, 2025 16:39
@movchan74 movchan74 requested a review from HRashidi January 29, 2025 16:47
@movchan74 movchan74 marked this pull request as ready for review January 29, 2025 16:47
Copy link
Contributor

@HRashidi HRashidi left a comment

Choose a reason for hiding this comment

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

Looks good.
As you added a attr like always_defer, maybe it is a good idea to generalize it to be an enum of always, 'neverandoptional`. because for some endpoints the user should not be able to defer the requests. like list of collections which we would like to add it as a task!

also there is a get method for delete task. I am not sure if it will break backward compatibility if anyone is using our sdk, but it is good to change the method to @app.delete(...)

@movchan74
Copy link
Contributor Author

Looks good. As you added a attr like always_defer, maybe it is a good idea to generalize it to be an enum of always, 'neverandoptional`. because for some endpoints the user should not be able to defer the requests. like list of collections which we would like to add it as a task!

also there is a get method for delete task. I am not sure if it will break backward compatibility if anyone is using our sdk, but it is good to change the method to @app.delete(...)

Good idea! I replaced it with a generalized option defer_option.

Regarding the task management endpoints, I think we should do it as a separate PR. We need to add proper CRUD operations for the tasks. There is even an issue for that #177. I don't know if we want to use REST for it though.

@movchan74 movchan74 requested a review from HRashidi January 31, 2025 09:47
@movchan74 movchan74 merged commit ae08b1d into main Feb 3, 2025
9 checks passed
@movchan74 movchan74 deleted the admin_only_endpoints branch February 3, 2025 14:59
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.

2 participants