Skip to content

Commit

Permalink
Make the API-server return HTTP status 404 on non-existing endpoints …
Browse files Browse the repository at this point in the history
…instead of 200.

Fixes #9860.
  • Loading branch information
fniessink committed Feb 6, 2025
1 parent 62ed9dc commit efc0e7c
Show file tree
Hide file tree
Showing 11 changed files with 67 additions and 26 deletions.
4 changes: 2 additions & 2 deletions components/api_server/src/healthcheck.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
api_server_port = getenv("API_SERVER_PORT", "5001")
exit_code = 1
try:
with urlopen(f"http://localhost:{api_server_port}/api/health") as response: # nosec B310
exit_code = 0 if response.status == HTTPStatus.OK else 1
with urlopen(f"http://localhost:{api_server_port}/api/internal/health") as response: # nosec B310
exit_code = 0 if response.status == HTTPStatus.OK and response.read() == '{"healthy": true}' else 1
finally:
sys.exit(exit_code)
3 changes: 2 additions & 1 deletion components/api_server/src/routes/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
get_report_changelog,
)
from .datamodel import get_data_model
from .documentation import get_api
from .documentation import get_api_docs
from .health import get_health
from .logo import get_logo
from .measurement import get_metric_measurements, get_measurements, set_entity_attribute, stream_nr_measurements
from .metric import (
Expand Down
15 changes: 8 additions & 7 deletions components/api_server/src/routes/documentation.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,18 @@


@bottle.get("/api", authentication_required=False)
@bottle.get("/api/<fragment>", authentication_required=False)
@bottle.get("/api/<version>/<fragment>", authentication_required=False)
def get_api(version="", fragment=""):
"""Return the API. Use version and/or fragment to limit the routes returned."""
@bottle.get("/api/", authentication_required=False)
def get_api_docs() -> dict[str, dict[str, str]]:
"""Return the API docs."""
port = os.environ.get("API_SERVER_PORT", "5001")
routes = [route for route in bottle.default_app().routes if version in route.rule and fragment in route.rule]
return {
docs = {
route.rule: {
"url": f"https://www.quality-time.example.org:{port}{route.rule}",
"method": route.method,
"description": route.callback.__doc__,
}
for route in routes
for route in bottle.default_app().routes
}
if not docs:
bottle.abort(404, bottle.HTTP_CODES[404])
return docs
9 changes: 9 additions & 0 deletions components/api_server/src/routes/health.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
"""Health end-point."""

import bottle


@bottle.get("/api/internal/health", authentication_required=False)
def get_health():
"""Return the API-server health."""
return {"healthy": True}
20 changes: 4 additions & 16 deletions components/api_server/tests/routes/test_documentation.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@

import unittest

from routes import get_api
import bottle

from routes import get_api_docs

EXTERNAL_API_VERSION = "v3"

Expand All @@ -16,22 +18,8 @@ class DocumentationTest(unittest.TestCase):

def test_get_api(self):
"""Test that the API can be retrieved."""
api_json = get_api()
api_json = get_api_docs()
self.assertIn("/api", api_json)
self.assertIn(self.login_internal, api_json)
self.assertIn(self.login_external, api_json)
self.assertIn(self.server_external, api_json)

def test_get_api_by_version(self):
"""Test that the API can be filtered by version."""
api_json = get_api(EXTERNAL_API_VERSION)
self.assertNotIn(self.login_internal, api_json)
self.assertIn(self.login_external, api_json)
self.assertIn(self.server_external, api_json)

def test_get_api_by_fragment(self):
"""Test that the API can be filtered by version and a fragment."""
api_json = get_api(EXTERNAL_API_VERSION, "login")
self.assertNotIn(self.login_internal, api_json)
self.assertIn(self.login_external, api_json)
self.assertNotIn(self.server_external, api_json)
13 changes: 13 additions & 0 deletions components/api_server/tests/routes/test_health.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
"""Unit tests for the health route."""

import unittest

from routes import get_health


class HealthTest(unittest.TestCase):
"""Unit tests for the health route."""

def test_health(self):
"""Test that the health status can be retrieved."""
self.assertEqual({"healthy": True}, get_health())
1 change: 1 addition & 0 deletions components/api_server/tests/test_healthcheck.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ def test_healthy(self, mock_urlopen, mock_exit):
"""Test that the server is healthy."""
response = MagicMock()
response.status = HTTPStatus.OK
response.read.return_value = '{"healthy": true}'
response.__enter__.return_value = response
mock_urlopen.return_value = response
import healthcheck # noqa: F401
Expand Down
6 changes: 6 additions & 0 deletions docs/src/changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,12 @@ If your currently installed *Quality-time* version is not the latest version, pl

<!-- The line "## <square-bracket>Unreleased</square-bracket>" is replaced by the release/release.py script with the new release version and release date. -->

## [Unreleased]

### Fixed

- Make the API-server return HTTP status 404 on non-existing endpoints instead of 200. Fixes [#9860](https://github.com/ICTU/quality-time/issues/9860).

## v5.24.0 - 2025-02-06

### Added
Expand Down
6 changes: 6 additions & 0 deletions docs/src/development.md
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,12 @@ python src/quality_time_server.py

The API of the API-server is served at [http://localhost:5001](http://localhost:5001), e.g. access [http://localhost:5001/api/internal/report](http://localhost:5001/api/internal/report) to get the available reports combined with their recent measurements.

To quickly see all endpoints, point curl at `/api`:

```console
curl http://localhost:5001/api | jq .
```

```{note}
If you're new to Python virtual environments, note that:
- Creating a virtual environment (`uv venv`) has to be done once. Only when the Python version changes, you want to recreate the virtual environment.
Expand Down
4 changes: 4 additions & 0 deletions tests/feature_tests/src/features/auth.feature
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ Feature: authentication
When the client tries to log in with incorrect credentials
Then the server tells the client the credentials are incorrect

Scenario: non-existing endpoint
When the client tries to access a non-existing endpoint
Then the server tells the client the endpoint does not exist

Scenario: change editors
When jadoe logs in
And the client changes the reports_overview permissions to "{"edit_reports": ["jadoe", "other_user"], "edit_entities": []}"
Expand Down
12 changes: 12 additions & 0 deletions tests/feature_tests/src/steps/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ def log_in_with_incorrect_credentials(context: Context) -> None:
context.post("login", {"username": "jadoe", "password": "wrong"}) # nosec


@when("the client tries to access a non-existing endpoint")
def access_non_existing_endpoint(context: Context) -> None:
"""Try to access non-existing endpoint."""
context.get("does-not-exist")


@then("the server tells the client the credentials are incorrect")
def check_invalid_credentials(context: Context) -> None:
"""Check that the server responded that the credentials are invalid."""
Expand All @@ -44,6 +50,12 @@ def check_unauthorized(context: Context) -> None:
assert_equal(403, context.response.status_code)


@then("the server tells the client the endpoint does not exist")
def check_does_not_exist(context: Context) -> None:
"""Check that the server responded with a resource does not exist error message."""
assert_equal(404, context.response.status_code)


@when("the client requests the public key")
def get_public_key(context: Context) -> None:
"""Get the public key."""
Expand Down

0 comments on commit efc0e7c

Please sign in to comment.