Skip to content
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

Support a default provider for OIDC + new example #373

Merged
merged 2 commits into from
Apr 8, 2021
Merged

Conversation

m-mohr
Copy link
Member

@m-mohr m-mohr commented Apr 7, 2021

  • Support a default client for OIDC
  • Added a new example, also showing the default_clients

@m-mohr m-mohr added this to the 1.1.0 milestone Apr 7, 2021
@m-mohr m-mohr requested a review from soxofaan April 7, 2021 15:10
@soxofaan
Copy link
Member

soxofaan commented Apr 7, 2021

issue title should say "default provider" instead of "default client"?

@m-mohr m-mohr changed the title Support a default client for OIDC + new example Support a default provider for OIDC + new example Apr 7, 2021
@m-mohr
Copy link
Member Author

m-mohr commented Apr 7, 2021

Yes, indeed.

@soxofaan
Copy link
Member

soxofaan commented Apr 7, 2021

not ideal that you can not enforce that at most one provider can be flagged as default.

Alternatives:

  • add a top level field "default_provider" (type: string) to point to id of the default provider
  • instead of "default" boolean flag: add "priority" integer field: highest priority is default provider
  • first provider in list should be considered default provider

@m-mohr
Copy link
Member Author

m-mohr commented Apr 7, 2021

Yes, that's always the issue (see also default_plan for example). You can't really provide a good way of doing it consistently. If you do "default_provider", then you can't check whether the value actually exists. Similarly for the priority, where you can't check whether all fields may be 0 for example.
The first in the list semantic could work, but is somewhat giving a meaning to something a provider may not have chosen explicitly as he may had implemented it in 1.0 where this was not the case yet. Let's say it's not really breaking, but also not really the nicest way of being backward-compatible.

@m-mohr
Copy link
Member Author

m-mohr commented Apr 8, 2021

@soxofaan Would you prefer the alternative of the first array element being the default? I think that's the only option that would lead to a bit more consistency.

@soxofaan
Copy link
Member

soxofaan commented Apr 8, 2021

Yes, "first of list" would be my preference. It introduces no real API changes, except for documenting this "convention". It would also work automatically for backends that just specify a single OIDC provider.

Second preference would be new "default_provider" field because if there is an inconsistency then it will fail in a clear way. If you have inconsistency with the boolean "default" flags, then some implementations might still accept that (because they pick the first match they find).

@m-mohr
Copy link
Member Author

m-mohr commented Apr 8, 2021

I'll update this PR to work with the array convention.

@m-mohr m-mohr self-assigned this Apr 8, 2021
@m-mohr
Copy link
Member Author

m-mohr commented Apr 8, 2021

PR has been updated. This really feels much better integrated, it also works out of the box with the Web Editor implementation. A review would be appreciated.

@m-mohr m-mohr requested review from soxofaan and removed request for soxofaan April 8, 2021 15:31
@m-mohr m-mohr merged commit a2ffeea into draft Apr 8, 2021
@m-mohr m-mohr deleted the default-client branch April 8, 2021 15:39
soxofaan added a commit to Open-EO/openeo-python-client that referenced this pull request Apr 20, 2021
soxofaan added a commit to Open-EO/openeo-geopyspark-driver that referenced this pull request Apr 21, 2021
First provider under `/credentials/oidc` is default provider
see Open-EO/openeo-api#373
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.

2 participants