Add Support for Custom AuthManager implementation #2055

sungwy · 2025-06-01T16:33:19Z

Rationale for this change

Expose a way for users to use a custom AuthManager when they define their REST Catalog.

Add property parsing for custom AuthManager
Add docs

The properties should follow the pattern in: apache/iceberg#13209

Are these changes tested?

Unit and integration tests will be added

Are there any user-facing changes?

New properties will be exposed to support adding a custom AuthManager to RestCatalog

corleyma

A couple of comments but no real substantive feedback because this is great. Looking forward to being able to use different auth implementations.

One question: I assume future authmanager implementations added to pyiceberg will always get a dedicated type, and custom will be reserved for implementations that aren't in tree?

corleyma · 2025-07-18T03:43:38Z

mkdocs/docs/configuration.md

+
+| Property         | Required | Description                                                                                     |
+|------------------|----------|-------------------------------------------------------------------------------------------------|
+| `auth.type`      | Yes      | The authentication type to use (`noop`, `basic`, `oauth2`, or `custom`).                       |


on review I was debating the necessity of a separate type config given an impl option, but I can see the appeal (we are free to change the implementation referred to by a given type).

we have similar pattern to load "custom" implementation for catalog/file_io/location_provider :)
https://grep.app/search?f.repo=apache%2Ficeberg-python&f.repo.pattern=iceberg&q=def+_import_

corleyma · 2025-07-18T03:47:56Z

tests/catalog/test_rest.py

+    assert "__init__() missing 1 required positional argument: 'password'" in str(e.value)
+
+
+def test_rest_catalog_with_custom_auth_type() -> None:


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.

kevinjqliu

super excited to see this. I added a few comments :)

kevinjqliu · 2025-07-18T21:04:12Z

mkdocs/docs/configuration.md

@@ -374,6 +374,94 @@ Specific headers defined by the RESTCatalog spec include:
 | ------------------------------------ | ------------------------------------- | -------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
 | `header.X-Iceberg-Access-Delegation` | `{vended-credentials,remote-signing}` | `vended-credentials` | Signal to the server that the client supports delegated access via a comma-separated list of access mechanisms. The server may choose to supply access via any or none of the requested mechanisms |

+#### Authentication in RESTCatalog


This is great! A heads up, we recently merged a PR which added an "Authentication Options" section under the REST Catalog docs. Would be great to merge with what you have here.

See,
https://github.com/apache/iceberg-python/blob/main/mkdocs/docs/configuration.md#authentication-options

kevinjqliu · 2025-07-18T21:16:07Z

pyiceberg/catalog/rest/auth.py

+        client_id: str,
+        client_secret: str,


should we let users provide these 2 separately? or parse them out from credential based on https://github.com/apache/iceberg-python/blob/main/mkdocs/docs/configuration.md#oauth2

there are a couple of options in the oauth section
token
credential
scope
resource
audience

LegacyOAuth2AuthManager currently supports this, so i think we should support them in the new implementation too
WDYT?

separated out to: #2244

sungwy · 2025-07-23T02:05:58Z

It looks like there's some more discussions to be had to introduce the OAuth2AuthManager into the repository, and I think there's a lot of value in introducing the custom AuthManager parsing in this release.

@kevinjqliu - let me separate that implementation out to a separate review

@corleyma - I will take your suggestion to use the basic auth manager for testing for this PR :)

talatuyarer

@sungwy For absolute completeness, you might consider adding a simple test for type: noop that asserts that the Authorization header is not present in the request. This would confirm the "off switch" works as intended.

But Overall this is awesome change to unblock custom implementation. Thank you so much.

Copilot

Pull Request Overview

This PR adds support for custom authentication managers in the REST catalog by introducing a new configuration pattern. The change allows users to specify different authentication types including basic auth and custom implementations through an auth configuration block.

Added support for pluggable authentication via auth configuration property with support for noop, basic, and custom types
Implemented validation logic to ensure proper configuration of auth type and implementation
Added comprehensive test coverage for all authentication scenarios

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
`pyiceberg/catalog/rest/__init__.py`	Adds auth configuration parsing and validation logic in `_create_session()` method
`pyiceberg/catalog/rest/auth.py`	Adds docstrings to existing AuthManager implementations
`tests/catalog/test_rest.py`	Comprehensive test suite for new auth configuration patterns
`mkdocs/docs/configuration.md`	Documentation for the new pluggable authentication system

pyiceberg/catalog/rest/__init__.py

Copilot · 2025-07-25T17:19:39Z

pyiceberg/catalog/rest/__init__.py

+            if auth_type is CUSTOM and not auth_impl:
+                raise ValueError("auth.impl must be specified when using custom auth.type")
+
+            if auth_type is not CUSTOM and auth_impl:


The comparison auth_type is not CUSTOM uses identity comparison with a string literal. This should use inequality comparison (!=) instead since auth_type comes from dictionary input and may not be the same string object.

Suggested change

if auth_type is not CUSTOM and auth_impl:

if auth_type != CUSTOM and auth_impl:

Copilot · 2025-07-25T17:19:39Z

tests/catalog/test_rest.py

+        },
+    }
+    with pytest.raises(ValueError) as e:
+        # Missing namespace


The comment 'Missing namespace' is misleading. The test is actually checking for an unsupported auth type, not a missing namespace.

Suggested change

# Missing namespace

# Unsupported authentication type

Copilot · 2025-07-25T17:19:40Z

tests/catalog/test_rest.py

+        },
+    }
+    with pytest.raises(ValueError) as e:
+        # Missing namespace


The comment 'Missing namespace' is misleading. The test is actually checking for an invalid impl property usage, not a missing namespace.

Suggested change

# Missing namespace

# Invalid impl property usage in non-custom auth.type

Copilot · 2025-07-25T17:19:40Z

tests/catalog/test_rest.py

+        },
+    }
+    with pytest.raises(ValueError) as e:
+        # Missing namespace


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

# Missing namespace

# Missing 'impl' property in custom auth configuration

kevinjqliu · 2025-07-25T17:21:38Z

first time trying to copilot, good bot!

kevinjqliu

LGTM! Could you address the comments from copilot?

@sungwy For absolute completeness, you might consider adding a simple test for type: noop that asserts that the Authorization header is not present in the request. This would confirm the "off switch" works as intended.

+1 to this. Could you also add a test for the fallback case where auth: is not specified?

Co-authored-by: Copilot <[email protected]>

sungwy

thnx copilot

kevinjqliu · 2025-07-25T18:15:56Z

Merged, this should unblock #2244 and #2072

Thanks everyone for the review!

# Rationale for this change Expose a way for users to use a custom AuthManager when they define their REST Catalog. - [x] Add property parsing for custom AuthManager - [x] Add docs The properties should follow the pattern in: apache/iceberg#13209 # Are these changes tested? Unit and integration tests will be added # Are there any user-facing changes? New properties will be exposed to support adding a custom AuthManager to RestCatalog --------- Co-authored-by: Copilot <[email protected]>

add OAuth2AuthManager

3ee34b7

sungwy added the changelog Indicates that the PR introduces changes that require an entry in the changelog. label Jun 1, 2025

sungwy added 4 commits July 4, 2025 14:14

Merge branch 'main' into custom-auth-support

8fbec86

add custom auth implementation parsing

5954550

add tests

92ad051

add docs

38042ca

sungwy marked this pull request as ready for review July 17, 2025 01:27

sungwy requested a review from Fokko July 17, 2025 01:27

fix test for python3.9

ab75641

sungwy requested a review from kevinjqliu July 17, 2025 01:43

corleyma approved these changes Jul 18, 2025

View reviewed changes

kevinjqliu reviewed Jul 18, 2025

View reviewed changes

kevinjqliu added this to the PyIceberg 0.11.0 milestone Jul 22, 2025

sungwy added 2 commits July 23, 2025 02:25

remove oauth2manager to separate PR

9c59d67

verify header in basic auth

51dddf4

sungwy requested a review from corleyma July 23, 2025 02:35

kevinjqliu self-requested a review July 24, 2025 17:15

talatuyarer approved these changes Jul 24, 2025

View reviewed changes

sungwy added 2 commits July 25, 2025 01:39

Merge branch 'main' into custom-auth-support

c951f83

combine auth docs

779b49b

kevinjqliu requested review from Copilot and removed request for corleyma July 25, 2025 17:18

Copilot AI reviewed Jul 25, 2025

View reviewed changes

kevinjqliu approved these changes Jul 25, 2025

View reviewed changes

kevinjqliu modified the milestones: PyIceberg 0.11.0, PyIceberg 0.10.0 Jul 25, 2025

thnx copilot

36b34e5

Co-authored-by: Copilot <[email protected]>

sungwy commented Jul 25, 2025

View reviewed changes

kevinjqliu mentioned this pull request Jul 25, 2025

New OAuth2Manager #2244

Open

kevinjqliu merged commit 5e0499c into apache:main Jul 25, 2025
11 of 12 checks passed

sungwy deleted the custom-auth-support branch July 25, 2025 19:23

sungwy mentioned this pull request Aug 8, 2025

Expose a way for users to use a custom AuthManager when they define their REST Catalog #1960

Closed

		assert "__init__() missing 1 required positional argument: 'password'" in str(e.value)


		def test_rest_catalog_with_custom_auth_type() -> None:

	if auth_type is not CUSTOM and auth_impl:
	if auth_type != CUSTOM and auth_impl:

	# Missing namespace
	# Invalid impl property usage in non-custom auth.type

	# Missing namespace
	# Missing 'impl' property in custom auth configuration

Add Support for Custom AuthManager implementation #2055

Add Support for Custom AuthManager implementation #2055

Uh oh!

Conversation

sungwy commented Jun 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

Are these changes tested?

Are there any user-facing changes?

Uh oh!

corleyma left a comment

Choose a reason for hiding this comment

Uh oh!

corleyma Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

kevinjqliu Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

corleyma Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

kevinjqliu Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

kevinjqliu Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

kevinjqliu Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

sungwy Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

sungwy commented Jul 23, 2025

Uh oh!

talatuyarer left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Jul 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 25, 2025

Choose a reason for hiding this comment

Uh oh!

kevinjqliu commented Jul 25, 2025

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

sungwy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevinjqliu commented Jul 25, 2025

Uh oh!

Uh oh!

sungwy commented Jun 1, 2025 •

edited

Loading