-
Notifications
You must be signed in to change notification settings - Fork 46
Implement list_auth_providers #826
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
base: master
Are you sure you want to change the base?
Conversation
499b300 to
4b87748
Compare
| assert isinstance(basic["id"], str) | ||
| assert len(basic["id"]) > 0 | ||
| assert basic["issuer"] == API_URL + "credentials/basic" | ||
| assert basic["title"] == "Internal" |
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.
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)
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.
I wasn't sure whether this might lead to problems when the (stub) API (for whatever reason) would change the order of the array?!
openeo/rest/connection.py
Outdated
| except OpenEoApiError: | ||
| pass |
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.
| except OpenEoApiError: | |
| pass | |
| except OpenEoApiError as e: | |
| warnings.warn(f"Unable to load the OpenID Connect provider list: {e.message}") |
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.
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)
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.
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...
13a0ccc to
9874dd7
Compare
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:
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.