Make the user authorization step optional

Add back page title and correct coding standards.

I think that we may need a different approach. We could track (using the State API) if a user has allowed a consumer before, regardless of 3rd party or not. If the consumer was already allowed by this user in the past, then skip the confirmation form in subsequent requests.

There should be a UI for users to revoke this trusted access for consumers under their profile somehow.

@skyredwang @alan_blake you make good points. These are separate issues. However I see the your skipping issue a particular use case of my state tracking issue. Maybe in your example YouTube would always return $accessPreviouslyConfirmed === TRUE based on the 3rd party field.

We may work these in separate tickets, but I think one would be blocking the other. Would you agree?

Yes, I think we are on the same page!

One last detail. Where do you think the third_party field fits better? I would say that it's best to have it in the Consumers project. My reasoning is that other projects may want to leverage that info even without needing simple_oauth.

@skyredwang, @alan_blake a good first step may be to add the field definition to the Consumer entity type. Do you agree?

remove third-party field and add $accessPreviouslyConfirmed determine statements.

Add test cover third_party field.

@alan_blake tests have failed. Can you please take a look?

1. +++ b/simple_oauth_extras/src/Controller/Oauth2AuthorizeController.php
   @@ -0,0 +1,119 @@
   +    $client_uuid = $request->get('client_id');
   +    if (empty($client_uuid)) {
   +      return new Response('Client authentication failed', 401);
   +    }
   

   Not sure if we should return plain responses like that, this isn't an API but something that actual users are accessing.

   That said, this is not a case that a normal user should ever see.

2. +++ b/simple_oauth_extras/src/Controller/Oauth2AuthorizeController.php
   @@ -0,0 +1,119 @@
   +    $client_drupal_entities = $this->entityTypeManager()
   +      ->getStorage('consumer')
   +      ->loadByProperties([
   +        'uuid' => $client_uuid,
   +      ]);
   

   you could also use loadEntityByUuid() from the entity.repository service for this

3. +++ b/simple_oauth_extras/src/Controller/Oauth2AuthorizeController.php
   @@ -0,0 +1,119 @@
   +    $accessPreviouslyConfirmed = FALSE;
   

   this is the only local variable that uses camel case

4. +++ b/simple_oauth_extras/src/Controller/Oauth2AuthorizeController.php
   @@ -0,0 +1,119 @@
   +    $previous_token_entities = $this->entityTypeManager()
   +      ->getStorage('oauth2_token')
   +      ->loadByProperties([
   +        'auth_user_id' => $this->currentUser()->id(),
   +        'client' => $client_uuid,
   +      ]);
   +    if (!empty($previous_token_entities)) {
   

   seems pretty implicit to rely on an existing token? even refresh tokens eventually expire and wouldn't the most likely case to re-visit to be exactly when even the refresh token has expired?

   Also, this would introduce a pretty serious security issue as far as I see, because it does not consider scopes. That means a client could first ask for a limited scope and then do a second request with additional scopes that is automatically approved.

   Seems to me that approved/connected clients should be stored explicitly together with the approved scopes (or they should be checked for the scopes at least).

   And if we'd have that, it should probably also be shown to the user so he can decide to delete clients again.. I can do that on twitter/google/..

   So, seems to me that this connected-clients business could get fairly complicated. It also doesn't seem to have test coverage yet, unlike the third-party setting. Maybe it would be best to split it out into a new issue and focus on the third-party setting here?

1. +++ b/simple_oauth_extras/src/Controller/Oauth2AuthorizeController.php
   @@ -0,0 +1,119 @@
   +    $client_drupal_entity = reset($client_drupal_entities);
   

   didn't notice this before, this is a strange variable name, why "drupal", nobody said that the client is drupal? IMHO just use $client/$clients or $consumer/$consumers

2. +++ b/simple_oauth_extras/src/Controller/Oauth2AuthorizeController.php
   @@ -0,0 +1,119 @@
   +    if ($this->currentUser()
   +        ->isAuthenticated() && ($client_drupal_entity->get('third_party')
   +          ->getString() == 0 || $accessPreviouslyConfirmed)) {
   +      if ($request->get('response_type') == 'code') {
   

   One more thing, getString() should not be used like this. This will return a comma separated list all field items, each item having its properties comma-separted as well.

   This happens to be a field type with a single property, so the result of all that logic is just that single property, but that's a complicated way to get that and not that reliable. Just use ->get('third_party')->value == 0 or even just !->get('third_party')->value.

Thanks for the work here @alan_blake! You guys at Sparkpad are leading the charge in several fronts. Kudos to you.

Some thoughts about @Berdir's reviews (thanks for that @Berdir!):

#24.1: I agree. An HttpException should be caught by the appropriate response subscriber to turn it into JSON. Maybe try that and see what happens.
#24.2: True. However we've been doing that in other places already in the scenario where we are already injecting entityTypeManager for other means. That allows us to skip an injected service. Any downsides to this approach?
#24.3: +1
#25.1 There is some clash of terminology in the Drupal realm and the thephpleague/oauth2-server hence the weird naming. This happens elsewhere.
#25.2: Agree. Let's cast into a boolean !$client_drupal_entity->get('third_party')->value.

My own review:

This patch makes the Simple OAuth Extras module to depend on >=8.x-1.0-beta2. Let's make sure we add that to the info file.

1. +++ b/simple_oauth_extras/src/Controller/Oauth2AuthorizeController.php
   @@ -0,0 +1,119 @@
   +    $accessPreviouslyConfirmed = FALSE;
   +
   +    $previous_token_entities = $this->entityTypeManager()
   +      ->getStorage('oauth2_token')
   +      ->loadByProperties([
   +        'auth_user_id' => $this->currentUser()->id(),
   +        'client' => $client_uuid,
   +      ]);
   +    if (!empty($previous_token_entities)) {
   +      $accessPreviouslyConfirmed = TRUE;
   +    }
   

   Let's move this to its own method: wasAccessConfirmed(…).

   This could return a list of scopes that have been accepted. If the scopes requested are all in this list, then we can redirect without a prompt. If there is at least one missing scope, we need to prompt the user. This should also address @Berdir's concerns (please confirm).

2. +++ b/simple_oauth_extras/src/Controller/Oauth2AuthorizeController.php
   @@ -0,0 +1,119 @@
   +    if ($this->currentUser()
   +        ->isAuthenticated() && ($client_drupal_entity->get('third_party')
   +          ->getString() == 0 || $accessPreviouslyConfirmed)) {
   

   Let's introduce local variables to improve legibility.

3. +++ b/simple_oauth_extras/src/Controller/Oauth2AuthorizeController.php
   @@ -0,0 +1,119 @@
   +      if ($request->get('response_type') == 'code') {
   +        $grant_type = 'code';
   +      }
   +      elseif ($request->get('response_type') == 'token') {
   +        $grant_type = 'implicit';
   +      }
   +      else {
   +        $grant_type = NULL;
   +      }
   

   Is there a better way to detect that a flow requires manual user interaction? Probably not, but worth investigating a bit.

4. +++ b/simple_oauth_extras/src/Controller/Oauth2AuthorizeController.php
   @@ -0,0 +1,119 @@
   +      $server = $this->grantManager->getAuthorizationServer($grant_type);
   +      $ps7_request = $this->messageFactory->createRequest($request);
   +      if ($auth_request = $server->validateAuthorizationRequest($ps7_request)) {
   +        $user_entity = new UserEntity();
   +        $user_entity->setIdentifier($this->currentUser()->id());
   

   This code (and beyond) seems like a copy and paste from the submit handler in the form class.

   Can we abstract it and be DRY?

5. +++ b/simple_oauth_extras/src/Controller/Oauth2AuthorizeController.php
   @@ -0,0 +1,119 @@
   +      return \Drupal::formBuilder()
   +        ->getForm('Drupal\simple_oauth_extras\Controller\Oauth2AuthorizeForm');
   

   We should inject the form builder, even if it's just because DI is fun.

6. +++ b/simple_oauth_extras/tests/src/Functional/AuthCodeFunctionalTest.php
   @@ -99,4 +99,55 @@ class AuthCodeFunctionalTest extends TokenBearerFunctionalTestBase {
   +    $this->drupalLogin($this->user);
   +
   +
   +
   

   Nit: additional blank lines.

7. +++ b/simple_oauth_extras/tests/src/Functional/ImplicitFunctionalTest.php
   @@ -89,4 +89,42 @@ class ImplicitFunctionalTest extends TokenBearerFunctionalTestBase {
   +    $assert_session->statusCodeEquals(500);
   +    $this
   +      ->config('simple_oauth_extras.settings')
   +      ->set('use_implicit', TRUE)
   +      ->save();
   

   If this is a copy & paste from another test we don't need to verify again that the use_implicit flag works correctly.

8. +++ b/simple_oauth_extras/tests/src/Functional/ImplicitFunctionalTest.php
   @@ -89,4 +89,42 @@ class ImplicitFunctionalTest extends TokenBearerFunctionalTestBase {
   +    $assert_session->statusCodeEquals(200);
   +    $assert_session->addressMatches('/\/oauth\/test#access_token=.*&token_type=Bearer&expires_in=\d*/');
   

   We should also assert the requested scopes.

   Also, we will need a test for each scenario:
   a) The user did not accept before.
   b) The user accepted before for a more limited set of scopes.
   c) The user accepted before for a broader set of scopes.

This is really close. Some extra review considerations.

1. +++ b/simple_oauth_extras/src/Controller/Oauth2AuthorizeController.php
   @@ -0,0 +1,151 @@
   +    if ($this->currentUser()->isAuthenticated() && (!$is_third_party || $access_confirmed)) {
   

   Worth adding a code comment explaining what the condition is actually testing.

2. +++ b/simple_oauth_extras/src/Controller/Oauth2AuthorizeController.php
   @@ -0,0 +1,151 @@
   +      if ($auth_request = $server->validateAuthorizationRequest($ps7_request)) {
   

   If this is false, then the controller logic will not return. We must ensure that all execution paths return.

   We could explore removing the last else.

3. +++ b/simple_oauth_extras/src/Controller/Oauth2AuthorizeController.php
   @@ -0,0 +1,151 @@
   +  /**
   +   * Check the client was access same scopes confirmed before.
   ...
   +  protected function wasAccessConfirmed($client_id, array $scopes) {
   +    $previous_token_entities = $this->entityTypeManager()
   +      ->getStorage('oauth2_token')
   +      ->loadByProperties([
   +        'auth_user_id' => $this->currentUser()->id(),
   +        'client' => $client_id,
   +        'scopes' => $scopes,
   +        'status' => 1,
   +      ]);
   

   What we need instead is to find out if the current user is in possession of any active token. Then we should check that any of the retrieved tokens has all the $scopes.

   That way we cover the case where there is a token with $scopes and more.

Fix #29 1&2.

#29.3 Why we need to check the token is active? If client access token expiration time is short, the token soon will be inactive, and then user must grant again in a short time. That's not we want. I think we should only check the user grants the same or broader scopes on the same client before and the token is not revoked.

• Fix endpoint page cache problem.
• Fix get 500 Error if client scopes set to null and request token without scope parameter

Fix tests errors.

I'll be focusing my attention here soon. Sorry for the delay.

I made some changes to the patch to handle errors and make the test DRY.

There are still some small standing issues with the main logic from previous comments not addressed with regards to scopes.

Why we need to check the token is active? If client access token expiration time is short, the token soon will be inactive, and then user must grant again in a short time. That's not we want. I think we should only check the user grants the same or broader scopes on the same client before and the token is not revoked.

This limits the impact of session hijacking. If an attacker rides you session then they can get a long lived token to impersonate you even if the session expires. That impersonation in the future will be almost undetectable. That's why we want to limit this behavior to if the token is active.

I also added a slightly more intelligent reduced scope in the tests, instead of just dropping the scopes key to make sure wasAccessConfirmed is performing the right task:

Then we should check that any of the retrieved tokens has all the $scopes.

+++ b/simple_oauth_extras/src/Controller/Oauth2AuthorizeController.php
@@ -0,0 +1,183 @@
+      $previous_token_entities = $token_storage
+        ->loadByProperties([
+          'bundle' => 'access_token',
+          'auth_user_id' => $this->currentUser()->id(),
+          'client' => $client_id,
+          'scopes' => $scopes,
+          'status' => 1,
+        ]);
+      if (!empty($previous_token_entities)) {
+        return TRUE;

I'm still not convinced that this is a reliable approach.

access_tokens are usually (very) short-lived. And once they expired, they will get deleted.

Doesn't that mean that this whole logic will only work as long as there is an active access token?

That doesn't seem very useful, because it's unlikely that a user will access the page again *while* he is still has an active access token.

Maybe we could use refresh tokens instead, but they seem to be missing any reference to user/client/scopes based on what I see on the Tokens page.

[…] it's unlikely that a user will access the page again *while* he is still has an active access token.

That's a good point.

I was thinking that the feature request was mainly for cross-consumer authorization when they are not 3rd party. The second part I considered more of a nice to have. Should we just drop this second part?

On a side note, after another quick look at the code we only want to call wasAccessConfirmed if necessary. Move the call inside the if statement. That is if we keep the feature.

Yes, I suggested in #24 to move that to a separate issue. I don't think it's a nice to have, I think there are more use cases that need that feature than 1st-party clients. But I think it is better to split it up and focus on that in a separate issue.

Because I'm not sure if should implicitly, always do that. Maybe it should be configurable and probably users should be see what clients they gave access and be able to remove it, or maybe even make the remembering an opt-in feature with a checkbox. We don't have to support all that initially, but at least think about it..

It is something we need for our project, so I will most likely be able to work in it in the next 1-2 weeks.

I'm not sure what you are arguing for exactly, but I fully agree that we want to have some version of what you are describing. But the patch right now doesn't work like that.

With oauth2, the access token that is being checked here is only valid for a *very* short time period. Could be 1 minute, maybe 5 but in most cases, that's it. It is *not* tied to the session that you have with the Drupal site in any way. Once it expired, the client can use the refresh token and get a new access token that is again valid for a few minutes.

And when the Drupal site (aka Google SSO in your scenario) runs cron, which it for example might do every hour, then it will delete that access cookie once it expired. So if the user logs out after a few minutes and then wants to log in again 1h later, it will *not* remember him with the current implementation.

And with Google, it is actually not tied to the session, Google SSO has a persistent list of connected sites, as long as I don't go there and remove it, logging in again on some other site will auto-accept it again, even if I have a new session on Google. This is basically what I'm proposing. But my suggestion is to do that implementation (or a first step toward something as fancy and complete as Google SSO) in a separate issue. One feature at a time.

That sounds like consensus to my ears!

Anyone willing to create the follow up issue, so we can start trimming the patch in this issue?

So I remove the valid $accessPreviouslyConfirmed part. This patch skip the user authorization step only if the client is not third party.

Started doing manual testing and it basically works, but I think we have a bit of a problem with how the third party field works that was added to the consumer entity.

It was added as a new field, with default value FALSE. That means all existing clients are now *not* third-party clients but are trusted. Combined with this patch, just by updating those two modules, existing sites will suddenly automatically grant access to all existing clients until they edit them again.

I think the default value for the third party setting field should be TRUE as that is the more secure choice if you're not sure what you want and is then consistent with not having the field, where every client was assumed to by third party. The UI could also do a better job at explaining the implications of being/not being third party. Which is of course tricky as the field is now defined in the consume entity.

Also, the patch should have added an update function to install the new field definition, stable modules should *not* rely on users having to run drush entup themself, not everyone can (granted, most people using these modules can, but it should still not be required and should not be part of automated deploy processes).

Not quite sure how to move forward, but I would suggest a new issue in the consumer project to make those changes. But lets wait for feedback from @e0ipso.

I created #2948267: Allow users to remember connected clients.

To be clear, I'm happy to work on the changes that I proposed above, just would like to have an OK from @e0ipso before I start.

@Berdir I agree, and that's why I set default value TRUE on my patch.

Oh, and @e0ipso changed it on commit? That doesn't seem like a "very minor change", weird.

And in regards to the upgrade path, I see, you did add one, but that is not quite correct. When working with entity fields, you should be using the entity update manager to make schema changes. You added the field (without explicitly setting existing clients to TRUE) to the schema, but the entity system didn't know about that.

See block_content_update_8003() for an example that creates new fields on an existing entity type. And starting with 8.4, it is also possible to provide an explicit value, even depending on another field. See block_content_update_8400() for an example for that (that only exists in 8.5.x despite its name)

@Berdir the reasoning is that (at least at the moment) you need administer consumer entities to create any Consumer. I consider administering an entity type a high clearance level. If you have that permission it implies:

• You have responsibilities in the Drupal site, hence your consumers can default to 1st party.
• You know what you are doing, hence you should not forget to tick the 3rd party box when creating a consumer for someone else.

In addition to that void (before adding the field) and FALSE (after adding the field) are both falsey, making the transition smoother (hopefully). Finally, first party consumers is the most common use case (by far) in decoupled Drupal according to my observations.

I want to be clear on this: I'm not married to that decision. If we collectively think that the reasoning above is incorrect, or there are more important reasons on the other side of the scale, I'm open to change. Should we change it?

@Berdir I agree, and that's why I set default value TRUE on my patch.

Oh, and @e0ipso changed it on commit? That doesn't seem like a "very minor change", weird.

I tend to do that. Some times it remember to post a commit-interdiff.txt, but in all honesty that's not the majority of the cases. As you well know maintaining a module can be a burden, I try to be practical and alleviate some chores wherever I can. Hopefully the commit is enough to trace back the changes that happened.

Also, the patch should have added an update function to install the new field definition, stable modules should *not* rely on users having to run drush entup themself, not everyone can (granted, most people using these modules can, but it should still not be required and should not be part of automated deploy processes).

You are absolutely right. I just didn't know any better. entup has never been a problem in my setups so I never bothered with a different way of doing it. How would you suggest to do it?

> In addition to that void (before adding the field) and FALSE (after adding the field) are both falsey, making the transition smoother (hopefully).

They are both falsey as a value, but the meaning is not the same.

Before this patch, everything is third party and the user needs to confirm the client. With the patch, false means *not* third party and is automatically accepted.

It seems wrong to me that this automatically happens by just updating the module. So for BC, IMHO void is the same as having it set to TRUE.

I have no strong opinion on the default value for *new* consumers, but I really think the update function should explicitly set it to TRUE for existing entities.

> How would you suggest to do it?

I would suggest to add an explicit update function similar to the examples above that sets the initial value to TRUE. Problem is that sites that already updated now already have the schema, so it won't change that.

Considering that, we should possibly even explicitly again set all entities to TRUE, maybe with message to notify users about this on update. It would reset sites that already explicitly created them as first-party but that should be OK as nothing really relies on that yet until this issue is actually committed? (except they do in custom code..)

Actually found an existing issue, provided a patch there: #2938932: Intall 'Is this consumer 3rd party?' on update

> Before this patch, I would image all use cases are for 1st party (otherwise, this issue, discussion and patch would have happened earlier)

I don't understand this.

Before this issue, every user was asked for every client if he wants to allow access. With the logic in this patch, that means that client is considered third-party. So to keep showing that form to users, they must be marked as third-party clients, and then site admins can decide to make them 1st party if they want to.

That's what my patch in the other issue is now doing, but it will only work for those who didn't update to beta2 yet, I guess it's not worth doing it explicitly for those too, it is just a beta version (but a dependency of a stable module, so technically, it should be considered stable).

I think combined with my patch from the issue above, this is fine.

Thanks all!

Sorry for not explaining before. Every time I (as a maintainer) require multiple changes and tests to a contributor and they keep up with the work, I grant several (empty) commits for the credit. I hope this encourages you to keep the great work @alan_blake!