diff --git a/CHANGELOG.md b/CHANGELOG.md index 522064b..fd072aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,8 @@ # Changelog +# 1.2.0 + * Streams returning 403 Forbidden during discovery are now excluded from the catalog; discovery fails only if none are accessible. [#52](https://github.com/singer-io/tap-gitlab/pull/52) # 1.1.0 * Add optional `api_url` config field to support on-premises GitLab instances; defaults to `https://gitlab.com`. [#51](https://github.com/singer-io/tap-gitlab/pull/51) * Bump singer-python to 6.8.0 and requests to 2.34.2. diff --git a/setup.py b/setup.py index 543e125..0855014 100644 --- a/setup.py +++ b/setup.py @@ -5,7 +5,7 @@ setup( name='tap-gitlab', - version='1.1.0', + version='1.2.0', description='Singer.io tap for extracting data from the GitLab API', author='Stitch', url='https://singer.io', diff --git a/tap_gitlab/__init__.py b/tap_gitlab/__init__.py index 334d4ca..9fe1fc6 100644 --- a/tap_gitlab/__init__.py +++ b/tap_gitlab/__init__.py @@ -11,12 +11,12 @@ REQUIRED_CONFIG_KEYS = ["private_token", "start_date", "groups", "projects"] -def do_discover(): +def do_discover(client): """ Discover and emit the catalog to stdout """ LOGGER.info("Starting discover") - catalog = discover() + catalog = discover(client) json.dump(catalog.to_dict(), sys.stdout, indent=2) LOGGER.info("Finished discover") @@ -33,7 +33,7 @@ def main(): with Client(parsed_args.config) as client: if parsed_args.discover: - do_discover() + do_discover(client) elif parsed_args.catalog: sync( client=client, diff --git a/tap_gitlab/discover.py b/tap_gitlab/discover.py index 3a72408..7867404 100644 --- a/tap_gitlab/discover.py +++ b/tap_gitlab/discover.py @@ -2,15 +2,67 @@ from singer import metadata from singer.catalog import Catalog, CatalogEntry, Schema from tap_gitlab.schema import get_schemas +from tap_gitlab.streams import STREAMS +from tap_gitlab.exceptions import ForbiddenError LOGGER = singer.get_logger() -def discover() -> Catalog: +def _prune_inaccessible_children(schemas: dict, field_metadata: dict) -> None: + """ + Remove child streams from the catalog whose parent stream was excluded. + Mutates schemas and field_metadata in place. + """ + for name, stream_cls in list(STREAMS.items()): + parent = getattr(stream_cls, "parent", None) + if name in schemas and parent and parent not in schemas: + LOGGER.warning( + "Stream '%s' excluded from catalog because its parent stream '%s' is not accessible.", + name, parent, + ) + schemas.pop(name, None) + field_metadata.pop(name, None) + + +def _apply_access_checks(client, schemas: dict, field_metadata: dict) -> None: + """ + Probe each stream for read access and remove inaccessible streams + (and their children) from schemas and field_metadata in place. + Raises ForbiddenError if no parent streams are accessible. + """ + inaccessible_streams = [ + stream_name + for stream_name, stream_cls in STREAMS.items() + if stream_name in schemas + and not stream_cls(client=client).check_access() + ] + + for stream_name in inaccessible_streams: + schemas.pop(stream_name, None) + field_metadata.pop(stream_name, None) + + _prune_inaccessible_children(schemas, field_metadata) + + if not schemas: + raise ForbiddenError( + "No streams are accessible. Ensure the credentials have read permission for at least one stream." + ) + elif inaccessible_streams: + LOGGER.warning( + "These streams have been excluded due to HTTP-Error-Code:403 Forbidden: %s", + ", ".join(inaccessible_streams), + ) + + +def discover(client) -> Catalog: """ Run the discovery mode, prepare the catalog file and return the catalog. + Access to each stream is verified using the provided client and streams + the credentials cannot read are excluded from the returned catalog. """ schemas, field_metadata = get_schemas() + _apply_access_checks(client, schemas, field_metadata) + catalog = Catalog([]) for stream_name, schema_dict in schemas.items(): diff --git a/tap_gitlab/streams/abstracts.py b/tap_gitlab/streams/abstracts.py index 452e783..6259936 100644 --- a/tap_gitlab/streams/abstracts.py +++ b/tap_gitlab/streams/abstracts.py @@ -13,6 +13,8 @@ from dateutil import parser from datetime import datetime, timezone +from tap_gitlab.exceptions import ForbiddenError + LOGGER = get_logger() @@ -31,8 +33,8 @@ class BaseStream(ABC): def __init__(self, client=None, catalog=None) -> None: self.client = client self.catalog = catalog - self.schema = self.catalog.schema.to_dict() if self.catalog else None - self.metadata = metadata.to_map(self.catalog.metadata) if self.catalog else None + self.schema = self.catalog.schema.to_dict() if self.catalog else {} + self.metadata = metadata.to_map(self.catalog.metadata) if self.catalog else {} self.child_to_sync = [] self.params = {} @@ -116,6 +118,29 @@ def modify_object(self, record: Dict, parent_record: Dict = None) -> Dict: def get_url_endpoint(self, parent_obj: Dict = None) -> str: return self.url_endpoint or f"{self.client.base_url}/{self.path}" + def check_access(self) -> bool: + """ + Verify that the API credentials have read access to this stream. + Returns True if accessible, False if a 403 Forbidden error is raised. + Child streams always return True (access is governed by the parent check). + """ + if self.parent: + return True + + url = self.get_url_endpoint() + params = {"per_page": 1} + + try: + self.client.get(url, params, self.headers, None) + return True + except ForbiddenError as exc: + LOGGER.warning( + "Unauthorized Stream: %s, excluding from catalog. HTTP-Error-Message:'%s'", + self.tap_stream_id, + str(exc), + ) + return False + class IncrementalStream(BaseStream): def get_bookmark(self, state: dict, stream: str, key: Any = None) -> int: diff --git a/tests/base.py b/tests/base.py index 0cd00fc..8bea0bf 100644 --- a/tests/base.py +++ b/tests/base.py @@ -18,6 +18,7 @@ class BaseTest(BaseCase): in tap-tester tests. Shared tap-specific methods (as needed). """ start_date = "2019-01-01T00:00:00Z" + IS_FORBIDDEN_STREAM = "is-forbidden-stream" @staticmethod def tap_name(): @@ -91,6 +92,14 @@ def expected_metadata(cls): } } + def expected_stream_names(self): + """The expected stream names and exclude forbidden streams.""" + return { + stream_name + for stream_name, metadata in self.expected_metadata().items() + if not metadata.get(self.IS_FORBIDDEN_STREAM, False) + } + @staticmethod def get_credentials(): """Authentication information for the test account.""" diff --git a/tests/unittests/test_discovery.py b/tests/unittests/test_discovery.py new file mode 100644 index 0000000..b5c410b --- /dev/null +++ b/tests/unittests/test_discovery.py @@ -0,0 +1,240 @@ +import unittest +from unittest.mock import patch, MagicMock +from tap_gitlab.discover import discover, _apply_access_checks, _prune_inaccessible_children +from tap_gitlab.exceptions import ForbiddenError + + +class TestAccessChecks(unittest.TestCase): + """Tests for stream access check logic during discovery.""" + + @patch("tap_gitlab.discover._prune_inaccessible_children") + @patch("tap_gitlab.discover.STREAMS") + def test_all_streams_accessible(self, mock_streams, mock_prune): + """All streams accessible - none excluded.""" + mock_client = MagicMock() + + mock_stream_instance = MagicMock() + mock_stream_instance.check_access.return_value = True + mock_stream_cls = MagicMock(return_value=mock_stream_instance) + + mock_streams.items.return_value = [("projects", mock_stream_cls), ("groups", mock_stream_cls)] + + schemas = {"projects": {"properties": {}}, "groups": {"properties": {}}} + field_metadata = {"projects": [], "groups": []} + + _apply_access_checks(mock_client, schemas, field_metadata) + + self.assertIn("projects", schemas) + self.assertIn("groups", schemas) + + @patch("tap_gitlab.discover._prune_inaccessible_children") + @patch("tap_gitlab.discover.STREAMS") + def test_partial_access(self, mock_streams, mock_prune): + """Some streams inaccessible - those are excluded.""" + mock_client = MagicMock() + + accessible_instance = MagicMock() + accessible_instance.check_access.return_value = True + accessible_cls = MagicMock(return_value=accessible_instance) + + forbidden_instance = MagicMock() + forbidden_instance.check_access.return_value = False + forbidden_cls = MagicMock(return_value=forbidden_instance) + + mock_streams.items.return_value = [ + ("projects", accessible_cls), + ("groups", forbidden_cls), + ] + + schemas = {"projects": {"properties": {}}, "groups": {"properties": {}}} + field_metadata = {"projects": [], "groups": []} + + _apply_access_checks(mock_client, schemas, field_metadata) + + self.assertIn("projects", schemas) + self.assertNotIn("groups", schemas) + self.assertIn("projects", field_metadata) + self.assertNotIn("groups", field_metadata) + + @patch("tap_gitlab.discover._prune_inaccessible_children") + @patch("tap_gitlab.discover.STREAMS") + def test_no_streams_accessible_raises(self, mock_streams, mock_prune): + """All streams inaccessible - raises ForbiddenError.""" + mock_client = MagicMock() + + forbidden_instance = MagicMock() + forbidden_instance.check_access.return_value = False + forbidden_cls = MagicMock(return_value=forbidden_instance) + + mock_streams.items.return_value = [ + ("projects", forbidden_cls), + ("groups", forbidden_cls), + ] + + schemas = {"projects": {"properties": {}}, "groups": {"properties": {}}} + field_metadata = {"projects": [], "groups": []} + + with self.assertRaises(ForbiddenError): + _apply_access_checks(mock_client, schemas, field_metadata) + + @patch("tap_gitlab.discover._prune_inaccessible_children") + @patch("tap_gitlab.discover.STREAMS") + def test_no_streams_accessible_raises_with_message(self, mock_streams, mock_prune): + """All streams inaccessible - ForbiddenError has correct message.""" + mock_client = MagicMock() + + forbidden_instance = MagicMock() + forbidden_instance.check_access.return_value = False + forbidden_cls = MagicMock(return_value=forbidden_instance) + + mock_streams.items.return_value = [ + ("projects", forbidden_cls), + ("groups", forbidden_cls), + ] + + schemas = {"projects": {"properties": {}}, "groups": {"properties": {}}} + field_metadata = {"projects": [], "groups": []} + + with self.assertRaises(ForbiddenError) as context: + _apply_access_checks(mock_client, schemas, field_metadata) + + self.assertIn( + "No streams are accessible. Ensure the credentials have read permission for at least one stream.", + str(context.exception), + ) + + @patch("tap_gitlab.discover.LOGGER") + @patch("tap_gitlab.discover._prune_inaccessible_children") + @patch("tap_gitlab.discover.STREAMS") + def test_partial_access_logs_warning(self, mock_streams, mock_prune, mock_logger): + """Some streams inaccessible - logs warning listing excluded streams.""" + mock_client = MagicMock() + + accessible_instance = MagicMock() + accessible_instance.check_access.return_value = True + accessible_cls = MagicMock(return_value=accessible_instance) + + forbidden_instance = MagicMock() + forbidden_instance.check_access.return_value = False + forbidden_cls = MagicMock(return_value=forbidden_instance) + + mock_streams.items.return_value = [ + ("projects", accessible_cls), + ("groups", forbidden_cls), + ] + + schemas = {"projects": {"properties": {}}, "groups": {"properties": {}}} + field_metadata = {"projects": [], "groups": []} + + _apply_access_checks(mock_client, schemas, field_metadata) + + mock_logger.warning.assert_called_with( + "These streams have been excluded due to HTTP-Error-Code:403 Forbidden: %s", + "groups", + ) + + def test_prune_inaccessible_children(self): + """Child streams are removed when parent is excluded.""" + schemas = { + "branches": {"properties": {}}, + "commits": {"properties": {}}, + "groups": {"properties": {}}, + "group_milestones": {"properties": {}}, + } + field_metadata = { + "branches": [], + "commits": [], + "groups": [], + "group_milestones": [], + } + + # branches and commits have parent="projects", which is not in schemas + # group_milestones has parent="groups", which IS in schemas + _prune_inaccessible_children(schemas, field_metadata) + + self.assertNotIn("branches", schemas) + self.assertNotIn("commits", schemas) + self.assertIn("groups", schemas) + self.assertIn("group_milestones", schemas) + + +class TestCheckAccessMethod(unittest.TestCase): + """Tests for BaseStream.check_access().""" + + def test_child_stream_always_returns_true(self): + """Child streams always return True without making API call.""" + from tap_gitlab.streams.branches import Branches + + mock_client = MagicMock() + stream = Branches(client=mock_client) + + result = stream.check_access() + + self.assertTrue(result) + mock_client.get.assert_not_called() + + def test_parent_stream_accessible(self): + """Parent stream returns True when API call succeeds.""" + from tap_gitlab.streams.groups import Groups + + mock_client = MagicMock() + mock_client.base_url = "https://gitlab.com/api/v4" + mock_client.get.return_value = [{"id": 1}] + stream = Groups(client=mock_client) + + result = stream.check_access() + + self.assertTrue(result) + + def test_parent_stream_forbidden(self): + """Parent stream returns False when 403 is raised.""" + from tap_gitlab.streams.groups import Groups + + mock_client = MagicMock() + mock_client.base_url = "https://gitlab.com/api/v4" + mock_client.get.side_effect = ForbiddenError("Forbidden") + stream = Groups(client=mock_client) + + result = stream.check_access() + + self.assertFalse(result) + + def test_parent_stream_forbidden_logs_warning(self): + """Parent stream logs warning with stream name and error message on 403.""" + from tap_gitlab.streams.groups import Groups + + mock_client = MagicMock() + mock_client.base_url = "https://gitlab.com/api/v4" + mock_client.get.side_effect = ForbiddenError("403 Access Denied") + stream = Groups(client=mock_client) + + with patch("tap_gitlab.streams.abstracts.LOGGER") as mock_logger: + result = stream.check_access() + + self.assertFalse(result) + mock_logger.warning.assert_called_once_with( + "Unauthorized Stream: %s, excluding from catalog. HTTP-Error-Message:'%s'", + stream.tap_stream_id, + "403 Access Denied", + ) + + +class TestDiscoverWithClient(unittest.TestCase): + """Tests for the discover() function accepting a client.""" + + @patch("tap_gitlab.discover._apply_access_checks") + @patch("tap_gitlab.discover.get_schemas") + def test_discover_calls_access_checks(self, mock_get_schemas, mock_access_checks): + """discover() calls _apply_access_checks with client.""" + mock_client = MagicMock() + mock_get_schemas.return_value = ({}, {}) + + discover(mock_client) + + mock_access_checks.assert_called_once() + args = mock_access_checks.call_args[0] + self.assertEqual(args[0], mock_client) + + +if __name__ == "__main__": + unittest.main()