Admin users can't create keys to other users

Here's a patch with a start on this. I'm passing a {user} param in the add route, and then trying to use an AccountInterface in my AddForm to fetch the user, that should be passed in a parameter with this route, although, my $user object is always NULL.

Here's another patch. I'd love if you could take a look.

Seems that keys are being created properly per user now, but they are not visible, which is bizarre, as they exist in the database.

I'm going to keep digging on this, but if you have any thoughts, I'm all ears :)

Looks like it was a cache issue, hah :/

The #6 seems to work well, I'm going to work on some custom auth checks now to make sure people can't mess with other users keys.

So I have some custom auth checking going on.

There is one problem to iron out here... Logged in as an admin user, this works great.

Logging in as a regular user, the oauth.user_consumer route seems to get cached.

I can add and delete keys, although the list itself is never updated. However, you can check in mysql and see the keys being added and deleted successfully.

I'm probably missing something, thought I'd throw this patch up anyway :)

Oops, that patch was missing some stuff, here we go!

Updated tests.

Ok, I went off down the rabbit hole... and this patch is a large refactor of the way we are storing consumers, it's all chx's fault. (Thanks for the guidance @chx)

I am happy to split this out in to a couple of patches, let me know if you'd like to do that...

I'm opting for storing consumers as UserData (and using the 'user.data' service, instead of using a custom SQL table)

Let me know if you want me to split this patch up in to this issue, and then perhaps one for a refactor of consumer storage?

I thought I'd wait on updating tests until I know what you'd like to do here.

Thanks, this has been fun and I've learned a lot! :)

This patch allows admin users to administer other users keys.

This patch introduces a new custom access check for routes, and a new way of storing consumer data (in UserData).

I've got tests passing locally. Happy Hogmanay!

Actually, I re-ran tests today, and realized my patch removed the schema from the .install, and I ran tests prior.

Running tests and the last test pukes, so tests still require work, back at it next week some time ;)

Ok, tests passing now, I forgot to remove the hook_user_delete() which was calling db_delete on a non-existent table, now that we're using UserData :)

I have these loops fixed in my local, just trying to understand how I write this update hook to port consumer data and then delete the old schema ;)

Here we go :D

How's this? :)

Doh, here we go!

Looks like the patch file got added to the patch?

+++ b/oauth-admin-2627994-22.patch

phpcs is showing errors added in this patch. I'm about to submit a phpcbf run against 8.x-1.x, but even after merging that here I'm getting the following. This is also failing all the tests on my local, so I haven't done an in-depth review yet.

FILE: ...nt/d8/www/docroot/modules/oauth/src/Access/CustomAccessCheck.php
----------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 3 LINES
----------------------------------------------------------------------
 21 | ERROR | Doc comment short description must be on a single line,
    |       | further text should be a separate paragraph
 23 | ERROR | Missing parameter comment
 23 | ERROR | Missing parameter name
 27 | ERROR | Return comment must be on the next line
----------------------------------------------------------------------


FILE: ...odules/oauth/src/Authentication/Provider/OAuthDrupalProvider.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 26 | ERROR | Class property $user_data should use lowerCamel naming
    |       | without underscores
 45 | ERROR | Missing parameter name
----------------------------------------------------------------------


FILE: .../d8/www/docroot/modules/oauth/src/Controller/OAuthController.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 34 | ERROR | Class property $user_data should use lowerCamel naming
    |       | without underscores
 42 | ERROR | Parameter tags must be grouped together in a doc
    |       | comment
----------------------------------------------------------------------


FILE: ...t/d8/www/docroot/modules/oauth/src/Form/OAuthAddConsumerForm.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 37 | ERROR | Class property $user_data should use lowerCamel naming
    |       | without underscores
----------------------------------------------------------------------


FILE: ...8/www/docroot/modules/oauth/src/Form/OAuthDeleteConsumerForm.php
----------------------------------------------------------------------
FOUND 7 ERRORS AFFECTING 4 LINES
----------------------------------------------------------------------
 39 | ERROR | Class property $user_data should use lowerCamel naming
    |       | without underscores
 64 | ERROR | Missing parameter comment
 64 | ERROR | Missing parameter name
 66 | ERROR | Parameter tags must be grouped together in a doc
    |       | comment
 66 | ERROR | Missing parameter comment
 66 | ERROR | Missing parameter name
 76 | ERROR | Public method name "OAuthDeleteConsumerForm::getFormID"
    |       | is not in lowerCamel format
----------------------------------------------------------------------


FILE: ...rew/vagrant/d8/www/docroot/modules/oauth/src/Tests/OAuthTest.php
----------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
----------------------------------------------------------------------
  28 | ERROR | Visibility must be declared on method "testConsumers"
  74 | ERROR | Visibility must be declared on method
     |       | "testRequestAuthentication"
 138 | ERROR | Visibility must be declared on method
     |       | "testConsumerDeletion"
----------------------------------------------------------------------

Time: 315ms; Memory: 7.5Mb

Woops! Let's try that again!

According to chx, it's not possible to support this in DCI currently:

chx: willwh: you cnat test that on DCI ATM
chx: willwh: it should not be hard to add a plugin to DCI to allow a PECL isntallation step
chx: willwh: but it'd require a C building environment which is slow and slow is expensive
willwh: my tests are passing locally, and I’m okay with that
chx: willwh: and this is a really rare request

are you guys ok with tests passing locally? :)

I've applied this patch to my local codebase and can confirm it does what it's supposed to.

* Applied patch
* drush updb -y
* drush cr
* Confirmed existing keys moved to user.data
* No oauth_consumer table
* Created a key as another user and confirmed uid in user.data
* Delete a key from uid 1 which was removed
* Delete a key from new user which was removed
* Succesfully tested REST endpoint which required oauth key from new user.