Add token exchange capability to FetchService #17678
Conversation
We will exchange tokens if there is no token for the given audience or if it's the only way to perform a refresh.
|
Since the separation of concerns topic already came up as a question in the daily today, I'll make another shot at separating involved services more cleanly. Let's see how this will work out. edit: This has been done |
Now splitting responsibilities into: * Fetching * Refreshing * Exchanging Where fetching is composed of the other two services, so that a caller can mostly care about receiving a usable access token and not the intricacies of refreshing and exchanging tokens that were obtained. This is intended to improve readability and understandability of the code, while keeping things together that need to be done in concert.
NobodysNightmare
force-pushed
the
fetch-service-exchange-token
branch
from
600e27e to
0966142
Compare
| def initialize(user:, | ||
| jwt_parser: JwtParser.new(verify_audience: false, verify_expiration: false), | ||
| token_exchange: ExchangeService.new(user:), | ||
| token_refresh: RefreshService.new(user:, token_exchange:)) |
There was a problem hiding this comment.
🟡 Seeing constructors like this makes me feel itchy. Should we consider any kind of IoC here? like Dry::AutoInject?
Also peeking into the direction of @mereghost
There was a problem hiding this comment.
To be fair: I would have probably not added token_exchange and token_refresh as parameters at all, but implemented them as private methods and then in specs I'd have mocked the new method of them, but @mereghost told me having a dependency injecting constructor would be preferrable to that.
In this specific case, I was at least able to use the DI for production code as well, by implementing ExchangeService::Disabled, which is a way to avoid passing an additional enable_token_exchange parameter.
There was a problem hiding this comment.
I just checked the code example of dry-autoinject. I understand that we use it in certain places (e.g. our storage queries), but to me it feels like a too big gun to use here ("Kanonen auf Spatzen").
If the problem we wanted to solve was, that it's somehow bad practice to mock internal implementation details of a class (e.g. knowing that it would internally call OtherService.new), then I can understand that making the injection of said class a part of the class's public interface (e.g. by passing OtherService.new as an argument).
However, I start to become skeptical, when the solution to now having to pass a class in via an argument, is to use a library that makes the internally used class a secret again (it would be "hidden" in a private method again), but allows to change it from another place (e.g. by changing the outside "container").
That at least does not contribute to clarity IMO. So the question is, which problem we are solving by doing that.
| class ExchangeService | ||
| include Dry::Monads[:result] | ||
|
|
||
| class Disabled |
There was a problem hiding this comment.
🟡 This also hints into the direction of dependency injection.
So, depending on the condition, either the ExchangeService or the DisabledExchangeService is injected.
| def call(audience) | ||
| return Failure("Provider does not support token exchange") unless supported? | ||
|
|
||
| FetchService.new(user: @user, token_exchange: Disabled) |
There was a problem hiding this comment.
🟡 a do notation would make this piece of code less indented and IMHO better readable
We will exchange tokens if there is no token for
the given audience or if it's the only way to perform a refresh.
Ticket
https://community.openproject.org/work_packages/60152/activity
What are you trying to accomplish?
Allowing to perform an OAuth 2.0 Token Exchange when that's necessary to obtain a token to be used at the target audience.
What approach did you choose and why?
I integrated this into the existing
FetchService, which now performs 2-3 things, depending on how you count:wherestatement)I chose to integrate all of these aspects into a single service, because it makes sense from a callers perspective, where you just want to have a token and you want to use it. However, I also found it hard to separate these aspects, because they depend on each other. Token Exchange is not only needed for tokens that can't be found, but also for tokens that need to be refreshed. And to perform a token exchange, we need to fetch the correct token first.
Merge checklist