Skip to content

Conversation

@m-mohr
Copy link
Member

@m-mohr m-mohr commented Nov 4, 2025

This implements a new method list_auth_providers so that clients can get a full list of authentication methods/providers easily.

Planned to be used in e.g. the openEO QGIS plugin, which plans to mimic the authentication interface of the Web Editor:

{452A3336-0A1E-4A99-BC3D-C1A37DCAC8F8}

For this, we need a way to retrieve all auth methods. We could also have this code in the plugin itself, but it felt better suited here. Considering also, that the other clients have similar functions.

@m-mohr m-mohr requested a review from soxofaan November 4, 2025 14:39
@m-mohr m-mohr force-pushed the list-auth-providers branch from 499b300 to 4b87748 Compare November 4, 2025 14:39
@m-mohr m-mohr self-assigned this Nov 4, 2025
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?!

Comment on lines 228 to 229
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...

@m-mohr m-mohr force-pushed the list-auth-providers branch from 13a0ccc to 9874dd7 Compare November 7, 2025 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants