Refactor Authorization Token Flow

Hi @redlucas25,

I looked through the code and I have a question. There is a separate method to get the $resource_owner_uri and with your changes, we will be returning a new GenericProvider with urlResourceOwnerDetails set to TRUE. Is this intended?

Perhaps we should define $redirect_uri in the Annotation class and just expand the logic in the GrantServiceBase to check if client has the redirect_uri defined and in case it's missing - generate it from current route.

@Postovan Dumitru, agreed. I don't think we need to touch the resource owner-related code within the scope of this issue. Here is a patch that does what you described. I tested this against the TdAmeritrade API, and it worked well.

@malikkotob, thanks for the effort!

I do not know if the module maintainers are going through the issue queue though, might take some time before this is reviewed/commited.

@Postovan Dumitru agreed! I can try to reach out to them. In the interim, we should be able to move this along to a status of RTBC if you or @RedLucas25 have a chance to review/test.

@malikkotob,

I implemented this initially with the idea of providing a patch, but then I ended up extending the existing grants, needed some custom approaches.

As for the patch you provided, I can confirm that it works for me, at least when I provide the redirect_uri in the plugin annotation.
What I suppose might be an issue is this:

public function getRedirectUri() {
    $this->checkKeyDefined('redirect_uri');

    return $this->pluginDefinition['redirect_uri'];
  }

and this:

  /**
   * Check that a key is defined when requested. Throw an exception if not.
   *
   * @param string $key
   *   The key to check.
   *
   * @throws \Drupal\oauth2_client\Exception\Oauth2ClientPluginMissingKeyException
   *   Thrown if the key being checked is not defined.
   */
  private function checkKeyDefined($key) {
    if (!isset($this->pluginDefinition[$key])) {
      throw new Oauth2ClientPluginMissingKeyException($key);
    }
  }

Just my guess, but I think we must declare the annotation variable with a default value of empty string, this way, it will always be set and not throw the exception. But what I really want to know is if it's an acceptable approach...
I would really appreciate if you test this out and modify the patch if needed, I would but I'm short on time. Otherwise I can give it a try the upcoming weekend, thanks!

Hi RedLucas25,

I think I've ran into the same issue as you did and it was related to the refresh token plugin.
Unfortunately, it has nothing to do with this patch, the error you would get is deeper down the code where it would tell you that there is a mismatch of the redirect uri or something similar.

@malik.kotob patch is applying and working perfectly. Thank you

hey everyone,

we noticed that for using the ebay api you need to set a specific redirect uri but it seems the plugin completely ignores what we set and just generates a uri to the front page everytime. this patch is similar to what we fixed. can this please be reviewed

I think this problem is related to this issue. Please let me know if isn't.

In the case where grant_type = "client_credentials" no redirectUri should be sent. How can I set that option?

\Drupal\oauth2_client\Service\Grant\Oauth2ClientGrantServiceBase::getProvider doesn't seems to allow that option. I'm correct?

I've tried the patch, but because redirectUri is empty it always generates a redirectUri from .

Thanks.

@malik.kotob: Perfect!

@introfini: I think that would be a separate issue. This patch only changes what is set for the redirect_uri parameter, not whether it should be set.

Hi! @perelesnyk and I have stepped up as maintainers, officially for D9 compatible and forward. Can those who have worked on this issue give some context on why the redirect_uri should be configurable in general?

As I understand it, the purpose of this URI is to capture query parameters here on the client side. Working on #3187565: Key module integration today I see that the plugin base class is currently processing these. What seems correct to me is to define a route and path explicitly for this purpose with a controller to complete the capture and respond appropriately. Something like /admin/config/system/oauth2-client/{plugin}/code so that we have the plugin id and can load it.

If there is a need to make this configurable that I'm not seeing, we can leave it as an annotation. Otherwise I'd move it to a fixed URI. So I'd like to hear from those who have worked on this issue before I switch this issue to the new version and extend the work that you have done here.

@FatherShawn - That's great news! I just put a fair bit of work into building our authentication around this module, so I'm glad to hear it will be maintained!

The reason for setting the redirect url is that I need authentication to happen from any page. This allows me to have one touchpoint that can reference a saved "destination" parameter.

@zerbash Indeed! Using a path for a redirect url is a use case I have a hard time understanding.

I just drafted a revision which defines a route for the path

/admin/config/system/oauth2-client/{plugin}/code

where {plugin} is the client plugin id. The client plugin returns an absolute url with this path as the redirect URI. A controller on that route then captures the code and validates the state. This also reduces code complexity in the Authorization Code grant service.

That sounds like it meets your use case?

Note that #3187565: Key module integration (merge request) refactors the routing.yml file, which is also refactored here, so I'm extending that work in my current draft here.

I tried to test it out yesterday, but kept running into bugs -- looks like you were a step ahead of me on the key integration branch. I think I have it mostly working with a hodgepodge of tweaks and patches. Enough anyway that I can answer your question:
Yes! I think that will work just fine, if:

• The callback is publicly accessible: users authenticating against an OAuth provider may or may not have accounts.
• The plugin can handle the response.

@zerbash First off, thank you so for the collaboration!

Yes- I tested the key integration with a resource_owner grant. Using the auth code grant revealed more bugs, which I fixed in that branch as I went along.

users authenticating against an OAuth provider may or may not have accounts.

I'd like to hear more about that. Our perspective is that it's not a good security model to reveal confidential information in annotations that are then committed to version control. How do you envision capturing and storing client id/client secret for anonymous users. That's really a question, I suppose for #3187565: Key module integration where we are doing that refactoring but clearly there is a connection between that issue and this.

I agree 100% with keeping config data out of version control. I had already been using the key module in the plugin to override getClientId() and getClientSecret(). Happy to see #3187565: Key module integration!

In this scenario, all users are sharing the same client id/client secret -- all that's doing is confirming that the auth provider is legit. After authentication, the returned token should then be stored in the privateTempStore. And that is it's own thing: #3182425: Authorization Code Grant Issues

Rebased recent work onto updated 3.x branch so it's clearer what is proposed in the issue fork.

@zerbash - What I'm not following in your use case, if all users are sharing the same client id/client secret, is why the auth code grant workflow needs to be re-executed by your anonymous users. What I would imagine is:

1. An admin sets up the auth_code grant workflow, authenticates on the 3rd part system, resulting in the Drupal site capturing the code, and requesting tokens.
2. An AuthToken object is saved, containing both an token and a refresh token
3. A user, including an anonymous user, takes an action that triggers a request to the 3rd party system.
4. The AuthToken is loaded from State.
5. If it is expired, the refresh token grant is used to replace it.
6. The request is executed with valid token.

Where does this breakdown for your use case?

Ah, that's why you're making the call on the config page. I assumed that was just for validation.

In our scenario, we have a student information system (not Drupal) that provides an OAuth endpoint. If a user requests a service from Drupal, we need to know who they are. Generally they are being redirected from the user portal on the SIS. In the spirit of SSO, it doesn't make much sense to then force them to log in to Drupal.

By executing the auth_code grant workflow, they will be redirected back to the SIS to authenticate. Because they are already authenticated, the SIS will respond with an access token, which we can then use to request whatever resource we may need for that user.

If this sounds a lot like OIDC, it basically is, but our SIS doesn't support the OIDC protocol. 🙁

Thanks for your full use case. I think that the base case is one AccessToken per plugin but I want to set this up so that it can be customized for different cases such as yours.

I've moved the response generation to the auth code grant service so that it can easily be overridden by decorating the service. The permission on the code capture route can also be overridden.

Assigning to @perelesnyk for a review

I guess part of my concern here is that there really shouldn't be a "base case." Ideally this module just assists in the retrieval of access tokens -- I don't think it should be making any determinations or assumptions regarding who is requesting them. I think rather than prescribing an intent on the base config form, that should be incumbent on the overriding plugin.

This would also get you out of the business of asking an admin user for his/her credentials so that you can authenticate against a third-party service -- isn't that the exact scenario that OAuth was developed to avoid?

What if the plugin were to have another method: setStorage() where a module could define where the AccessToken was stored? Setting a long-lived AccessToken in the state is essentially saving a username/password combination to the database -- which is largely what the key module aims to avoid. This would take care of your scenario, where the token could be set somewhere securely, and my case, where the token is relatively ephemeral.

Thoughts?

I do want to make it as useful as possible, which means extendible in the right ways. Whatever choices we make are what starts to define a base case. When the route for capturing the returned code is declared, it either is publicly accessible or has access control. Permissions add security but assume a logged in user. PrivateTempStore adds Session for an anonymous user, which has performance implications. None of these choices are correct so if others of the 11 who are following this issue would join this discussion that would be awesome! Otherwise @perelesnyk, @zerbash and I will make a call.

Yes, let's figure out what the dynamic elements are and those should be in the plugin. So far I hear additional dynamic elements as:

• access to the oauth2_client.code route proposed in this issue
• AccessToken storage system raised in #3182425: Authorization Code Grant Issues

I've extended this work @zerbash to make those two behaviors extendable in the plugin.

@FatherShawn -- that looks great! I still have a couple of issues:

OAuth2ClientPluginConfigForm::submitForm
I don't think the module should be attempting to retrieve/store a token here. If a plugin-providing module wants to store a token at the configuration stage, it can do so by overriding Oauth2ClientPluginBase::submitConfigurationForm.

/admin/config/system/oauth2-client/{plugin}/code
I like the idea of consolidating the code endpoint.. wouldn't it be easier/cleaner to allow the plugin to determine the (ultimate) Response rather than the grant service? Also, it probably shouldn't be /admin/config/system, since it's not really (or shouldn't be, IMO) related to config. Maybe just /oauth2_client/{plugin}/code?

@zerbash, first of all thank you for your valuable input.

Regarding your suggestions:
OAuth2ClientPluginConfigForm::submitForm
Me and @FatherShawn share the opinion that upon entering clientId and secret, the administrator should be able to check their validity/connection. So we may have it not as a default action upon form submit, but as a separate action. We consider adding submodules that will provide both customization guidance and functionality to some alternative usage scenarios, at the same time keeping our default scenario more or less intact.

/admin/config/system/oauth2-client/{plugin}/code
This endpoint is for the in-between leg of the OAuth2 protocol, limited basically to one grant type, so the outcome is standard and should be associated with that grant type rather than with the plugin.
Apparently, in your use-case you might want to customize the final leg of the exchange, the token retrieval. That can easily be achieved by decorating the relevant service, of which we will provide an example in the module's Readme.

Please let me know if I am missing any of the important parts of your use-case/inquiries. And thanks again for your help

I think it's fine to have the validation check, as long as the returned token isn't stored anywhere -- again, you wouldn't want a user to inadvertently store an access token in the database.

As far as the endpoint, is there a compelling reason to have it on the admin path? That path is going to bail if it's not the original requester visiting it, so it seems like requiring admin credentials is unnecessary and limiting.

My thought of letting the plugin (be able to) return the final response actually came up during this thread -- I had been content with setting the redirectUri (the original issue!), but then realized that the boilerplate code could be moved to this module.

After the back-and-forth is complete, we get
return $this->grantService->getPostCaptureRedirect();

Since the plugin is available here, why not allow the plugin to override this final Response? That would solve the original problem AND all functionality could be encapsulated within the plugin (no decorators, no endpoint definitions). Seems win-win.

I've added on screen documentation so that it's clear that a token is in fact saved if the user chooses Save and request token.

I've also moved the code capture endpoint to /oauth2-client/{plugin}/code there was no functional reason to have it on the same path stem as plugin listing. It was just the aesthetics of consistency :)

I didn't want to burden all grant flow plugins with a redirect method only needed by one grant type. I realized though that we could use an additional interface to segregate these overrides as needed. So that is now updated.

<gasp> ... It works! Well, sorta. Two changes I had to make:

AuthorizationCodeGrantService::getAccessToken() needs exit();at the end. It went away in 2ef71963.

AuthorizationCodeGrantService::getAccessToken() needs (well, I'd sure be appreciative if it had):
$this->currentRequest->getSession()->save();
Otherwise it doesn't work for anonymous users.

I don't think these changes would affect your use case, and if a small change will make the code more versatile...

I also noticed that OauthResponse::code() is setting the message "OAuth token obtained from remote website and stored." You use the same message in the form, and that's a better home for it. For my uses, the details of the authentication flow need to be invisible to the user.

Lastly, (and you've provided a storage override, so I really will stop beating this dead horse, but) I do think having the default token storage be state is really dangerous. Ideally, this module is going to be dead-simple to use. It's too easy to forget to implement a custom storage mechanism, and then every user of my module gets access to some third-party service using my credentials!

I'd say either
a) don't provide a default storage mechanism -- make that a requirement of the plugin to implement one.
or
b) provide some sane defaults that can be chosen during configuration (state, tempPrivate, key)
or
c) default to tempPrivate since at least any tokens a user has access to would be his/hers alone.

Again, I'm happy enough to implement store/retrieve/delete on the access token, but I confess to not having done so, and wondering why all subsequent users of my service were getting the first user's credentials... (Test users, fortunately!)

Thank you so much for the review and testing @zerbash

1. Added the missing line and session check/store.
2. I'll add an additional item to the definition/annotation to flag showing the success message and make them conditional. I added it here because in the shared single resource case, the config form message is never displayed for the auth code flow.
3. I really like the idea of not implementing a default. Especially since the plugin base is already declared abstract and we are including an example sub-module. I've added:
   

   /*
      * This example assumes that the Drupal site is using a shared resource
      * from a third-party service that provides a service to all users of the site.
      *
      * Storing a single AccessToken in state for the plugin shares access to the
      * external resource for ALL users of this plugin.
      */
   
     /**
      * {@inheritdoc}
      */
     public function storeAccessToken(AccessToken $accessToken) {
       $this->state->set('oauth2_client_access_token-' . $this->getId(), $accessToken);
     }
   
     /**
      * {@inheritdoc}
      */
     public function retrieveAccessToken() {
       return $this->state->get('oauth2_client_access_token-' . $this->getId());
     }
   
     /**
      * {@inheritdoc}
      */
     public function clearAccessToken() {
       $this->state->delete('oauth2_client_access_token-' . $this->getId());
     }
   

   to two of the examples. Would you post back your implementation and I'll add that to the other two examples?

@FatherShawn: Beautiful! Functionally, I think I'm in business!

For your consideration:

• Instead of implementing store/retrieve/clear, what about implementing an interface with get/set/delete methods? You use State and I'm using PrivateTempStore -- both can satisfy a TokenStorageInterface. (See attached example)
• You define $state and $uuid in Oauth2ClientPluginBase, but these are both specific to your needs, and save an unnecessary key/value for me. Better to move them to the plugin implementation?

Anyway, neither is a roadblock - thanks for all of your work and I'm looking forward to seeing this in production!

@zerbash Super news!

Both State and PrivateTempStore implement those methods but they do not have the type created by your proposed interface. One would probably need to add a service that implements the interface, which only seems worth the effort if a good number of our plugins are in use. I think I'm going to leave it as it is for now.

I use both $state and $uuid in the default storage (no Key module) of the client id/client secret so I need to leave them in place.

I'll take your example temp store implementation and put it in a couple of the example plugins

Merged so we can make an alpha release for easier testing.

updating credits

Release 8.x-3.0-alpha1 is now available