-
Notifications
You must be signed in to change notification settings - Fork 49
Support new JWT token based auth (openEO API 1.3.0) #859
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?
Changes from all commits
26422dc
cde00d6
b0a66a5
d4d5dad
1e75abe
6566959
208b729
39958bf
65eff77
4273d74
84a82ab
ce742e3
d05271f
5166eda
9884bf8
299a079
71a4503
d8dda41
7107497
d925094
7a92e8f
9317759
dc89970
6040a9c
8b72876
85ff1b0
240c18d
94fe914
e6230a3
c2c5766
82c00f4
1d5c20f
8f960e8
33641c4
f943691
9a810c1
a6d3cc2
4abf67e
7c02e85
8ed91bf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,8 +29,9 @@ class BearerAuth(OpenEoApiAuthBase): | |
| https://open-eo.github.io/openeo-api/apireference/#section/Authentication/Bearer | ||
| """ | ||
|
|
||
| def __init__(self, bearer: str): | ||
| def __init__(self, bearer: str, origin: str): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm still unsure about this at least I would make it optional (e.g. default but looking more into this: if isinstance(self.auth, BearerAuth) and self.auth.origin == "oidc" and self._oidc_auth_renewer:as update to the original But I think that original So I would just remove the |
||
| self.bearer = bearer | ||
| self.origin = origin | ||
|
|
||
| def __call__(self, req: Request) -> Request: | ||
| # Add bearer authorization header. | ||
|
|
@@ -42,11 +43,11 @@ class BasicBearerAuth(BearerAuth): | |
| """Bearer token for Basic Auth (openEO API 1.0.0 style)""" | ||
|
|
||
| def __init__(self, access_token: str): | ||
| super().__init__(bearer="basic//{t}".format(t=access_token)) | ||
| super().__init__(bearer="basic//{t}".format(t=access_token), origin="basic") | ||
|
|
||
|
|
||
| class OidcBearerAuth(BearerAuth): | ||
| """Bearer token for OIDC Auth (openEO API 1.0.0 style)""" | ||
|
|
||
| def __init__(self, provider_id: str, access_token: str): | ||
| super().__init__(bearer="oidc/{p}/{t}".format(p=provider_id, t=access_token)) | ||
| super().__init__(bearer="oidc/{p}/{t}".format(p=provider_id, t=access_token), origin="oidc") | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -281,7 +281,7 @@ def validate_access_token(self, access_token: str): | |
| raise LookupError("Invalid access token") | ||
|
|
||
| def invalidate_access_token(self): | ||
| self.state["access_token"] = "***invalidated***" | ||
| self.state["access_token"] = None | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this change necessary to unbreak something? |
||
|
|
||
| def get_request_history( | ||
| self, url: Optional[str] = None, method: Optional[str] = None | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -68,7 +68,7 @@ | |||||
| OidcRefreshTokenAuthenticator, | ||||||
| OidcResourceOwnerPasswordAuthenticator, | ||||||
| ) | ||||||
| from openeo.rest.capabilities import OpenEoCapabilities | ||||||
| from openeo.rest.capabilities import CONFORMANCE_JWT_BEARER, OpenEoCapabilities | ||||||
| from openeo.rest.datacube import DataCube, InputDate | ||||||
| from openeo.rest.graph_building import CollectionProperty | ||||||
| from openeo.rest.job import BatchJob | ||||||
|
|
@@ -277,8 +277,15 @@ def authenticate_basic(self, username: Optional[str] = None, password: Optional[ | |||||
| # /credentials/basic is the only endpoint that expects a Basic HTTP auth | ||||||
| auth=HTTPBasicAuth(username, password) | ||||||
| ).json() | ||||||
|
|
||||||
| # check for JWT bearer token conformance | ||||||
| jwt_conformance = self.capabilities().has_conformance(CONFORMANCE_JWT_BEARER) | ||||||
|
|
||||||
| # Switch to bearer based authentication in further requests. | ||||||
| self.auth = BasicBearerAuth(access_token=resp["access_token"]) | ||||||
| if jwt_conformance: | ||||||
| self.auth = BearerAuth(bearer=resp["access_token"], origin="basic") | ||||||
| else: | ||||||
| self.auth = BasicBearerAuth(access_token=resp["access_token"]) | ||||||
| return self | ||||||
|
|
||||||
| def _get_oidc_provider( | ||||||
|
|
@@ -416,7 +423,12 @@ def _authenticate_oidc( | |||||
| ) | ||||||
|
|
||||||
| token = tokens.access_token | ||||||
| self.auth = OidcBearerAuth(provider_id=provider_id, access_token=token) | ||||||
| # check for JWT bearer token conformance | ||||||
| jwt_conformance = self.capabilities().has_conformance(CONFORMANCE_JWT_BEARER) | ||||||
| if jwt_conformance: | ||||||
| self.auth = BearerAuth(bearer=token, origin="oidc") | ||||||
| else: | ||||||
| self.auth = OidcBearerAuth(provider_id=provider_id, access_token=token) | ||||||
| self._oidc_auth_renewer = oidc_auth_renewer | ||||||
| return self | ||||||
|
|
||||||
|
|
@@ -726,7 +738,7 @@ def authenticate_bearer_token(self, bearer_token: str) -> Connection: | |||||
|
|
||||||
| .. versionadded:: 0.38.0 | ||||||
| """ | ||||||
| self.auth = BearerAuth(bearer=bearer_token) | ||||||
| self.auth = BearerAuth(bearer=bearer_token, origin="unknown") | ||||||
| self._oidc_auth_renewer = None | ||||||
| return self | ||||||
|
|
||||||
|
|
@@ -736,7 +748,7 @@ def try_access_token_refresh(self, *, reason: Optional[str] = None) -> bool: | |||||
| Returns whether a new access token was obtained. | ||||||
| """ | ||||||
| reason = f" Reason: {reason}" if reason else "" | ||||||
| if isinstance(self.auth, OidcBearerAuth) and self._oidc_auth_renewer: | ||||||
| if isinstance(self.auth, BearerAuth) and self.auth.origin == "oidc" and self._oidc_auth_renewer: | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as mentioned: I think this can be simplified to just:
Suggested change
the original |
||||||
| try: | ||||||
| self._authenticate_oidc( | ||||||
| authenticator=self._oidc_auth_renewer, | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,10 @@ def api_version(request): | |
| return request.param | ||
|
|
||
|
|
||
| @pytest.fixture(params=["1.0.0", "1.3.0"]) | ||
| def api_version_authentication_tests(request): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see you originally just updated the parameterization of the existing What's the reason for introducing it? Note: if you did this to avoid explosion of test duplication in test modules where v1.3 is not relevant (yet): pytest allows you to override a fixture at module level and leave the one in the global conftest alone.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (e.g. the |
||
| return request.param | ||
|
|
||
| class _Sleeper: | ||
| def __init__(self): | ||
| self.history = [] | ||
|
|
@@ -99,6 +103,12 @@ def con120(requests_mock, api_capabilities): | |
| con = Connection(API_URL) | ||
| return con | ||
|
|
||
| @pytest.fixture | ||
| def con130(requests_mock, api_capabilities): | ||
| requests_mock.get(API_URL, json=build_capabilities(api_version="1.3.0", **api_capabilities)) | ||
| con = Connection(API_URL) | ||
| return con | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def dummy_backend(requests_mock, con120) -> DummyBackend: | ||
|
|
||
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 think it should also be possible to test use cases of an v1.3 api without jwt conformance class
(the current implementation assumes hard coupling of 1.3 and jwt)