-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
[AccessToken] A component for fetching and caching remote services access tokens #54013
base: 7.2
Are you sure you want to change the base?
Conversation
First time I try to add a new component, may someone help me with this test failing ?
It seems that the added component tries to put an empty array in default framework configuration when unconfigured, whereas it should not, did I miss some function call in the configuration builder ? |
EDIT: And I was wrongly reading it, some are unrelated, some are my fault. Will fix it ASAP. |
If I understand properly, if this component is approved, everybody will be able to contribute to add bridge to any OAuth service in a similar way we can contribute to the Notifier/Mailer/Messenger component to add a bridge to a service? A common usage for this component can also be for Google/Facebook... sign implementation in I guess? I like it. |
I have a question about strict types, fabbot.io https://fabbot.io/report/symfony/symfony/54013/13a53c83f8401eac907e7fd712dca8105a9480b8 seems to consider as an error only 11 of them. Is this an error I should ignore (I think that the fabbot.io report is mixing two rules, one that says that declare() must be first in file, and the other that says that the licence header must be first in file). Does Symfony convention states that strict_types should not be used at all ? |
Yes it's designed to be easily pluggable, that's the whole goal of it. FYI I tested the current OAuth2 implementation with Microsoft Azure OAuth, and it works. In the past and present, I had to deal with Salesforce OAuth2 as well (a lot), with the tiny exception that Salesforce requires some additional parameters (which can be arbitrarily user given in the actual state of code) it should work as well. The URL to DSN parsing we implemented passes along all unhandled parameters to the final endpoint URL, which allows that. Most OAuth2 providers that implements the OAuth standard as documented should work with the current implementation (you don't even have to add a bridge).
I'm not sure to understand what your are talking about, it's important to keep in mind this is for authorization flows that don't require user interaction, dedicated to B2B/MTM. |
Oh yes, sorry I read but forgot that part. |
5a76d9f
to
cacd021
Compare
src/Symfony/Component/AccessToken/Bridge/OAuth/ClientCredentials.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/AccessToken/Bridge/OAuth/ClientCredentialsProvider.php
Show resolved
Hide resolved
src/Symfony/Component/AccessToken/Manager/LockAccessTokenManagerDecorator.php
Outdated
Show resolved
Hide resolved
@OskarStark Thank you very much ! |
7fb26f3
to
7ca4507
Compare
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.
wow, massive body of work! i like it. the basic organization seems good to me, i only commented some minor details.
would be awesome to get a review by the maintainers of HWIOAuthBundle for this. ideally, this component would be useful for such a bundle and allow them to simplify what they provide. have you looked at HWIOAuthBundle to see if part of their OAuth implementation could be used here as well? (the license is MIT for both the bundle and symfony, so legally that should be no issue)
paging @stloyd :-)
public function __construct( | ||
protected readonly string $value, | ||
protected readonly string $type = 'Bearer', | ||
protected readonly int $expiresIn = 600, |
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.
where does this default come from? does it make sense to have a default?
and it would make sense to phpdoc that this is in seconds.
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.
Indeed, rightly spotted, I maybe have missed to document it's seconds!
The default value actually come from an history of successive copy-pastes, this is a value I found in more than one pieces of code, gists, and other code samples. It actually makes sense, it's 10 minutes, and I often in the past stumbled upon 15 minutes-ish or so lifetime tokens. When the provider doesn't expose a lifetime, we need an arbitrary value to set for the cache anyway.
If you have a better idea how to manage this, but ideally I'd like to keep a default here. It could be simply 0, to tell not to cache, nevertheless this would have other implications and complexify the code (because token is validity tested right after being fetched, this another use case to handle, one-time usage tokens, which for now are handled quite transparently).
In the end I'd prefer to be conservative here and cache it anyway when the provider erroneously didn't expose a lifetime, so that this token will be retried at least once.
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.
it could make sense to have an option (on the factory, not the token) to provide a default value. and as the token is always created by the factory (from how i understood) the token could chose to not provide any defaults.
and maybe even an option to force a value for working with bad implementation (or not, could also ask user to extend the factory for such edge cases).
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 agree, I actually asked myself the question a few times, if that's your sentiment then I'll implement this as well.
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.
@dbu Making the default lifetime configurable is what I would want to do, but I have a very theorical question about this.
There is basically two options:
- Either make it configurable at the provider level, but as it is now, providers are not configurable, they are dead simple. Adding some common configuration options on
AbstractProvider
would either mean force new parameter constructors (or setters) and make them pass all the way from the concrete implementation to the abstract implementation. I don't like this, I do it when it makes sense, but I don't think it does here. Furthermore, I'd like to keep those completely stateless. - Either make it configurable on a per-credentials basis, I would tend to allow user to give a different default lifetime depending upon the remote service, not the provider base implementation, it's much more flexible!
So this poses a new question, fetchers are stateless, more or less, access token manager is too, so the credentials class must carry it, here again, two options:
- Either add a
CredentialsInterface::getDefaultLifetime(): int
and implement it over all credentials, then use it in the providers. - Either simply let each implementation does it by itself, i.e. using an additional GET parameter over the user-provider DSN (for example:
oauth://service.tld/oauth/token?client_id=foo&client_secret=bar&default_lifetime=600
and deal with that in each specialized credentials factory.
In all cases, I'd probably wish to keep the default value on the AccessToken
class, but it would allow each credentials to carry its own value here.
I'd lean toward the second option here, what do you think ?
TL;DR:
- Let the user add a default_lifetime=N option in its URL.
- Let each specialized credentials factory deal with it.
- Let each specialized provider deal with it.
- No method added in interfaces.
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.
ah, i see the credentials also provide the endpoint url. is the credentials ~ the value object for the DSN? then it would make sense to have the options on the credentials indeed.
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.
Yes, indeed, external services such as Microsoft will give you different token URL depending upon your tenant identifier, so one way or another, the user will have to write its URL in configuration. There's of course some cases for specific providers that would handle by themselves the service URL (which is planned using foo_service://default?options=...
) but for a generic OAuth implementation, that's simply not possible.
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.
Moreover, I think this is an edge case, because in theory, the remote service will always expose the token lifetime.
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.
@dbu Sorry, it took me a while, I was absent for 10 days. I implemented it this way:
- Add
CredentialsInterface::getDefaultLifetime(): int
which is implemented per default onAbstractCredentials
returning either an arbitrary constructor set property, or the default 600 value if null. - This new method can be implemented by each provider to give a sensible value.
- Add
&default_lifetime=X
query parameter handling in existing factory in order to propagate the user given value, on a per-url basis.
Do you have any opinion or suggestion about this ?
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.
great, to me that seems like a clean and no-surprises solution that should cover all relevant scenarios.
src/Symfony/Component/AccessToken/Bridge/OAuth/ClientCredentialsProvider.php
Outdated
Show resolved
Hide resolved
@dbu thank you very much for your review, please ask any questions or don't hesitate to ask for more inline documentation when something doesn't seem clear enough to you ! |
8351a2a
to
d981a14
Compare
92d2839
to
39eaaf1
Compare
[AccessToken] fix licence year fix
[AccessToken] apply cs fixes (again) [AccessToken] CS fixes [AccessToken] some pslam fixes (again)
…ng directly the AccessTokenManager service as a factory
39eaaf1
to
f34e667
Compare
Access Token component
Use case (TL;DR)
When you develop a Symfony project that needs to consume data from a remote service/API, in B2B (Business To Business) / MTM (Machine To Machine) mode, without any user interaction, in most cases you will need to authenticate your HTTP requests for this remote service. It's rather common that this authentication is done by a Bearer access token.
This component aims to provide ways to configuring credentials then retrieve those tokens in a generic manner. It also aims improve performance improvement using caching for storing those access tokens for their lifetime, and to ensure consistency by using locking around remote token fetch, in order to avoid parallel accesses to the remote endpoint when a token expires.
In this scenario, the Symfony application is a client, not a server, requesting tokens and consuming data, and not the other way around.
Rationale
This main purpose component is to provide a common interface for fetching and refreshing remote access tokens, for webservices bearer tokens, SMTP XOAUTH, etc... as well as a some commonly used bridges.
The obvious use case is for managing OAuth2 token credentials, using the
client_credentials
andrefresh_token
grants, which are provided by a remote service and have short life time, in order to use them as a client only. Theauthorization_code
flow is voluntarily excluded because it's meant to trigger a user interaction, which is out of scope right now (but may be later).It's important to keep this in mind, OAuth2 is a standard, and security wise very hazardous to implement by one self without using a production-ready, industry grade, well maintained specialized component. This component is not going to provide any crypto related or server side code: it's meant for B2B / MTM token exchange only. This component is not likely to introduce any security issue.
The main goal being to provide a token fetcher and manager generic API interface, which is not OAuth2 specific, thus may be used for various other kind of tokens. In real life, most token-based authentication seem to be inspired or variations of one of the OAuth2 grant flows.
Why would Symfony need this? For a few different reasons:
symfony/notifier
transports, especially theOrangeSmsTransport
that handles the token fetch by itself.symfony/mailer
component holds theXOAuth2Authenticator
which only accepts an hardcoded access token from configuration, without any refresh logic, which is carried by theDsn
instance upper in the stack. Tokens are almost always bound to a life time, sometimes a few minutes, sometimes a year, and those tokens cannot simply be set in configuration for this exact reason. Even if given via environment variables: changing it almost always in the current state requires a manual operation, or sometimes some services restart.Bearer
authentication or similar to authenticate into third party web services is a common task that all developer probably writes at least once per project.And why would it be better to have a component than doing it manually, like it is done in the
OrangeSmsTransport
? Well, for many reasons, one being that a few mistakes are common to see:OrangeSmsTransport
, it will fetch a token for each notification being sent, but the token it gets is valid for a certain amount of time: it's a waste of resources and a performance hog not to reuse it.symfony/mailer
Dsn
class use case, the configuration and the token will be cached in memory, which means that when used through a service that lives during asymfony/messenger
bus process long-lived runtime, the token could get invalidated during runtime. Fixing it requires a restart. We can fix this by forcing the token to be systematically fetched for cache, validated, and refreshed programmatically, for each use.When you start to put all pieces altogether, it shows to be more complex than simply doing an HTTP request to fetch a token. In most real life scenarios we always want caching, validating and refreshing features to be implemented. This is even more true now that the
symfony/messenger
component is widely adopted by the community.My personal experience with this is that, in the company I work for, each project owns its own implementation of this lock, cache, validate and refresh algorithm. Sometimes more than once in a single project because we need to interconnect with various third party remote web services API, each having its own dialect for fetching tokens. That is the main reason why we want a generic, reusable and well-maintained component for this. We hope and do expect it will also be in the interest of many other people.
Proposed design
Main concepts
There the following concepts/interfaces which are really important here:
ProviderInterface
is what the bridge must implement: it has one method for fetching a new token, another one for refreshing an existing token. The abstract base implementation merges the two methods into a single fetch new token interface. This interface is hidden for the end user.CredentialsInterface
is a DTO that is typed and carries user credentials information, bridges must implement their credentials as well, this interface is exposed to the end user. Its only generic feature is to be able to produce a reproducible identifier for caching and locking.FactoryInterface
is what the bridge can implement in order to enable user-driven credentials configuration. From an URL schemes, it produces aCredentialsInterface
instance suitable for this component usage.AccessTokenManagerInterface
is the user facade, it provides the same two methods, but they are different in the sense that they only work with aCredentialsInterface
parameter.AccessTokenFetcher
is a service created from user YAML configuration, it is a replica ofAccessTokenManagerInterface
without the credentials parameter, since it already knows the user configuration.The access token manager works in this way:
Caching and locking
Caching and locking are done by decorating the
AccessTokenManagerInterface
.Caching should do:
CacheInterface
contract.Locking:
Note: for locking, there is probably ways to improve it, such as getting a read-only lock on first cache get, we are eager to hear your suggestions.
For this to work, cache and lock, we need for each credential a unique and reproducible identifier, that's why the only public visible method on
CredentialsInterface
isgetId(): string
. It's up to the implementation to compute a unique identifier.Note:
getId(): string
method could boil down to a__toString(): string
method which returns the URI (which is supposed to be unique). I have no strong opinion about this so all suggestions are welcome.In default configuration, lock decorates cache which decorates the default implementation in order to prevent concurrent cache accesses: a thread could fetch an invalid token from cache while another thread is fetching a new one, here this scenario is impossible. In case you prefer performance over consistency, it is possible, per configuration, to disable the lock completely.
Configuration and factories
In order to allow user configuration in YAML and environment, a piece of code is needed to glue user strings to
CredentialsInterface
instances, it's theFactoryInterface
:oauth
for all OAuth implementations, that requires agrant_type=
parameter in the URL to discriminate).During container compilation, each user given credentials in YAML configuration is parsed, in order to check for validality, then an associated service factory is registered that creates an
AccessTokenFetch
at runtime when requested.Usage and configuration
Overview
Component in its current state can be used with three different workflows:
AccessTokenManagerInterface
service: user handles its credentials configuration manually, creates aCredentialsInterface
implementation instance and passes it to the manager service for fetching the token. For example, sinceOrangeSmsTransport
already carries its credentials in the user configured DSN from the mailer component, it would be appropriate to keep it that way for backward compatibility and simplicity.access-token
component (which can be retrieved from environment variables like the database URL or SMTP DNS are) which is injected as aAccessTokenFetcher
instance into the container, one per configured credentials. This method would be preferable for users that need to extensively use this component for various services at once: it allows them to register all in configuration, pass credentials via environment variables, and keep their services implementations simple.access-token
component (which can be retrieved from environment variables like the database URL or SMTP DNS are) which is injected as aCredentialsInterface
instance into the container, one per configured credentials. The user can use theAccessTokenManagerInterface
by passing over the credentials that were injected.Using
AccessTokenManagerInterface
directlySimple API user code example using OAuth
client_credentials
grant flow:And that's pretty much it. Considering the user doesn't need to implement its own bridge and one of the core provided one fits the need.
Using YAML configuration
AccessTokenFetcher
Let's start by configuration:
The environment variable would look like:
Then, the same user code as upper would boil down to:
Implementing a bridge
Implementing a bridge consists in three different things:
CredentialsInterface
concrete implementations. This is mandatory.ProviderInterface
concrete implementations that deals with the associated credentials. This is mandatory.FactoryInterface
concrete implementations, that allows the credentials to be tied to a scheme and registered to allow the user to drive their configuration in YAML. This is optional, but if not implemented, cannot be YAML configured.About the current code state
It may be incomplete, nevertheless it's a fully functional, working, MVP. It holds the most important structural pieces and our opinion is a sufficiently good design to be used :
Per default, the choice has been made to prefer consistency over speed, in all cases, locking then cache access will be significantly faster than doing an HTTP request for fetching a token. In high throughput scenarios, this is probably a sensible default, for all others, it won't make a visible difference.
Naming can be wrong at some places, we are opened to any suggestion. Especially for shortening some interfaces names. Actual names are the most verbose and expressive names we could find, but they are great chances that you might have better ideas than us!
Known limitations
It's not possible, in the current state, to detect remote access token invalidity in a generic manner (e.g. you send the token to an SMTP server and it gets rejected) because the remote service implementation is opaque, errors can differ depending upon the remote service ("535 Authentication Unsucessful" for SMTP, "401 Authorization Required" for HTTP, etc... and which
\Exception
class ? It depends upon the user implementation or library in use). So, the final try/catch-error/refresh-token/retry algorithm must be implemented by the user, as shown in the previous example.DSN parsing is simplistic for now, and improvements will be done, in order, for example, to be able to combine HTTP Basic Auth with OAuth2 client credentials, this is quite common to stumble upon remote services requiring it.
What it is not
I'm repeating it, because it's important:
So now, what?
We are seeking for reviews, and will glady accept any opinion, any criticism. We do really hope this would get adopted, because this would improve:
Future plans
Note about dependency
In order to avoid creating hard dependencies to other components (notifier, mailer), there is two different ways I can see right now:
access-token
component, and inject it properly with some container compilation magic.