Consistent JSON API incl. Proper Handling of Errors (2nd half of big refactoring)

[nagmat84]

Summary

Custom Exceptions with Optional HTTP Status Code

A lot of custom exception classes have been created. They are organized as follows:

All exceptions realize the interface LycheeException. This interface does not declare any methods. It is only used for grouping all Lychee exceptions. Lychee exceptions are categorized into external and internal exceptions via the interfaces ExternalLycheeException and InternalLycheeException.

External exceptions are meant to be returned to the frontend. To foster this, external Lychee exceptions must also realize
HttpExceptionInterface to provide meaningful HTTP status codes to the frontend. In particular, the message text of external Lychee exceptions should

1. be understandable and provide a meaningful information for an end user as the message text is displayed to the user inside the status bar of the front-end
2. not disclose any sensitive, security-relevant information

Internal exceptions are used for internal error propagation. While internal exceptions bubble up the call graph they should become wrapped into an external exception at some point.

Ideally, the methods of the HTTP controllers eventually only throw external exceptions. However, if an internal exception slips through the exception handler will take care of that and return a generic 500 Internal Server Error to the frontend as a last line of defense to avoid unintended leakage of sensitive information.

Revised Exception Handler

The exception handler App\Exceptions\Handler has heavily been revised.

Correct content type

The old method render is gone. This method was bad, because it did not honor the content type which was expected by the client but unconditionally returned a HTML representation of the exception even if the frontend requested JSON.

The method has been replaced by the two distinct methods renderHttpException and convertExceptionToArray.
The former is called by the Laravel framework for HTML responses, the latter for JSON responses. Note, that renderHttpException is actually a misnomer, it should be named renderHTMLException (because JSON also uses HTTP as a transport layer), but heh, this is Laravel. 😢

Logging

Exceptions are now properly logged to our own home-brewed logging mechanism. The method report of the Laravel default exception handler has been overridden. The Larvavel default method tries to use a service which realizes the \Psr\Log\LoggerInterface which is not implemented by our class Logs and hence it has not worked until now.

This gives us two benefits:

• We can avoid the "double" error handling in our code, i.e. we do not need to throw an exception and separately create a log entry with the same error message. Creating and throwing the exception now suffices.
• If any uncaught exception (from the framework or a low-level PHP exception) occurs, we will see it in the logs with a proper error message and backtrace. So we will be able to help our users much faster if they experience an unexpected error. 😎 🎉 🎊

Consistent Handling of Requests

Previously, incoming requests have been handled very inconsistently. In particular, proper and complete input validation of was matter of luck and even if it partially happened it was too late most of the time. Authorization was inconsistent, too. Some of it happened inside the middleware (but at a too early stage), some of it happened inside the controller methods or even deeper down.

Now, there is a strict order

1. Input validation (HTTP status on failure: 422 Unprocessable)
2. User authentication (HTTP status on failure: 401 Unauthorized)
3. Model hydration (HTTP status on failure: 404 Not Found)
4. Authorization (HTTP status on failure: 403 Forbidden)

Or in cat format:

This gives us three benefits:

• Higher Security: Previously, the middleware which tried to do some authorization during a very early stage fed the unvalidated input to the DB. That's avoided now; we always validate first.
• Slightly Higher Efficiency: Previously, there were several database requests for the same model which was indicated by the request while the request was being processed only to hydrate this model from the DB eventually as part of the controller method. This is gone now; the models which are indicated by the request are now loaded in step 3 and become part of the request object so they can be used later on without repeated DB request.
• Solved Regression Bug: The known regression bug of the first half of the re-factoring was due to the wrong order of step 4 and 3. So the frontend always got a 403 response even if the model did not exist.

All this is achieved by a lot of custom request classes in App\Http\Requests. Maybe you know the Laravel feature that routes can have place-holders for model IDs, e.g. something like GET /album/{album}/ and then "by magic" Laravel hydrates the model and puts it into the request object. This does not work in out case, because our routes don't follow this pattern and the IDs are part of the JSON payload of the request. The custom request classes rectify this problem.

Middleware Cleanup

The middleware has been mucked out:

• Authorization in the middleware is gone. It is now part of the request handling, see previous point.
• Strict checks for the content type in a request and for the expected content type of the response have been added.
• Middlewares do not directly return a HTML response unconditionally, if something fails but they only throw a specific exception and let the exception handler do the rest. This is required in order to return the correct content type depending
  on the expectation of the request (either HTML or JSON)

Miscellaneous

• The streamed response for importing images from the server streamed a proper JSON array. This allows much easier and "type-safe" parsing on the frontend.
• In Albums::get the tags albums have been split from the collection of built-in smart albums. Using two separate collections avoids some awkward and unnecessary checks in the frontend.
• A lot of DTO (data-transfer objects): previously, a lot of methods passed around associative arrays instead of proper and type-safe objects.
• The routing files have been cleaned up; in particular requests for HTML pages and API requests for JSON have been separated.
• Many requests and responses now use proper JSON array to transport lists (e.g. lists of tags, lists of IDs, etc.)

[nagmat84]

Just a review on my own so that GIThub memorizes the files which I have viewed and checked.

[kamil4]

Deleting the contents of the Unsorted album as an unprivileged user deletes visible photos of other users as well

This appears to be fixed now.

[kamil4]

Legacy ID translation fails because of this condition (is_int). I can make it work by changing it to is_numeric but then I'm not sure what would be the intended difference between this case and the one right above?

[nagmat84]

Fixed this in LycheeOrg/Lychee-front@cd6df80 and in 6c99d6a.

The difference between is_int and is_numeric is that the latter also excepts numbers encoded as strings. But I really wanted is_int now after we have proper JSON everywhere. But I missed to update the rest.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
64 Code Smells

No Coverage information
0.2% Duplication