Improve ExternalServiceFilter

Apply my comments from the original patch [1] and add unit tests.

[1] https://review.opendev.org/c/openstack/blazar/+/781917

Change-Id: I116aad6677423aedb5dda48f06b36afc32819ba8

blazar/enforcement/exceptions.py

@@ -21,3 +21,8 @@ class MaxLeaseDurationException(exceptions.NotAuthorized):
     msg_fmt = _('Lease duration of %(lease_duration)s seconds must be less '
                 'than or equal to the maximum lease duration of '
                 '%(max_duration)s seconds.')
+
+
+class ExternalServiceFilterException(exceptions.BlazarException):
+    code = 400
+    msg_fmt = _('%(message)s')

blazar/enforcement/filters/base_filter.py

@@ -22,9 +22,12 @@ class BaseFilter(metaclass=abc.ABCMeta):
 
     def __init__(self, conf=None):
         self.conf = conf
+        self.register_opts(conf)
 
-        for opt in self.enforcement_opts:
-            self.conf.register_opt(opt, 'enforcement')
+    @classmethod
+    def register_opts(cls, conf):
+        for opt in cls.enforcement_opts:
+            conf.register_opt(opt, 'enforcement')
 
     def __getattr__(self, name):
         func = getattr(self.conf.enforcement, name)

blazar/enforcement/filters/external_service_filter.py

@@ -13,14 +13,16 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-import datetime
+from datetime import datetime
 import json
 import requests
+from urllib.parse import urljoin
+from urllib.parse import urlparse
 
+from blazar.enforcement.exceptions import ExternalServiceFilterException
 from blazar.enforcement.filters import base_filter
-from blazar import exceptions
+from blazar.exceptions import BlazarException
 from blazar.i18n import _
-from blazar.utils.openstack.keystone import BlazarKeystoneClient
 
 from oslo_config import cfg
 from oslo_log import log as logging
@@ -28,29 +30,18 @@ from oslo_log import log as logging
 LOG = logging.getLogger(__name__)
 
 
-class DateTimeEncoder(json.JSONEncoder):
+class ISODateTimeEncoder(json.JSONEncoder):
     def default(self, o):
-        if isinstance(o, datetime.datetime):
-            return str(o)
+        if isinstance(o, datetime):
+            return o.isoformat()
 
         return json.JSONEncoder.default(self, o)
 
 
-class ExternalServiceUnsupportedHTTPResponse(exceptions.BlazarException):
-    code = 400
-    msg_fmt = _('External service enforcement filter returned a %(status)s '
-                'HTTP response. Only 204 and 403 responses are supported.')
+GENERIC_DENY_MSG = 'External service enforcement filter denied the request.'
 
 
-class ExternalServiceUnsupportedDeniedResponse(exceptions.BlazarException):
-    code = 400
-    msg_fmt = _('External service enforcement filter returned a 403 HTTP '
-                'response %(response)s without a valid JSON dictionary '
-                'containing a "message" key.')
-
-
-class ExternalServiceFilterException(exceptions.NotAuthorized):
-    code = 400
+class ExternalServiceMisconfigured(BlazarException):
     msg_fmt = _('%(message)s')
 
 
@@ -58,21 +49,21 @@ class ExternalServiceFilter(base_filter.BaseFilter):
 
     enforcement_opts = [
         cfg.StrOpt(
-            'external_service_endpoint',
+            'external_service_base_endpoint',
             default=None,
             help='The URL of the external service API.'),
         cfg.StrOpt(
-            'external_service_check_create',
+            'external_service_check_create_endpoint',
             default=None,
-            help='Overwrite check-create endpoint with absolute URL.'),
+            help='Overrides check-create endpoint with another URL.'),
         cfg.StrOpt(
-            'external_service_check_update',
+            'external_service_check_update_endpoint',
             default=None,
-            help='Overwrite check-update endpoint with absolute URL.'),
+            help='Overrides check-update endpoint with another URL.'),
         cfg.StrOpt(
-            'external_service_on_end',
+            'external_service_on_end_endpoint',
             default=None,
-            help='Overwrite on-end endpoint with absolute URL.'),
+            help='Overrides on-end endpoint with another URL.'),
         cfg.StrOpt(
             'external_service_token',
             default="",
@@ -82,75 +73,99 @@ class ExternalServiceFilter(base_filter.BaseFilter):
     def __init__(self, conf=None):
         super(ExternalServiceFilter, self).__init__(conf=conf)
 
-    def get_headers(self):
+        self._validate_url(conf.enforcement.external_service_base_endpoint)
+        self.base_endpoint = conf.enforcement.external_service_base_endpoint
+
+        self.check_create_endpoint = self._construct_url(
+            "check-create",
+            conf.enforcement.external_service_check_create_endpoint)
+        self.check_update_endpoint = self._construct_url(
+            "check-update",
+            conf.enforcement.external_service_check_update_endpoint)
+        self.on_end_endpoint = self._construct_url(
+            "on-end",
+            conf.enforcement.external_service_on_end_endpoint)
+
+        endpoints = (
+            self.check_create_endpoint,
+            self.check_update_endpoint,
+            self.on_end_endpoint,
+        )
+
+        if all(x is None for x in endpoints):
+            raise ExternalServiceMisconfigured(
+                message=_("ExternalService has no endpoints set."))
+
+        self.token = conf.enforcement.external_service_token
+
+    @staticmethod
+    def _validate_url(url):
+        if url is None:
+            return
+        parsed_url = urlparse(url)
+        if parsed_url.scheme not in ("http", "https"):
+            raise ExternalServiceMisconfigured(
+                message=_("ExternalService URL scheme must be http(s): "
+                          "%s") % url)
+        if parsed_url.netloc == '':
+            raise ExternalServiceMisconfigured(
+                message=_("ExternalService URL must have netloc: "
+                          "%s") % url)
+
+    def _construct_url(self, method, replacement_url):
+        if replacement_url is None:
+            if self.base_endpoint is None:
+                return None
+            return urljoin(self.base_endpoint, method)
+
+        self._validate_url(replacement_url)
+        return replacement_url
+
+    def _get_headers(self):
         headers = {'Content-Type': 'application/json'}
 
-        if self.external_service_token:
-            headers['X-Auth-Token'] = (self.external_service_token)
-        else:
-            client = BlazarKeystoneClient()
-            headers['X-Auth-Token'] = client.session.get_token()
+        if self.token:
+            headers['X-Auth-Token'] = self.token
 
         return headers
 
-    def _get_absolute_url(self, path):
-        url = self.external_service_endpoint
+    def _post(self, url, body):
+        body = json.dumps(body, cls=ISODateTimeEncoder)
+        res = requests.post(url, headers=self._get_headers(), data=body)
 
-        if url[-1] == '/':
-            url += path[1:]
-        else:
-            url += path
-
-        return url
-
-    def post(self, url, body):
-        body = json.dumps(body, cls=DateTimeEncoder)
-        req = requests.post(url, headers=self.get_headers(), data=body)
-
-        if req.status_code == 204:
+        if res.status_code == 204:
             return True
-        elif req.status_code == 403:
+        elif res.status_code == 403:
             try:
-                message = req.json()['message']
+                message = res.json()['message']
             except (requests.JSONDecodeError, KeyError):
-                raise ExternalServiceUnsupportedDeniedResponse(
-                    response=req.content)
-
-            raise ExternalServiceFilterException(message=message)
+                # NOTE(yoctozepto): It is more secure not to send the actual
+                # response to the end user as it may leak something.
+                # Instead, we log it for debugging.
+                LOG.debug("The External Service API returned a malformed "
+                          "response (403): %s", res.content)
+                message = GENERIC_DENY_MSG
         else:
-            raise ExternalServiceUnsupportedHTTPResponse(
-                status=req.status_code)
+            # NOTE(yoctozepto): It is more secure not to send the actual
+            # response to the end user as it may leak something.
+            # Instead, we log it for debugging.
+            LOG.debug("The External Service API returned a malformed "
+                      "response (%d): %s", res.status_code, res.content)
+            message = GENERIC_DENY_MSG
+        raise ExternalServiceFilterException(message=message)
 
     def check_create(self, context, lease_values):
-        body = dict(context=context, lease=lease_values)
-        if self.external_service_check_create:
-            self.post(self.external_service_check_create, body)
-            return
-
-        if self.external_service_endpoint:
-            path = '/check-create'
-            self.post(self._get_absolute_url(path), body)
-            return
+        if self.check_create_endpoint:
+            self._post(self.check_create_endpoint, dict(
+                context=context, lease=lease_values))
 
     def check_update(self, context, current_lease_values, new_lease_values):
-        body = dict(context=context, current_lease=current_lease_values,
-                    lease=new_lease_values)
-        if self.external_service_check_update:
-            self.post(self.external_service_check_update, body)
-            return
-
-        if self.external_service_endpoint:
-            path = '/check-update'
-            self.post(self._get_absolute_url(path), body)
-            return
+        if self.check_update_endpoint:
+            self._post(self.check_update_endpoint, dict(
+                context=context, current_lease=current_lease_values,
+                lease=new_lease_values))
 
     def on_end(self, context, lease_values):
-        body = dict(context=context, lease=lease_values)
-        if self.external_service_on_end:
-            self.post(self.external_service_on_end, body)
-            return
-
-        if self.external_service_endpoint:
-            path = '/on-end'
-            self.post(self._get_absolute_url(path), body)
-            return
+        if self.on_end_endpoint:
+            self._post(self.on_end_endpoint, dict(
+                context=context, lease=lease_values))

blazar/tests/enforcement/filters/test_external_service_filter.py

@@ -0,0 +1,252 @@
+# Copyright (c) 2022 Radosław Piliszek <radoslaw.piliszek@gmail.com>
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+# implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import datetime
+import json
+from unittest import mock
+
+from blazar.enforcement.exceptions import ExternalServiceFilterException
+from blazar.enforcement.filters import external_service_filter
+from blazar.tests import TestCase
+
+from oslo_config.cfg import CONF
+
+
+class FakeResponse204():
+    status_code = 204
+
+
+class FakeResponse403Empty():
+    status_code = 403
+    content = "irrelevant_but_logged"
+
+    @staticmethod
+    def json():
+        return {}
+
+
+class FakeJSONDecodeError(Exception):
+    pass
+
+
+class FakeResponse403InvalidJSON():
+    status_code = 403
+    content = "NOT_JSON"
+
+    @staticmethod
+    def json():
+        raise FakeJSONDecodeError()
+
+
+class FakeResponse403WithMessage():
+    status_code = 403
+    content = "irrelevant"
+
+    @staticmethod
+    def json():
+        return {"message": "Hello!"}
+
+
+class FakeResponse500():
+    status_code = 500
+    content = "ALL_YOUR_BUGS_BELONG_TO_US"
+
+
+class ISODateTimeEncoderTestCase(TestCase):
+
+    def test_json_date(self):
+        dt = datetime.datetime(2022, 9, 8, 13, 31, 44, 12345)
+        obj = {"datetime": dt}
+        x = json.dumps(obj, cls=external_service_filter.ISODateTimeEncoder)
+        self.assertEqual('{"datetime": "2022-09-08T13:31:44.012345"}', x)
+
+    def test_json_with_tz(self):
+        tz = datetime.timezone(datetime.timedelta(hours=2))
+        dt = datetime.datetime(2022, 9, 8, 13, 31, 44, 12345, tz)
+        obj = {"datetime": dt}
+        x = json.dumps(obj, cls=external_service_filter.ISODateTimeEncoder)
+        self.assertEqual('{"datetime": "2022-09-08T13:31:44.012345+02:00"}', x)
+
+
+class ConfiguringExternalServiceFilterTestCase(TestCase):
+    def setUp(self):
+        super().setUp()
+
+        external_service_filter.ExternalServiceFilter.register_opts(CONF)
+
+    def test_basic_misconfiguration(self):
+        self.assertRaises(external_service_filter.ExternalServiceMisconfigured,
+                          external_service_filter.ExternalServiceFilter, CONF)
+
+    def test_bad_url(self):
+        CONF.set_override(
+            'external_service_base_endpoint', 'this_url_cOuLDnOtBeWoRsE',
+            group='enforcement')
+        self.addCleanup(CONF.clear_override, 'external_service_base_endpoint',
+                        group='enforcement')
+
+        self.assertRaises(external_service_filter.ExternalServiceMisconfigured,
+                          external_service_filter.ExternalServiceFilter, CONF)
+
+    def test_check_create_endpoint_is_enough(self):
+        CONF.set_override(
+            'external_service_check_create_endpoint', 'http://localhost',
+            group='enforcement')
+        self.addCleanup(CONF.clear_override,
+                        'external_service_check_create_endpoint',
+                        group='enforcement')
+
+        external_service_filter.ExternalServiceFilter(CONF)
+
+    def test_check_updaye_endpoint_is_enough(self):
+        CONF.set_override(
+            'external_service_check_update_endpoint', 'http://localhost',
+            group='enforcement')
+        self.addCleanup(CONF.clear_override,
+                        'external_service_check_update_endpoint',
+                        group='enforcement')
+
+        external_service_filter.ExternalServiceFilter(CONF)
+
+    def test_on_end_endpoint_is_enough(self):
+        CONF.set_override(
+            'external_service_on_end_endpoint', 'http://localhost',
+            group='enforcement')
+        self.addCleanup(CONF.clear_override,
+                        'external_service_on_end_endpoint',
+                        group='enforcement')
+
+        external_service_filter.ExternalServiceFilter(CONF)
+
+
+class ExternalServiceFilterTestCase(TestCase):
+    def setUp(self):
+        super().setUp()
+
+        external_service_filter.ExternalServiceFilter.register_opts(CONF)
+
+        CONF.set_override(
+            'external_service_base_endpoint', 'http://localhost',
+            group='enforcement')
+        self.addCleanup(CONF.clear_override, 'external_service_base_endpoint',
+                        group='enforcement')
+
+        self.filter = external_service_filter.ExternalServiceFilter(CONF)
+
+        self.ctx = {
+            "is_context": True
+        }
+
+        self.lease = {
+            "is_lease": True
+        }
+
+        self.old_lease = {
+            "is_old_lease": True
+        }
+
+    @mock.patch("requests.post")
+    def test_check_create_allowed(self, post_mock):
+        post_mock.return_value = FakeResponse204()
+        self.filter.check_create(self.ctx, self.lease)
+        post_mock.assert_called_with(
+            "http://localhost/check-create",
+            headers={'Content-Type': 'application/json'},
+            data='{"context": {"is_context": true}, '
+                 '"lease": {"is_lease": true}}')
+
+    @mock.patch("requests.post")
+    def test_check_create_denied(self, post_mock):
+        post_mock.return_value = FakeResponse403WithMessage()
+        self.assertRaises(ExternalServiceFilterException,
+                          self.filter.check_create,
+                          self.ctx, self.lease)
+        post_mock.assert_called_with(
+            "http://localhost/check-create",
+            headers={'Content-Type': 'application/json'},
+            data='{"context": {"is_context": true}, '
+                 '"lease": {"is_lease": true}}')
+
+    @mock.patch("requests.post")
+    def test_check_create_failed(self, post_mock):
+        post_mock.return_value = FakeResponse403Empty()
+        self.assertRaises(ExternalServiceFilterException,
+                          self.filter.check_create,
+                          self.ctx, self.lease)
+        post_mock.assert_called_with(
+            "http://localhost/check-create",
+            headers={'Content-Type': 'application/json'},
+            data='{"context": {"is_context": true}, '
+                 '"lease": {"is_lease": true}}')
+
+    @mock.patch("requests.post")
+    def test_check_update_allowed(self, post_mock):
+        post_mock.return_value = FakeResponse204()
+        self.filter.check_update(self.ctx, self.old_lease, self.lease)
+        post_mock.assert_called_with(
+            "http://localhost/check-update",
+            headers={'Content-Type': 'application/json'},
+            data='{"context": {"is_context": true}, '
+                 '"current_lease": {"is_old_lease": true}, '
+                 '"lease": {"is_lease": true}}')
+
+    @mock.patch("requests.post")
+    def test_check_update_denied(self, post_mock):
+        post_mock.return_value = FakeResponse403WithMessage()
+        self.assertRaises(ExternalServiceFilterException,
+                          self.filter.check_update,
+                          self.ctx, self.old_lease, self.lease)
+        post_mock.assert_called_with(
+            "http://localhost/check-update",
+            headers={'Content-Type': 'application/json'},
+            data='{"context": {"is_context": true}, '
+                 '"current_lease": {"is_old_lease": true}, '
+                 '"lease": {"is_lease": true}}')
+
+    @mock.patch("requests.post")
+    @mock.patch("requests.JSONDecodeError", FakeJSONDecodeError)
+    def test_check_update_failed(self, post_mock):
+        post_mock.return_value = FakeResponse403InvalidJSON()
+        self.assertRaises(ExternalServiceFilterException,
+                          self.filter.check_update,
+                          self.ctx, self.old_lease, self.lease)
+        post_mock.assert_called_with(
+            "http://localhost/check-update",
+            headers={'Content-Type': 'application/json'},
+            data='{"context": {"is_context": true}, '
+                 '"current_lease": {"is_old_lease": true}, '
+                 '"lease": {"is_lease": true}}')
+
+    @mock.patch("requests.post")
+    def test_on_end_success(self, post_mock):
+        post_mock.return_value = FakeResponse204()
+        self.filter.on_end(self.ctx, self.lease)
+        post_mock.assert_called_with(
+            "http://localhost/on-end",
+            headers={'Content-Type': 'application/json'},
+            data='{"context": {"is_context": true}, '
+                 '"lease": {"is_lease": true}}')
+
+    @mock.patch("requests.post")
+    def test_on_end_failure(self, post_mock):
+        post_mock.return_value = FakeResponse500()
+        self.assertRaises(ExternalServiceFilterException,
+                          self.filter.on_end,
+                          self.ctx, self.lease)
+        post_mock.assert_called_with(
+            "http://localhost/on-end",
+            headers={'Content-Type': 'application/json'},
+            data='{"context": {"is_context": true}, '
+                 '"lease": {"is_lease": true}}')

doc/source/admin/usage-enforcement.rst

@@ -37,6 +37,11 @@ as follows:
 
 ..
 
+Do note that filter config options follow filter names - the prefix is always
+the snake case of the filter name (``MaxLeaseDurationFilter`` becomes
+``max_lease_duration``; in this case it is special that there is nothing
+beyond the prefix but there is also ``max_lease_duration_exempt_project_ids``).
+
 MaxLeaseDurationFilter
 ----------------------
 
@@ -55,17 +60,32 @@ ExternalServiceFilter
 ---------------------
 
 This filter delegates the decision for each API to an external HTTP service.
-The service must use token-based authentication and implement the following
-endpoints for POST method:
+The service must use token-based authentication, accepting (or ignoring)
+the static token sent by Blazar in the ``X-Auth-Token`` header.
+The following endpoints should be implemented:
 
-* ``POST /v1/check-create``
-* ``POST /v1/check-update``
-* ``POST /v1/on-end``
+* ``POST /check-create``
+* ``POST /check-update``
+* ``POST /on-end``
+
+The exact URLs can be overridden and not all have to be used (although
+we imagine a proper implementation requires at least both checks unless
+lease updates are disabled in the first place).
 
 The external service should return ``204 No Content`` if the parameters meet
-defined criteria and ``403 Forbidden`` if not.
+defined criteria and ``403 Forbidden`` if not. The service may send a JSON
+body response with the ``403 Forbidden`` reply, including the rejection
+reasoning in the field named ``message`` as in:
 
-Example format of data the external service will receive in a request body:
+.. sourcecode:: json
+
+   {
+     "message": "You shall not pass!"
+   }
+
+An example of data the external service will receive in a request body (do note
+all dates and times are encoded as strings following the ISO8601 standard that
+is expected in JSON to represent dates and times):
 
 * Request example:
 
@@ -79,8 +99,8 @@ Example format of data the external service will receive in a request body:
        "region_name": "RegionOne"
      },
      "current_lease": {
-       "start_date": "2020-05-13 00:00",
-       "end_time": "2020-05-14 23:59",
+       "start_date": "2020-05-13T00:00:00.012345+02:00",
+       "end_time": "2020-05-14T23:59:00.012345+02:00",
        "reservations": [
          {
            "resource_type": "physical:host",
@@ -101,8 +121,8 @@ Example format of data the external service will receive in a request body:
        ]
      },
      "lease": {
-       "start_date": "2020-05-13 00:00",
-       "end_time": "2020-05-14 23:59",
+       "start_date": "2020-05-13T00:00:00.012345+02:00",
+       "end_time": "2020-05-14T23:59:00.012345+02:00",
        "reservations": [
          {
            "resource_type": "physical:host",
@@ -130,3 +150,11 @@ Example format of data the external service will receive in a request body:
        ]
      }
    }
+
+The ``current_lease`` field is present only in ``check-update`` requests and
+describes the existing lease. In both checks the ``lease`` field describes
+the new lease. In ``on-end``, the ``lease`` field describes the lease that
+has just ended.
+
+There is no guarantee on the delivery of the ``on-end`` event and it should be
+considered an optimisation rather than a reliable mechanism.