-
Notifications
You must be signed in to change notification settings - Fork 343
Add Support for Custom AuthManager implementation #2055
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
Changes from all commits
3ee34b7
8fbec86
5954550
92ad051
38042ca
ab75641
9c59d67
51dddf4
c951f83
779b49b
36b34e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -15,6 +15,7 @@ | |||||
# specific language governing permissions and limitations | ||||||
# under the License. | ||||||
# pylint: disable=redefined-outer-name,unused-argument | ||||||
import base64 | ||||||
import os | ||||||
from typing import Any, Callable, Dict, cast | ||||||
from unittest import mock | ||||||
|
@@ -1519,6 +1520,132 @@ def test_request_session_with_ssl_client_cert() -> None: | |||||
assert "Could not find the TLS certificate file, invalid path: path_to_client_cert" in str(e.value) | ||||||
|
||||||
|
||||||
def test_rest_catalog_with_basic_auth_type(rest_mock: Mocker) -> None: | ||||||
# Given | ||||||
rest_mock.get( | ||||||
f"{TEST_URI}v1/config", | ||||||
json={"defaults": {}, "overrides": {}}, | ||||||
status_code=200, | ||||||
) | ||||||
# Given | ||||||
catalog_properties = { | ||||||
"uri": TEST_URI, | ||||||
"auth": { | ||||||
"type": "basic", | ||||||
"basic": { | ||||||
"username": "one", | ||||||
"password": "two", | ||||||
}, | ||||||
}, | ||||||
} | ||||||
catalog = RestCatalog("rest", **catalog_properties) # type: ignore | ||||||
assert catalog.uri == TEST_URI | ||||||
|
||||||
encoded_user_pass = base64.b64encode(b"one:two").decode() | ||||||
expected_auth_header = f"Basic {encoded_user_pass}" | ||||||
assert rest_mock.last_request.headers["Authorization"] == expected_auth_header | ||||||
|
||||||
|
||||||
def test_rest_catalog_with_custom_auth_type() -> None: | ||||||
# Given | ||||||
catalog_properties = { | ||||||
"uri": TEST_URI, | ||||||
"auth": { | ||||||
"type": "custom", | ||||||
"impl": "dummy.nonexistent.package", | ||||||
"custom": { | ||||||
"property1": "one", | ||||||
"property2": "two", | ||||||
}, | ||||||
}, | ||||||
} | ||||||
with pytest.raises(ValueError) as e: | ||||||
# Missing namespace | ||||||
RestCatalog("rest", **catalog_properties) # type: ignore | ||||||
assert "Could not load AuthManager class for 'dummy.nonexistent.package'" in str(e.value) | ||||||
|
||||||
|
||||||
def test_rest_catalog_with_custom_basic_auth_type(rest_mock: Mocker) -> None: | ||||||
# Given | ||||||
catalog_properties = { | ||||||
"uri": TEST_URI, | ||||||
"auth": { | ||||||
"type": "custom", | ||||||
"impl": "pyiceberg.catalog.rest.auth.BasicAuthManager", | ||||||
"custom": { | ||||||
"username": "one", | ||||||
"password": "two", | ||||||
}, | ||||||
}, | ||||||
} | ||||||
rest_mock.get( | ||||||
f"{TEST_URI}v1/config", | ||||||
json={"defaults": {}, "overrides": {}}, | ||||||
status_code=200, | ||||||
) | ||||||
catalog = RestCatalog("rest", **catalog_properties) # type: ignore | ||||||
assert catalog.uri == TEST_URI | ||||||
|
||||||
encoded_user_pass = base64.b64encode(b"one:two").decode() | ||||||
expected_auth_header = f"Basic {encoded_user_pass}" | ||||||
assert rest_mock.last_request.headers["Authorization"] == expected_auth_header | ||||||
|
||||||
|
||||||
def test_rest_catalog_with_custom_auth_type_no_impl() -> None: | ||||||
# Given | ||||||
catalog_properties = { | ||||||
"uri": TEST_URI, | ||||||
"auth": { | ||||||
"type": "custom", | ||||||
"custom": { | ||||||
"property1": "one", | ||||||
"property2": "two", | ||||||
}, | ||||||
}, | ||||||
} | ||||||
with pytest.raises(ValueError) as e: | ||||||
# Missing namespace | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment 'Missing namespace' is misleading. The test is actually checking for a missing impl property when using custom auth type, not a missing namespace.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||
RestCatalog("rest", **catalog_properties) # type: ignore | ||||||
assert "auth.impl must be specified when using custom auth.type" in str(e.value) | ||||||
|
||||||
|
||||||
def test_rest_catalog_with_non_custom_auth_type_impl() -> None: | ||||||
# Given | ||||||
catalog_properties = { | ||||||
"uri": TEST_URI, | ||||||
"auth": { | ||||||
"type": "basic", | ||||||
"impl": "basic.package", | ||||||
"basic": { | ||||||
"username": "one", | ||||||
"password": "two", | ||||||
}, | ||||||
}, | ||||||
} | ||||||
with pytest.raises(ValueError) as e: | ||||||
# Missing namespace | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment 'Missing namespace' is misleading. The test is actually checking for an invalid impl property usage, not a missing namespace.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||
RestCatalog("rest", **catalog_properties) # type: ignore | ||||||
assert "auth.impl can only be specified when using custom auth.type" in str(e.value) | ||||||
|
||||||
|
||||||
def test_rest_catalog_with_unsupported_auth_type() -> None: | ||||||
# Given | ||||||
catalog_properties = { | ||||||
"uri": TEST_URI, | ||||||
"auth": { | ||||||
"type": "unsupported", | ||||||
"unsupported": { | ||||||
"property1": "one", | ||||||
"property2": "two", | ||||||
}, | ||||||
}, | ||||||
} | ||||||
with pytest.raises(ValueError) as e: | ||||||
# Missing namespace | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment 'Missing namespace' is misleading. The test is actually checking for an unsupported auth type, not a missing namespace.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||
RestCatalog("rest", **catalog_properties) # type: ignore | ||||||
assert "Could not load AuthManager class for 'unsupported'" in str(e.value) | ||||||
|
||||||
|
||||||
EXAMPLE_ENV = {"PYICEBERG_CATALOG__PRODUCTION__URI": TEST_URI} | ||||||
|
||||||
|
||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could imagine doing a version of this test that specifies the basic auth as the implementation and just verifies that the custom implementation is actually used/that properties specified for custom are actually passed to the implementation.