[FEAT] Add (limited) API endpoint #237
Conversation
…dpoint documentation
tchapi
left a comment
tchapi
left a comment
There was a problem hiding this comment.
Hi @unkn0wnAPI 👋🏼
First of all, thanks for your interest and the work you put in adding this feature.
I need to get my head around it and I lack some time right now, but I'll probably be able to dedicate more focus in the coming week. Still, I've added a few comments on the code so we can move forward.
Apart from them, an important point would be on the API authentication. I'd rather use a standard Symfony protocol than custom checks in the controller (maybe JSON-login, tied to the admin user or to each user, or a custom authenticator that indeed uses an API-key header).
Happy to discuss, and thanks again!
src/Controller/Api/ApiController.php
Outdated
| if (!$principals) { | ||
| return $this->json(['status' => 'success', 'data' => []], 200); | ||
| } |
| } | ||
|
|
||
| foreach ($principals as $principal) { | ||
| $users[] = [ |
There was a problem hiding this comment.
nit: you should declare $users = [] before the loop
src/Controller/Api/ApiController.php
Outdated
|
|
||
| $response = [ | ||
| 'status' => 'success', | ||
| 'data' => $data ?? [], |
| if (!$user) { | ||
| return $this->json(['status' => 'success', 'data' => []], 200); | ||
| } |
There was a problem hiding this comment.
At first I thought that in theory no data return on valid API request is valid output, but you are right, it is better to return 404 to user for better DX.
Fixed in commit 0c28377
src/Controller/Api/ApiController.php
Outdated
| if (empty($username) || !is_string($username) || preg_match('/[^a-zA-Z0-9_-]/', $username)) { | ||
| return $this->json(['status' => 'error', 'message' => 'Invalid Username'], 400); | ||
| } |
There was a problem hiding this comment.
This should be a helper, maybe validateUserName() ?
There was a problem hiding this comment.
Moreover it could probably be done with a route requirement
There was a problem hiding this comment.
I've added both helper function (to be used if it's required in the future) and moved the username check to route requirement.
Fixed in commit: d8fc2e9
src/Controller/Api/ApiController.php
Outdated
| if (!$allCalendars && !$subscriptions) { | ||
| return $this->json(['status' => 'success', 'data' => []], 200); | ||
| } |
There was a problem hiding this comment.
Same as above: not needed, but you should declare $calendars etc before the loop below
src/Controller/Api/ApiController.php
Outdated
| if (empty($calendar_id) || !is_int($calendar_id)) { | ||
| return $this->json(['status' => 'error', 'message' => 'Invalid Calendar ID'], 400); | ||
| } |
There was a problem hiding this comment.
Isn't this validation done by the route requirements already?
|
Hi, I will try my best to go over the code in the next few days (midterms are approaching, which limits my free time). In the meantime, I attempted to address most of the code review questions. Regarding API authentication, I would need to explore the options available in Symfony, as I have not worked with Symfony Custom/JSON Auth before. However, I believe this functionality should be restricted to administrators, since most of the API functionality revolves around admin tasks (most users only interact with their CalDav-supporting application of choice). Both JSON Login and API Key authentication seem promising, depending on the implementation:
Do you have a preferred approach or any suggestions regarding API authentication? |
Absolutely no pressure. I myself have a tight planning.
Agreed
I guess an API key in the request (headers probably) — just like you initially envisioned — but embedded in a proper Symfony class / process should fit the use case quite well. BR |
2 participants
Description
This PR adds an API endpoint that allows admins to perform the following user/calendar management tasks, which are normally only available via the admin dashboard:
Additionally, this PR would include:
Reasoning for this PR
I wanted to centralize and automate calendar management in my environment. Rather than emulating browser clicks, I created an API endpoint that allows admins to perform basic tasks via API requests.
The logic for specific endpoints is based on existing code in this project and was designed to mimic its output and functionality, with additional security checks. I made the API endpoint paths match the paths I found using
console debug:routerto make it easier to understand what each route does. Since I use Davis with OpenLDAP, I skipped the logic regarding user creation, removal, etc.Source / References
Logic found in
/src/Controller/*.phpfilesSymfony Command Creation