Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
32 changes: 32 additions & 0 deletions openeo/rest/connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import os
import shlex
import urllib.parse
import uuid
import warnings
from collections import OrderedDict
from pathlib import Path, PurePosixPath
Expand Down Expand Up @@ -211,6 +212,37 @@ def _get_refresh_token_store(self) -> RefreshTokenStore:
self._refresh_token_store = RefreshTokenStore()
return self._refresh_token_store

def list_auth_providers(self) -> list[dict]:
providers = []
cap = self.capabilities()

# Add OIDC providers
oidc_path = "/credentials/oidc"
if cap.supports_endpoint(oidc_path, method="GET"):
try:
data = self.get(oidc_path, expected_status=200).json()
if isinstance(data, dict):
for provider in data.get("providers", []):
provider["type"] = "oidc"
providers.append(provider)
except OpenEoApiError:
pass
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
except OpenEoApiError:
pass
except OpenEoApiError as e:
warnings.warn(f"Unable to load the OpenID Connect provider list: {e.message}")

Copy link
Member

@soxofaan soxofaan Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we try to avoid usage warnings (I see we still have some cases of that in connection.py), as it generally composes not that good with other tooling. Instead use _log.warning().

Also when including the exception in the warning message, we typically just do {e!r} so that more useful info is included (error code, http code, error message, correlation id )

so:

_log.warning(f"Unable to load the OpenID Connect provider list: {e!r}")

(!r is a shortcut for generic repr()-style rendering of the exception)

Copy link
Member Author

@m-mohr m-mohr Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a warning for now.

Must say I don't like the {e!r} though.

Showing in a GUI (like QGIS, not Python coding)

Unable to load the OpenID Connect provider list: OpenEoApiError('[500] Internal: Maintanence ongoing')

feels like bad UX compared to:

Unable to load the OpenID Connect provider list: Maintanence ongoing')

But if the former is consistently done everywhere, I guess that's how it shall be for now...


# Add Basic provider
basic_path = "/credentials/basic"
if cap.supports_endpoint(basic_path, method="GET"):
providers.append(
{
"id": uuid.uuid4().hex,
"issuer": self.build_url(basic_path),
"type": "basic",
"title": "Internal",
"description": "The HTTP Basic authentication method is mostly used for development and testing purposes.",
}
)

return providers

def authenticate_basic(self, username: Optional[str] = None, password: Optional[str] = None) -> Connection:
"""
Authenticate a user to the backend using basic username and password.
Expand Down
53 changes: 53 additions & 0 deletions tests/rest/test_connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -767,6 +767,59 @@ def test_create_connection_lazy_refresh_token_store(requests_mock):
)


def test_list_auth_providers(requests_mock, api_version):
requests_mock.get(
API_URL,
json={
"api_version": api_version,
"endpoints": [
{"methods": ["GET"], "path": "/credentials/basic"},
{"methods": ["GET"], "path": "/credentials/oidc"},
],
},
)
requests_mock.get(
API_URL + "credentials/oidc",
json={
"providers": [
{"id": "p1", "issuer": "https://openeo.example", "title": "openEO", "scopes": ["openid"]},
{"id": "p2", "issuer": "https://other.example", "title": "Other", "scopes": ["openid"]},
]
},
)

conn = Connection(API_URL)
providers = conn.list_auth_providers()
assert len(providers) == 3

p1 = next(filter(lambda x: x["id"] == "p1", providers), None)
assert isinstance(p1, dict)
assert p1["type"] == "oidc"
assert p1["issuer"] == "https://openeo.example"
assert p1["title"] == "openEO"

p2 = next(filter(lambda x: x["id"] == "p2", providers), None)
assert isinstance(p2, dict)
assert p2["type"] == "oidc"
assert p2["issuer"] == "https://other.example"
assert p2["title"] == "Other"

basic = next(filter(lambda x: x["type"] == "basic", providers), None)
assert isinstance(basic, dict)
assert isinstance(basic["id"], str)
assert len(basic["id"]) > 0
assert basic["issuer"] == API_URL + "credentials/basic"
assert basic["title"] == "Internal"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of using separate asserts, one for each fields, I prefer a single assert that checks the response as a whole, which is perfectly doable here, e.g.

assert providers == [
    {
        "type": "oidc", 
        "issuer": ...
        "title": ...
    },
    {
         ...
]

The advantage of this is that you can easily see and understand the expected output.
And when there is mismatch, pytest will visualize the difference in a very accessible way (when verbosity level is high enough, e.g. see https://docs.pytest.org/en/stable/how-to/output.html#verbosity)

Copy link
Member Author

@m-mohr m-mohr Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure whether this might lead to problems when the (stub) API (for whatever reason) would change the order of the array?!



def test_list_auth_providers_empty(requests_mock, api_version):
requests_mock.get(API_URL, json={"api_version": api_version, "endpoints": []})

conn = Connection(API_URL)
providers = conn.list_auth_providers()
assert len(providers) == 0


def test_authenticate_basic_no_support(requests_mock, api_version):
requests_mock.get(API_URL, json={"api_version": api_version, "endpoints": []})

Expand Down