diff --git a/dms/models/directory.py b/dms/models/directory.py index 62caffae6..ec67953d0 100644 --- a/dms/models/directory.py +++ b/dms/models/directory.py @@ -237,24 +237,24 @@ def _compute_access_url(self): def check_access_token(self, access_token=False): res = False if access_token: - items = ( + token_directory = ( self.env["dms.directory"] .sudo() - .search([("access_token", "=", access_token)]) + .search([("access_token", "=", access_token)], limit=1) ) - if items: - item = items[0] - if item.id == self.id: - return True - else: - directory_item = self - while directory_item.parent_id: - if directory_item.id == item.id: - return True - directory_item = directory_item.parent_id - # Fix last level - if directory_item.id == item.id: + if token_directory: + # sudo because the user might not usually have access to the record but + # now the token is valid. + # `seen` bounds the walk: _check_directory_recursion rejects + # cycles created through the ORM, but this path is reachable + # anonymously and must not hang on corrupted data. + directory_item = self.sudo() + seen = set() + while directory_item and directory_item.id not in seen: + if directory_item.id == token_directory.id: return True + seen.add(directory_item.id) + directory_item = directory_item.parent_id return res @api.model diff --git a/dms/models/dms_file.py b/dms/models/dms_file.py index 0b73d9e38..974106cf7 100644 --- a/dms/models/dms_file.py +++ b/dms/models/dms_file.py @@ -167,24 +167,26 @@ def check_access_token(self, access_token=False): if self.access_token and consteq(self.access_token, access_token): return True else: - items = ( + token_directory = ( self.env["dms.directory"] .sudo() - .search([("access_token", "=", access_token)]) + .search([("access_token", "=", access_token)], limit=1) ) - if items: - item = items[0] - if self.directory_id.id == item.id: - return True - else: - directory_item = self.directory_id - while directory_item.parent_id: - if directory_item.id == self.directory_id.id: - return True - directory_item = directory_item.parent_id - # Fix last level - if directory_item.id == self.directory_id.id: + if token_directory: + # The token is known to belong to some directory, but it is not yet + # valid for this file: it only is when that directory is the file's + # own directory or one of its ancestors. sudo() so the walk can + # traverse ancestors the caller is not allowed to read. + # `seen` bounds the walk: _check_directory_recursion rejects cycles + # created through the ORM, but this path is reachable anonymously + # and must not hang on corrupted data. + directory = self.sudo().directory_id + seen = set() + while directory and directory.id not in seen: + if directory.id == token_directory.id: return True + seen.add(directory.id) + directory = directory.parent_id return res res_model = fields.Char( diff --git a/dms/tests/__init__.py b/dms/tests/__init__.py index 2502c8f40..55f0de35e 100644 --- a/dms/tests/__init__.py +++ b/dms/tests/__init__.py @@ -5,3 +5,4 @@ from . import test_file from . import test_benchmark from . import test_portal +from . import test_access_token diff --git a/dms/tests/test_access_token.py b/dms/tests/test_access_token.py new file mode 100644 index 000000000..823e220c9 --- /dev/null +++ b/dms/tests/test_access_token.py @@ -0,0 +1,91 @@ +# Copyright 2026 Millow AB +# License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl). +"""Regression tests for share-token scoping. + +``dms.file.check_access_token`` used to compare the directory walker against +itself instead of against the directory that owns the token, which made the +whole branch collapse to ``return True``. Any valid directory token therefore +granted read access to *every* file in the database, including files in +unrelated trees and in directories with broken group inheritance. +""" + +import uuid + +from .common import StorageFileBaseCase + + +class TestDmsAccessToken(StorageFileBaseCase): + def setUp(self): + super().setUp() + # Two unrelated trees under the same storage. + # shared_root / shared_child <- the token lives on shared_root + # other_root <- must stay unreachable + self.shared_root = self.create_directory(storage=self.storage) + self.shared_child = self.create_directory(directory=self.shared_root) + self.other_root = self.create_directory(storage=self.storage) + + self.file_in_shared_root = self.create_file(directory=self.shared_root) + self.file_in_shared_child = self.create_file(directory=self.shared_child) + self.file_in_other_root = self.create_file(directory=self.other_root) + + self.token = uuid.uuid4().hex + self.shared_root.access_token = self.token + + # ------------------------------------------------------------------ + # The defect + # ------------------------------------------------------------------ + def test_token_does_not_grant_access_to_unrelated_tree(self): + """A token on shared_root must NOT unlock a file under other_root.""" + self.assertFalse( + self.file_in_other_root.check_access_token(self.token), + "Directory token leaked into an unrelated directory tree", + ) + + def test_token_does_not_grant_access_to_root_level_file(self): + """The `# Fix last level` branch self-compared too. + + A file whose directory has no parent skipped the loop entirely and + fell through to a second always-true comparison, so root-level files + leaked as well. + """ + self.assertFalse( + self.file.check_access_token(self.token), + "Directory token leaked into a root-level file of another tree", + ) + + def test_unrelated_token_value_is_rejected(self): + self.assertFalse( + self.file_in_shared_child.check_access_token(uuid.uuid4().hex), + "An unknown token value was accepted", + ) + + def test_no_token_is_rejected(self): + self.assertFalse(self.file_in_shared_child.check_access_token(False)) + + # ------------------------------------------------------------------ + # Legitimate behaviour that must keep working + # ------------------------------------------------------------------ + def test_token_grants_access_to_file_in_the_shared_directory(self): + self.assertTrue( + self.file_in_shared_root.check_access_token(self.token), + "Token did not unlock a file in its own directory", + ) + + def test_token_grants_access_to_file_in_a_descendant_directory(self): + self.assertTrue( + self.file_in_shared_child.check_access_token(self.token), + "Token did not unlock a file in a descendant directory", + ) + + def test_file_own_token_still_works(self): + own_token = uuid.uuid4().hex + self.file_in_other_root.access_token = own_token + self.assertTrue( + self.file_in_other_root.check_access_token(own_token), + "A file's own access token stopped working", + ) + + def test_directory_token_grants_access_to_descendant_directory(self): + """The directory-side implementation was already correct; pin it.""" + self.assertTrue(self.shared_child.check_access_token(self.token)) + self.assertFalse(self.other_root.check_access_token(self.token))