Conversation
Fix merge
Fix tests
Support audience for refresh token
nsklikas
requested review from
nsklikas,
peppelinux and
rohe
and removed request for
peppelinux and
rohe
ctriant
force-pushed
the
refresh_token_exchange
branch
2 times, most recently
from
a75d7ef to
1eeb2f8
Compare
src/oidcop/oidc/token.py
Outdated
| fun = importer(self.config["token_exchange"]["policy"][""]["callable"]) | ||
| kwargs = self.config["token_exchange"]["policy"][""]["kwargs"] |
There was a problem hiding this comment.
What if that doesn't exist? Should we simply pass if a function isn't defined and accept the request?
src/oidcop/oauth2/token.py
Outdated
| _session_info = _mngr.get_session_info( | ||
| session_id=sid, grant=True) | ||
| except Exception: | ||
| logger.error("Error retrieving token exchabge session information") |
There was a problem hiding this comment.
Typo Error retrieving token exchange session information
src/oidcop/oauth2/token.py
Outdated
| if not resource: | ||
| pass |
There was a problem hiding this comment.
This if clause is unnecessary
src/oidcop/oauth2/token.py
Outdated
| error_description=f"Exchange {request['subject_token_type']} to refresh token forbbiden", | ||
| ) | ||
|
|
||
| scopes = list(set(request.get("scope", ["openid"])).intersection(kwargs.get("scope", ["openid"]))) |
There was a problem hiding this comment.
Why set the scope default value to openid?
src/oidcop/oauth2/token.py
Outdated
| if not audience: | ||
| pass |
There was a problem hiding this comment.
This if clause is unnecessary
ctriant
force-pushed
the
refresh_token_exchange
branch
from
dd1e2c4 to
30f269e
Compare
src/oidcop/session/grant.py
Outdated
| revoked: bool = False, | ||
| token_map: Optional[dict] = None, | ||
| users: list = None, | ||
| users: list = None |
There was a problem hiding this comment.
What is the use of this parameter? Maybe we should remove it?
ctriant
force-pushed
the
refresh_token_exchange
branch
2 times, most recently
from
43f6256 to
867229e
Compare
src/oidcop/oauth2/token.py
Outdated
| error_description="Unauthorized scope requested", | ||
| ) | ||
|
|
||
| subject_token_type = request["subject_token_type"] |
There was a problem hiding this comment.
src/oidcop/oauth2/token.py
Outdated
| response_args["access_token"] = token.value | ||
| response_args["scope"] = token.scope | ||
| response_args["issued_token_type"] = token.token_class | ||
| response_args["expires_in"] = token.usage_rules.get("expires_in", 0) |
There was a problem hiding this comment.
Let's use the same logic as in https://github.com/IdentityPython/oidc-op/blob/master/src/oidcop/oauth2/token.py#L166:
if token.expires_at:
_response["expires_in"] = token.expires_at - utc_time_sans_frac()
src/oidcop/oauth2/token.py
Outdated
| def default_token_exchange_policy(request, context, subject_token, **kwargs): | ||
| if "resource" in request: | ||
| resource = kwargs.get("resource", []) | ||
| if not len(set(request["resource"]).intersection(set(resource))): |
There was a problem hiding this comment.
This is cleaner:
if not set(request["resource"]).issubset(set(resource)):
src/oidcop/oauth2/token.py
Outdated
| error="invalid_target", error_description="Refresh token has single owner" | ||
| ) | ||
| audience = kwargs.get("audience", []) | ||
| if audience and not len(set(request["audience"]).intersection(set(audience))): |
ctriant
force-pushed
the
refresh_token_exchange
branch
from
867229e to
168e67a
Compare
src/oidcop/session/grant.py
Outdated
| self.usage_rules = { | ||
| "access_token": {"supports_minting": ["access_token"], "expires_in": 60} | ||
| } |
There was a problem hiding this comment.
I don't think this is still needed.
ctriant
force-pushed
the
refresh_token_exchange
branch
from
168e67a to
95eda1a
Compare
|
Run darker on this. Other than this LGTM. @rohe @peppelinux what do you think? |
| response_args = {} | ||
| response_args["access_token"] = token.value | ||
| response_args["scope"] = token.scope | ||
| response_args["issued_token_type"] = token.token_class |
There was a problem hiding this comment.
This must be the issued_token_type that was requested not the one on the class (urn:ietf:params:oauth:token-type:access_token vs access_token)
|
Nice set of tests ! |
|
We merged this in our private fork, here are some things that need to be done:
|
|
@nsklikas we just have two conflicting files right now :) |
|
@peppelinux @nsklikas Hello, are you planning to merge this in public repository? I would really like to have token exchange in SATOSA. |
Hello! It is already supported in idpy-oidc. https://github.com/IdentityPython/idpy-oidc/tree/main |
|
It's supported in idpy-oidc but the official SATOSA version at IdentityPython is still not based on idpy-oidc. |
|
The default OIDC frontend in SATOSA uses pyop and oic, but I am using satosa-oidcop which uses this library (oidc-op).
So which library is the "new" one? I though it is this one. |
|
idpy-oidc is where the action is. |
So let me recap:
What are the development plans? Are you going to support oidc-op until idpy-oidc is mature enough and integrated with SATOSA? |
|
Good questions all. |
6 participants
No description provided.