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

✨ [#4398] Check object ownership when creating submission #4696

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
5a03ecd
:sparkles: [#4398] Pass initial_data_reference from auth start to ret…
stevenbal Oct 10, 2024
7381213
:sparkles: [#4398] Check object ownership during prefill and pre-regi…
stevenbal Oct 22, 2024
3099441
:white_check_mark: [#4398] Add tests for initial_data_reference owner…
stevenbal Oct 22, 2024
5b06bc4
:recycle: [#4398] Modify ownership check behavior in case of errors
stevenbal Oct 29, 2024
ba61ffd
:recycle: [#4398] Move ownership check to proper location in prefill …
stevenbal Oct 29, 2024
9f58726
:sparkles: [#4398] Ensure logs from ownership check in pre-registrati…
stevenbal Oct 29, 2024
dc2f955
:white_check_mark: [#4398] Fix VCR issue in Objects API ownership tests
stevenbal Oct 29, 2024
1525548
:white_check_mark: [#4398] Fix failing tests due to new ownership check
stevenbal Oct 29, 2024
b870408
:ok_hand: [#4398] Process PR feedback
stevenbal Nov 4, 2024
0e8176d
:sparkles: [#4398] Configurable path to auth attribute for ownership …
stevenbal Nov 4, 2024
541f3b1
:fire: [#4398] Remove authentication view code to pass initial_data_r…
stevenbal Nov 5, 2024
3658d02
:loud_sound: [#4398] Add logevents for object ownership check
stevenbal Nov 5, 2024
11bf3ed
:white_check_mark: [#4398] Update object ownership tests with log checks
stevenbal Nov 5, 2024
1940faa
:recycle: [#4398] Use submission.registration_backend in registration…
stevenbal Nov 12, 2024
15203ce
:sparkles: [#4398] Make auth_attribute_path required if update existi…
stevenbal Nov 12, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 25 additions & 2 deletions src/openforms/authentication/tests/test_views.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
from unittest.mock import patch

from django.contrib.sessions.middleware import SessionMiddleware
from django.core.handlers.wsgi import WSGIRequest
from django.http import HttpResponse
from django.test import TestCase, override_settings, tag
from django.test.client import RequestFactory
from django.utils.translation import gettext as _
Expand Down Expand Up @@ -32,6 +35,11 @@
from .mocks import FailingPlugin, Plugin, RequiresAdminPlugin, mock_register


def add_session_to_request(request: WSGIRequest) -> None:
middleware = SessionMiddleware(lambda x: HttpResponse())
middleware.process_request(request)


class AuthenticationFlowTests(APITestCase):
@override_settings(
CORS_ALLOW_ALL_ORIGINS=False, CORS_ALLOWED_ORIGINS=["http://foo.bar"]
Expand All @@ -54,6 +62,7 @@ def test_standard_authentication_flow(self):
# we need an arbitrary request
factory = RequestFactory()
init_request = factory.get("/foo")
add_session_to_request(init_request)

next_url = "http://foo.bar"
bad_url = "http://buzz.bazz"
Expand All @@ -65,24 +74,28 @@ def test_standard_authentication_flow(self):

with self.subTest("start ok"):
request = factory.get(url, {"next": next_url})
add_session_to_request(request)
response = start_view(request, slug=form.slug, plugin_id=plugin.identifier)
self.assertEqual(response.content, b"start")
self.assertEqual(response.status_code, 200)

with self.subTest("start missing next"):
request = factory.get(url)
add_session_to_request(request)
response = start_view(request, slug=form.slug, plugin_id=plugin.identifier)
self.assertEqual(response.content, b"missing 'next' parameter")
self.assertEqual(response.status_code, 400)

with self.subTest("start bad plugin"):
request = factory.get(url, {"next": next_url})
add_session_to_request(request)
response = start_view(request, slug=form.slug, plugin_id="bad_plugin")
self.assertEqual(response.content, b"unknown plugin")
self.assertEqual(response.status_code, 400)

with self.subTest("start bad redirect"):
request = factory.get(url, {"next": bad_url})
add_session_to_request(request)
response = start_view(request, slug=form.slug, plugin_id=plugin.identifier)
self.assertEqual(response.content, b"redirect not allowed")
self.assertEqual(response.status_code, 400)
Expand All @@ -94,25 +107,29 @@ def test_standard_authentication_flow(self):

with self.subTest("return ok"):
request = factory.get(url, {"next": next_url})
add_session_to_request(request)
response = return_view(request, slug=form.slug, plugin_id=plugin.identifier)
self.assertEqual(response.content, b"")
self.assertEqual(response.status_code, 302)

with self.subTest("return bad method"):
request = factory.post(f"{url}?next={next_url}")
add_session_to_request(request)
response = return_view(request, slug=form.slug, plugin_id=plugin.identifier)
self.assertEqual(response.content, b"")
self.assertEqual(response.status_code, 405)
self.assertEqual(response["Allow"], "GET")

with self.subTest("return bad plugin"):
request = factory.get(url, {"next": next_url})
add_session_to_request(request)
response = return_view(request, slug=form.slug, plugin_id="bad_plugin")
self.assertEqual(response.content, b"unknown plugin")
self.assertEqual(response.status_code, 400)

with self.subTest("return bad redirect"):
request = factory.get(url, {"next": bad_url})
add_session_to_request(request)
response = return_view(request, slug=form.slug, plugin_id=plugin.identifier)
self.assertEqual(response.content, b"redirect not allowed")
self.assertEqual(response.status_code, 400)
Expand All @@ -133,14 +150,17 @@ def test_plugin_start_failure_redirects(self):
# we need an arbitrary request
factory = RequestFactory()
init_request = factory.get("/foo")
add_session_to_request(init_request)

# actual test starts here
next_url = furl("http://foo.bar?bazz=buzz")
url = plugin.get_start_url(init_request, form)
start_view = AuthenticationStartView.as_view(register=register)

request = factory.get(url, {"next": next_url})
add_session_to_request(request)
response = start_view(
factory.get(url, {"next": next_url}),
request,
slug=form.slug,
plugin_id="plugin1",
)
Expand Down Expand Up @@ -169,13 +189,16 @@ def test_plugin_start_failure_not_enabled(self, mock_get_solo):
# we need an arbitrary request
factory = RequestFactory()
init_request = factory.get("/foo")
add_session_to_request(init_request)

# actual test starts here
next_url = furl("http://foo.bar?bazz=buzz")
url = plugin.get_start_url(init_request, form)
start_view = AuthenticationStartView.as_view(register=register)
request = factory.get(url, {"next": next_url})
add_session_to_request(request)
response = start_view(
factory.get(url, {"next": next_url}),
request,
slug=form.slug,
plugin_id="plugin1",
)
Expand Down
1 change: 0 additions & 1 deletion src/openforms/authentication/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,6 @@ def _handle_return(self, request: Request, slug: str, plugin_id: str):

if hasattr(request, "session") and FORM_AUTH_SESSION_KEY in request.session:
authentication_success.send(sender=self.__class__, request=request)

return response

def _handle_co_sign(self, form: Form, plugin: BasePlugin) -> None:
Expand Down
77 changes: 77 additions & 0 deletions src/openforms/contrib/objects_api/ownership_validation.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
from __future__ import annotations

import logging
from typing import TYPE_CHECKING

from django.core.exceptions import ObjectDoesNotExist, PermissionDenied

from glom import Path, glom
from requests.exceptions import RequestException

from openforms.contrib.objects_api.clients import ObjectsClient
from openforms.logging import logevent

logger = logging.getLogger(__name__)

if TYPE_CHECKING:
from openforms.prefill.contrib.objects_api.plugin import ObjectsAPIPrefill
from openforms.registrations.contrib.objects_api.plugin import (
ObjectsAPIRegistration,
)
from openforms.submissions.models import Submission


def validate_object_ownership(
submission: Submission,
client: ObjectsClient,
object_attribute: list[str],
plugin: ObjectsAPIPrefill | ObjectsAPIRegistration,
) -> None:
"""
Function to check whether the user associated with a Submission is the owner
of an Object in the Objects API, by comparing the authentication attribute.

This validation should only be done if the Submission has an `initial_data_reference`
"""
assert submission.initial_data_reference

try:
auth_info = submission.auth_info
except ObjectDoesNotExist:
logger.exception(
"Cannot perform object ownership validation for reference %s with unauthenticated user",
submission.initial_data_reference,
)
logevent.object_ownership_check_anonymous_user(submission, plugin=plugin)
raise PermissionDenied("Cannot pass data reference as anonymous user")

object = None
try:
object = client.get_object(submission.initial_data_reference)
except RequestException:
logger.exception(
"Something went wrong while trying to retrieve "
"object for initial_data_reference"
)

if not object:
# If the object cannot be found, we cannot consider the ownership check failed
# because it is not verified that the user is not the owner
logger.info(
"Could not find object for initial_data_reference: %s",
submission.initial_data_reference,
)
return

if (
glom(object["record"]["data"], Path(*object_attribute), default=None)
!= auth_info.value
):
logger.exception(
"Submission with initial_data_reference did not pass ownership check for reference %s",
submission.initial_data_reference,
)
logevent.object_ownership_check_failure(submission, plugin=plugin)
raise PermissionDenied("User is not the owner of the referenced object")

logevent.object_ownership_check_success(submission, plugin=plugin)
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
interactions:
- request:
body: null
headers:
Accept:
- '*/*'
Accept-Encoding:
- gzip, deflate, br
Authorization:
- Token 7657474c3d75f56ae0abd0d1bf7994b09964dca9
Connection:
- keep-alive
Content-Crs:
- EPSG:4326
User-Agent:
- python-requests/2.32.2
method: GET
uri: http://localhost:8002/api/v2/objects/300e61aa-e150-459c-aa30-d3f0fe4f5506
response:
body:
string: '{"url":"http://objects-web:8000/api/v2/objects/300e61aa-e150-459c-aa30-d3f0fe4f5506","uuid":"300e61aa-e150-459c-aa30-d3f0fe4f5506","type":"http://objecttypes-web:8000/api/v2/objecttypes/8faed0fa-7864-4409-aa6d-533a37616a9e","record":{"index":1,"typeVersion":1,"data":{"bsn":"111222333","foo":"bar"},"geometry":null,"startAt":"2024-11-05","endAt":null,"registrationAt":"2024-11-05","correctionFor":null,"correctedBy":null}}'
headers:
Allow:
- GET, PUT, PATCH, DELETE, HEAD, OPTIONS
Connection:
- keep-alive
Content-Crs:
- EPSG:4326
Content-Length:
- '422'
Content-Type:
- application/json
Cross-Origin-Opener-Policy:
- same-origin
Date:
- Tue, 05 Nov 2024 14:28:43 GMT
Referrer-Policy:
- same-origin
Server:
- nginx/1.27.0
Vary:
- origin
X-Content-Type-Options:
- nosniff
X-Frame-Options:
- DENY
status:
code: 200
message: OK
version: 1
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
interactions:
- request:
body: null
headers:
Accept:
- '*/*'
Accept-Encoding:
- gzip, deflate, br
Authorization:
- Token 7657474c3d75f56ae0abd0d1bf7994b09964dca9
Connection:
- keep-alive
Content-Crs:
- EPSG:4326
User-Agent:
- python-requests/2.32.2
method: GET
uri: http://localhost:8002/api/v2/objects/300e61aa-e150-459c-aa30-d3f0fe4f5506
response:
body:
string: '{"url":"http://objects-web:8000/api/v2/objects/300e61aa-e150-459c-aa30-d3f0fe4f5506","uuid":"300e61aa-e150-459c-aa30-d3f0fe4f5506","type":"http://objecttypes-web:8000/api/v2/objecttypes/8faed0fa-7864-4409-aa6d-533a37616a9e","record":{"index":1,"typeVersion":1,"data":{"bsn":"111222333","foo":"bar"},"geometry":null,"startAt":"2024-11-05","endAt":null,"registrationAt":"2024-11-05","correctionFor":null,"correctedBy":null}}'
headers:
Allow:
- GET, PUT, PATCH, DELETE, HEAD, OPTIONS
Connection:
- keep-alive
Content-Crs:
- EPSG:4326
Content-Length:
- '422'
Content-Type:
- application/json
Cross-Origin-Opener-Policy:
- same-origin
Date:
- Tue, 05 Nov 2024 14:28:43 GMT
Referrer-Policy:
- same-origin
Server:
- nginx/1.27.0
Vary:
- origin
X-Content-Type-Options:
- nosniff
X-Frame-Options:
- DENY
status:
code: 200
message: OK
version: 1
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
interactions:
- request:
body: null
headers:
Accept:
- '*/*'
Accept-Encoding:
- gzip, deflate, br
Authorization:
- Token 7657474c3d75f56ae0abd0d1bf7994b09964dca9
Connection:
- keep-alive
Content-Crs:
- EPSG:4326
User-Agent:
- python-requests/2.32.2
method: GET
uri: http://localhost:8002/api/v2/objects/300e61aa-e150-459c-aa30-d3f0fe4f5506
response:
body:
string: '{"url":"http://objects-web:8000/api/v2/objects/300e61aa-e150-459c-aa30-d3f0fe4f5506","uuid":"300e61aa-e150-459c-aa30-d3f0fe4f5506","type":"http://objecttypes-web:8000/api/v2/objecttypes/8faed0fa-7864-4409-aa6d-533a37616a9e","record":{"index":1,"typeVersion":1,"data":{"bsn":"111222333","foo":"bar"},"geometry":null,"startAt":"2024-11-05","endAt":null,"registrationAt":"2024-11-05","correctionFor":null,"correctedBy":null}}'
headers:
Allow:
- GET, PUT, PATCH, DELETE, HEAD, OPTIONS
Connection:
- keep-alive
Content-Crs:
- EPSG:4326
Content-Length:
- '422'
Content-Type:
- application/json
Cross-Origin-Opener-Policy:
- same-origin
Date:
- Tue, 05 Nov 2024 14:28:44 GMT
Referrer-Policy:
- same-origin
Server:
- nginx/1.27.0
Vary:
- origin
X-Content-Type-Options:
- nosniff
X-Frame-Options:
- DENY
status:
code: 200
message: OK
version: 1
Loading
Loading