Page MenuHomePhorge

Laravel Passport support
ClosedPublic

Authored by mollekopf on Apr 22 2021, 12:13 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 16, 9:54 PM
Unknown Object (File)
Sat, Mar 16, 9:48 PM
Unknown Object (File)
Fri, Mar 15, 12:32 PM
Unknown Object (File)
Sun, Mar 10, 12:39 AM
Unknown Object (File)
Thu, Mar 7, 4:36 PM
Unknown Object (File)
Thu, Mar 7, 2:47 AM
Unknown Object (File)
Thu, Mar 7, 2:46 AM
Unknown Object (File)
Thu, Mar 7, 2:46 AM
Subscribers

Details

Summary

Replaces the use of tymon:jwt with laravel:passport.

A password grant client is used create tokens for the webclient in the
same fashion as we used to the tymon:jwt solution.
The same password grant client can be used for other client applications.

Notes:

  • We're not currently purging invalidated/expired tokens. This can be

done via artisan command or scheduled task from php.

Diff Detail

Repository
rK kolab
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
  • Custom authentication hook for passport (so we can insert 2fa)
  • Added 2fa to user verification
  • Rely on second factor authentication in user model (instead of auth controller)
  • Disabled unnecessary passport routes
  • Fixed password-reset and signup to use the plain-text password for oauth

TODO: add 2fa to password-reset

src/database/migrations/2016_06_01_000001_create_oauth_auth_codes_table.php
35 ↗(On Diff #6991)

I don't think it's strictly necessary, but seems reasonable.

src/database/migrations/2021_04_28_090011_create_oauth_tables.php
59

It's a reference to the other table, but it IMO doesn't require the foreign key constraint (which afaik forces the creation of an index).

I don't think we frequently access this link, and the lifetime of the token is not bound to the lifetime of the client.

74

Could make sense, but I'm not sure it's necessary either as all tokens are regularly cleaned up individually (So the ON DELETE CASCADE doesn't seem required).

src/tests/Feature/Controller/AuthTest.php
15

I figured we can do that as soon as we need it someplace.

Got most tests to pass

We now report the reason for authentication failure via
User::findAndAuthenticate => OAuth exception => oauth request json response => AuthController

  1. See inline comments

Done

  1. When logging into the user UI I see this in the log:
[2021-05-04 09:27:08] local.DEBUG: [SQL] select * from `oauth_clients` where `id` = ? limit 1 [array (
  0 => '1',
)]
[2021-05-04 09:27:08] local.DEBUG: [SQL] select * from `oauth_clients` where `id` = ? limit 1 [array (
  0 => '1',
)]

I.e. a duplicated query. Would be good to investigate why it is done twice.

Not sure, but seems to be somewhere in the passport internals.

One is from issuing the token, the other could be the token guard, but I don't know without further investigation.

  1. Token refresh does not work. A scenario:
    • set token_expiry_minutes = 1 minute,
    • log in to the UI, and wait a minute,
    • you'll see that the refresh request returns 401 Unauthorized.
    • in log: The resource owner or authorization server denied the request. {"exception":"[object] (League\\OAuth2\\Server\\Exception\\OAuthServerException(code: 9): The resource owner or authorization server denied the request. at /home/alec/repos/kolab/src/vendor/league/oauth2-server/src/Exception/OAuthServerException.php:243).
    • I guess you just have to add refresh_token to the request in line 166 of app.js.

Fixed.

  1. When runing Feature suite tests I got 3 errors:
1) Tests\Feature\Controller\PasswordResetTest::testPasswordResetValidInput
ErrorException: Undefined property: stdClass::$access_token
2) Tests\Feature\Controller\SignupTest::testSignupValidInput
ErrorException: Undefined property: stdClass::$access_token
3) Tests\Feature\Controller\SignupTest::testSignupGroupAccount
ErrorException: Undefined property: stdClass::$access_token

Fixed.

  1. Look like a few browser test failures are also related to Undefined property: stdClass::$access_token.

I tried fixing what I found, but I seem to have a problem with redirects in my local setup.

  1. We should think how deploy encryption keys across multiple hosts.

True, but that seems like a container thing for jeroen/liutauras.

  1. Could https://github.com/laravel/passport/issues/379 be a problem for us in any way considering our future use of Passport?

This is indeed largely the issue we discussed before that you can't access e.g. the /authorize url without being authenticated already (I suppose session-less is the stateless part?),
but I think we'll be able to manage what we need to.

There was 1 error:

1) Tests\Feature\Controller\AuthTest::testInfo
ErrorException: Undefined index: refresh_token

/home/alec/repos/kolab/src/tests/Feature/Controller/AuthTest.php:87

--

There were 6 failures:

1) Tests\Feature\Controller\AuthTest::testLogin
Expected status code 200 but received 401.
Failed asserting that false is true.

/home/alec/repos/kolab/src/vendor/laravel/framework/src/Illuminate/Foundation/Testing/TestResponse.php:186
/home/alec/repos/kolab/src/tests/Feature/Controller/AuthTest.php:131

2) Tests\Feature\Controller\AuthTest::testRefresh
Expected status code 200 but received 401.
Failed asserting that false is true.

/home/alec/repos/kolab/src/vendor/laravel/framework/src/Illuminate/Foundation/Testing/TestResponse.php:186
/home/alec/repos/kolab/src/tests/Feature/Controller/AuthTest.php:206

3) Tests\Feature\Controller\PasswordResetTest::testPasswordResetValidInput
Expected status code 200 but received 401.
Failed asserting that false is true.

/home/alec/repos/kolab/src/vendor/laravel/framework/src/Illuminate/Foundation/Testing/TestResponse.php:186
/home/alec/repos/kolab/src/tests/Feature/Controller/PasswordResetTest.php:306

4) Tests\Feature\Controller\SignupTest::testSignupValidInput
Expected status code 200 but received 401.
Failed asserting that false is true.

/home/alec/repos/kolab/src/vendor/laravel/framework/src/Illuminate/Foundation/Testing/TestResponse.php:186
/home/alec/repos/kolab/src/tests/Feature/Controller/SignupTest.php:481

5) Tests\Feature\Controller\SignupTest::testSignupGroupAccount
Expected status code 200 but received 401.
Failed asserting that false is true.

/home/alec/repos/kolab/src/vendor/laravel/framework/src/Illuminate/Foundation/Testing/TestResponse.php:186
/home/alec/repos/kolab/src/tests/Feature/Controller/SignupTest.php:595
src/app/Auth/SecondFactor.php
37

$e is not needed here.

src/app/Http/Controllers/API/AuthController.php
38

Descriptions should also be aligned with each other.

159

You have to heck if $data is not empty here. I got: ErrorException: Trying to get property 'error' of non-object in /home/alec/repos/kolab/src/app/Http/Controllers/API/AuthController.php:156 when running tests.

src/app/User.php
808

I think we can remove this error logging here and below.

809

Is this user facing error message? Should we localize it?

This revision now requires changes to proceed.May 19 2021, 10:39 AM

I can't reproduce the test failures on my workstation. I'll try running the test on a separate system, to see if I can reproduce there.

src/app/Http/Controllers/API/AuthController.php
98

Then the authenticated call shouldn't have been possible.

159

I can't reproduce this error, but I'm now checking if error isset (so we just run into the generic error case otherwise).

src/app/User.php
809

Only in the case of secondfactor error, which is already localized.

I don't think we should be localizing error messages in general (that should be part of the UI layer), so I wouldn't localize these for the time being.

mollekopf marked 2 inline comments as done.

Addressed comments

Included the sql logging patch

machniak requested changes to this revision.EditedMay 20 2021, 12:44 PM

phpstan errors:

Line   app/Http/Controllers/API/AuthController.php                  
------ ------------------------------------------------------------- 
 157    Access to an undefined property object::$error_description.  
------ ------------------------------------------------------------- 

------ ------------------------------------------------------------------------------- 
 Line   app/User.php                                                                   
------ ------------------------------------------------------------------------------- 
 758    PHPDoc tag @throws with type OAuthServerException is not subtype of Throwable  
------ -------------------------------------------------------------------------------

Now, if I run vendor/bin/phpunit tests/Browser/Admin/UserTest alone, all is green. However, if I run vendor/bin/phpunit --testsuite=Browser --exclude-group=openvidu,stripe,mollie I got:

1) Tests\Browser\Admin\UserTest::testExternalEmail
Facebook\WebDriver\Exception\TimeoutException: Waited 5 seconds for location [/user/4181228433].

/home/alec/repos/kolab/src/vendor/laravel/dusk/src/Concerns/WaitsForElements.php:312
/home/alec/repos/kolab/src/vendor/laravel/dusk/src/Concerns/WaitsForElements.php:201
/home/alec/repos/kolab/src/vendor/laravel/dusk/src/Concerns/WaitsForElements.php:163
/home/alec/repos/kolab/src/tests/Browser/Pages/Admin/User.php:40
/home/alec/repos/kolab/src/vendor/laravel/dusk/src/Browser.php:199
/home/alec/repos/kolab/src/vendor/laravel/dusk/src/Browser.php:159
/home/alec/repos/kolab/src/tests/Browser/Admin/UserTest.php:399
/home/alec/repos/kolab/src/vendor/laravel/dusk/src/Concerns/ProvidesBrowser.php:68
/home/alec/repos/kolab/src/tests/Browser/Admin/UserTest.php:441

2) Tests\Browser\Admin\UserTest::testSuspendAndUnsuspend
Facebook\WebDriver\Exception\TimeoutException: Waited 5 seconds for location [/user/4181228433].

/home/alec/repos/kolab/src/vendor/laravel/dusk/src/Concerns/WaitsForElements.php:312
/home/alec/repos/kolab/src/vendor/laravel/dusk/src/Concerns/WaitsForElements.php:201
/home/alec/repos/kolab/src/vendor/laravel/dusk/src/Concerns/WaitsForElements.php:163
/home/alec/repos/kolab/src/tests/Browser/Pages/Admin/User.php:40
/home/alec/repos/kolab/src/vendor/laravel/dusk/src/Browser.php:199
/home/alec/repos/kolab/src/vendor/laravel/dusk/src/Browser.php:159
/home/alec/repos/kolab/src/tests/Browser/Admin/UserTest.php:452
/home/alec/repos/kolab/src/vendor/laravel/dusk/src/Concerns/ProvidesBrowser.php:68
/home/alec/repos/kolab/src/tests/Browser/Admin/UserTest.php:464

3) Tests\Browser\Admin\UserTest::testReset2FA
Facebook\WebDriver\Exception\TimeoutException: Waited 5 seconds for location [/user/1027597698].

/home/alec/repos/kolab/src/vendor/laravel/dusk/src/Concerns/WaitsForElements.php:312
/home/alec/repos/kolab/src/vendor/laravel/dusk/src/Concerns/WaitsForElements.php:201
/home/alec/repos/kolab/src/vendor/laravel/dusk/src/Concerns/WaitsForElements.php:163
/home/alec/repos/kolab/src/tests/Browser/Pages/Admin/User.php:40
/home/alec/repos/kolab/src/vendor/laravel/dusk/src/Browser.php:199
/home/alec/repos/kolab/src/vendor/laravel/dusk/src/Browser.php:159
/home/alec/repos/kolab/src/tests/Browser/Admin/UserTest.php:479
/home/alec/repos/kolab/src/vendor/laravel/dusk/src/Concerns/ProvidesBrowser.php:68
/home/alec/repos/kolab/src/tests/Browser/Admin/UserTest.php:496

Finally, the log is spammed with OAuthServerException errors, it would be good to get rid of these. A user providing invalid password is not something that should put ~80 linies into the log.

src/app/Providers/AppServiceProvider.php
37

I don't the indentation above. And I would add a new line before return.

src/config/auth.php
121

The old default was 1 hour. 24 hours is too much imo.

This revision now requires changes to proceed.May 20 2021, 12:44 PM

I did another run of tests and got the same result as above. I.e. I run vendor/bin/phpunit tests/Browser/Admin twice on master and all is green. Then I run the same on this diff and I got three tests failing. Looks like the problem is with refreshing the token. I see this in console dump:

https:\/\/admin.alec.dev.kolab.io\/api\/auth\/info?refresh=1 - Failed to load resource: the server responded with a status of 401 ()

I see C: POST https://admin.alec.dev.kolab.io/api/auth/info?refresh=1 [5M]: 0.0055 sec in the log and this is strange because it's too fast and there are no sql queries for this request logged.

Normal refresh requests look like this:

[2021-05-21 08:04:25] local.DEBUG: [SQL] select * from `oauth_clients` where `id` = ? limit 1 [1]  
[2021-05-21 08:04:25] local.DEBUG: [SQL] select * from `oauth_clients` where `id` = ? limit 1 [1]  
[2021-05-21 08:04:25] local.DEBUG: [SQL] select * from `oauth_refresh_tokens` where `id` = ? limit 1 [674abe63f4a135f153a7e9a1bfabc23da8c6aa269e3f6d2981d7fbf4822dba33820f14251d47c01b]  
[2021-05-21 08:04:25] local.DEBUG: [SQL] update `oauth_access_tokens` set `revoked` = ? where `id` = ? [1, df314bf90fc52c4f25f12bd6cabd7f50142448e0310efc14dcd6aee7eda5e847cb7d2ab4a9171c0d]  
[2021-05-21 08:04:25] local.DEBUG: [SQL] update `oauth_refresh_tokens` set `revoked` = ? where `id` = ? [1, 674abe63f4a135f153a7e9a1bfabc23da8c6aa269e3f6d2981d7fbf4822dba33820f14251d47c01b]  
[2021-05-21 08:04:25] local.DEBUG: [SQL] insert into `oauth_access_tokens` (`id`, `user_id`, `client_id`, `scopes`, `revoked`, `created_at`, `updated_at`, `expires_at`) values (?, ?, ?, ?, ?, ?, ?, ?) [de4a733ac1d60636df06fc6e...
[2021-05-21 08:04:25] local.DEBUG: [SQL] insert into `oauth_refresh_tokens` (`id`, `access_token_id`, `revoked`, `expires_at`) values (?, ?, ?, ?) [5289a1a999a40c3af3554077a78a772b9903dc2e94394e86bb416ebbc78560a4dfcd8cc02f3622...
[2021-05-21 08:04:25] local.DEBUG: C: POST https://alec.dev.kolab.io/api/auth/info?refresh=1 [5M]: 0.6060 sec.

And these and /auth/login requests are much slower than all others. Maybe because of encryption involved? 0.6 sec. is a lot, and is 10x more than on master.

@alec; suggested;

# yum install haveged
# systemctl start haveged
  • Lowered timeout
  • Removed throttling on token route
  • I also turned the expires_in comparison into a fuzzy comparison. Because of passport internals it's possible that some time has already passed and the expires_in response is off by a second.

From phpunit tests/Browser/Admin I now have only testUserInfo failing, because the user is somehow not imapReady (no idea why), but that seems unrelated.

I could reproduce some other failures before, which was caused by rate-limiting on the oauth/token route, these pass now.

I have not investigate why refreshing tokens takes as long as it does, but there are a couple of overheads compared to the previous JWT solution:

  • We're proxying the request from the auth controller to /oauth/token
  • Refreshing a token revokes and re-issues the refresh token as well (which isn't required by oauth, but that's what passport does)
  • We're now doing multiple database operations to issue a token. The JWT tokens are stateless AFAIR (so no db involved).

I continue to have some failures in other tests (seemingly unrelated), full log below.

There were 5 errors:

1) Tests\Feature\Controller\UsersTest::testStore
Illuminate\Http\Exceptions\ThrottleRequestsException: Too Many Attempts.

/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Routing/Middleware/ThrottleRequests.php:125
/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Routing/Middleware/ThrottleRequests.php:54
/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:171
/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:105
/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Routing/Router.php:683
/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Routing/Router.php:658
/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Routing/Router.php:624
/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Routing/Router.php:613
/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Foundation/Http/Kernel.php:170
/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:130
/home/mollekopf/src/kolab/src/vendor/swooletw/laravel-swoole/src/Middleware/AccessLog.php:42
/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:171
/home/mollekopf/src/kolab/src/app/Http/Middleware/DevelConfig.php:36
/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:171
/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Foundation/Http/Middleware/TransformsRequest.php:21
/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:171
/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Foundation/Http/Middleware/TransformsRequest.php:21
/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:171
/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Foundation/Http/Middleware/ValidatePostSize.php:27
/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:171
/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Foundation/Http/Middleware/CheckForMaintenanceMode.php:63
/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:171
/home/mollekopf/src/kolab/src/vendor/fideloper/proxy/src/TrustProxies.php:57
/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:171
/home/mollekopf/src/kolab/src/app/Http/Middleware/RequestLogger.php:17
/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:171
/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:105
/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Foundation/Http/Kernel.php:145
/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Foundation/Http/Kernel.php:110
/home/mollekopf/src/kolab/src/app/Http/Kernel.php:99
/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Foundation/Testing/Concerns/MakesHttpRequests.php:470
/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Foundation/Testing/Concerns/MakesHttpRequests.php:288
/home/mollekopf/src/kolab/src/tests/Feature/Controller/UsersTest.php:593

2) Tests\Browser\Admin\UserTest::testUserInfo
Facebook\WebDriver\Exception\NoSuchElementException: no such element: Unable to locate element: {"method":"css selector","selector":"body #user-info form .row:nth-child(3) #status span.text-success"}
  (Session info: headless chrome=90.0.4430.93)

/home/mollekopf/src/kolab/src/vendor/php-webdriver/webdriver/lib/Exception/WebDriverException.php:117
/home/mollekopf/src/kolab/src/vendor/php-webdriver/webdriver/lib/Remote/HttpCommandExecutor.php:371
/home/mollekopf/src/kolab/src/vendor/php-webdriver/webdriver/lib/Remote/RemoteWebDriver.php:591
/home/mollekopf/src/kolab/src/vendor/php-webdriver/webdriver/lib/Remote/RemoteWebDriver.php:212
/home/mollekopf/src/kolab/src/vendor/laravel/dusk/src/ElementResolver.php:373
/home/mollekopf/src/kolab/src/vendor/laravel/dusk/src/Concerns/MakesAssertions.php:169
/home/mollekopf/src/kolab/src/tests/Browser/Admin/UserTest.php:102
/home/mollekopf/src/kolab/src/vendor/laravel/dusk/src/Browser.php:451
/home/mollekopf/src/kolab/src/tests/Browser/Admin/UserTest.php:111
/home/mollekopf/src/kolab/src/vendor/laravel/dusk/src/Concerns/ProvidesBrowser.php:68
/home/mollekopf/src/kolab/src/tests/Browser/Admin/UserTest.php:159

3) Tests\Browser\PaymentMollieTest::testAutoPaymentSetup
Facebook\WebDriver\Exception\TimeoutException: Waited 5 seconds for selector [#mandate-info].

/home/mollekopf/src/kolab/src/vendor/laravel/dusk/src/Concerns/WaitsForElements.php:291
/home/mollekopf/src/kolab/src/vendor/laravel/dusk/src/Concerns/WaitsForElements.php:44
/home/mollekopf/src/kolab/src/tests/Browser/PaymentMollieTest.php:176
/home/mollekopf/src/kolab/src/vendor/laravel/dusk/src/Concerns/ProvidesBrowser.php:68
/home/mollekopf/src/kolab/src/tests/Browser/PaymentMollieTest.php:190

4) Tests\Browser\PaymentStripeTest::testAutoPaymentSetup
Facebook\WebDriver\Exception\TimeoutException: Waited 5 seconds for selector [#mandate-info].

/home/mollekopf/src/kolab/src/vendor/laravel/dusk/src/Concerns/WaitsForElements.php:291
/home/mollekopf/src/kolab/src/vendor/laravel/dusk/src/Concerns/WaitsForElements.php:44
/home/mollekopf/src/kolab/src/tests/Browser/PaymentStripeTest.php:164
/home/mollekopf/src/kolab/src/vendor/laravel/dusk/src/Concerns/ProvidesBrowser.php:68
/home/mollekopf/src/kolab/src/tests/Browser/PaymentStripeTest.php:176

5) Tests\Browser\StatusTest::testUserStatus
Facebook\WebDriver\Exception\NoSuchElementException: no such element: Unable to locate element: {"method":"css selector","selector":"body #user-list table tbody tr:first-child td:first-child svg.fa-user.text-success"}
  (Session info: headless chrome=90.0.4430.93)

/home/mollekopf/src/kolab/src/vendor/php-webdriver/webdriver/lib/Exception/WebDriverException.php:117
/home/mollekopf/src/kolab/src/vendor/php-webdriver/webdriver/lib/Remote/HttpCommandExecutor.php:371
/home/mollekopf/src/kolab/src/vendor/php-webdriver/webdriver/lib/Remote/RemoteWebDriver.php:591
/home/mollekopf/src/kolab/src/vendor/php-webdriver/webdriver/lib/Remote/RemoteWebDriver.php:212
/home/mollekopf/src/kolab/src/vendor/laravel/dusk/src/ElementResolver.php:373
/home/mollekopf/src/kolab/src/vendor/laravel/dusk/src/Concerns/MakesAssertions.php:615
/home/mollekopf/src/kolab/src/tests/Browser/StatusTest.php:247
/home/mollekopf/src/kolab/src/vendor/laravel/dusk/src/Concerns/ProvidesBrowser.php:68
/home/mollekopf/src/kolab/src/tests/Browser/StatusTest.php:287

--

There were 9 failures:

1) Tests\Functional\HorizonTest::testRegularAccess
Expected status code 404 but received 200.
Failed asserting that false is true.

/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Foundation/Testing/TestResponse.php:186
/home/mollekopf/src/kolab/src/tests/Functional/HorizonTest.php:22

2) Tests\Feature\Controller\WalletsTest::testReceiptDownload
Expected status code 403 but received 429.
Failed asserting that false is true.

/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Foundation/Testing/TestResponse.php:186
/home/mollekopf/src/kolab/src/tests/Feature/Controller/WalletsTest.php:112

3) Tests\Feature\Controller\WalletsTest::testReceipts
Expected status code 403 but received 429.
Failed asserting that false is true.

/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Foundation/Testing/TestResponse.php:186
/home/mollekopf/src/kolab/src/tests/Feature/Controller/WalletsTest.php:156

4) Tests\Feature\Controller\WalletsTest::testShow
Expected status code 200 but received 429.
Failed asserting that false is true.

/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Foundation/Testing/TestResponse.php:186
/home/mollekopf/src/kolab/src/tests/Feature/Controller/WalletsTest.php:221

5) Tests\Feature\Controller\WalletsTest::testTransactions
Expected status code 403 but received 429.
Failed asserting that false is true.

/home/mollekopf/src/kolab/src/vendor/laravel/framework/src/Illuminate/Foundation/Testing/TestResponse.php:186
/home/mollekopf/src/kolab/src/tests/Feature/Controller/WalletsTest.php:247

6) Tests\Feature\Documents\ReceiptTest::testPdfOutput
Failed asserting that false is true.

/home/mollekopf/src/kolab/src/tests/Feature/Documents/ReceiptTest.php:191

7) Tests\Browser\Meet\RoomControlsTest::testNicknameAndMuting
Saw unexpected element [body div.meet-video.self .status .status-audio].
Failed asserting that false is true.

/home/mollekopf/src/kolab/src/vendor/laravel/dusk/src/Concerns/MakesAssertions.php:656
/home/mollekopf/src/kolab/src/tests/Browser/Meet/RoomControlsTest.php:179
/home/mollekopf/src/kolab/src/vendor/laravel/dusk/src/Concerns/ProvidesBrowser.php:68
/home/mollekopf/src/kolab/src/tests/Browser/Meet/RoomControlsTest.php:224

8) Tests\Browser\Meet\RoomControlsTest::testChat
Count of [@chat-list .message div] elements is not 4
Failed asserting that 2 matches expected 4.

/home/mollekopf/src/kolab/src/tests/Browser.php:44
/home/mollekopf/src/kolab/src/tests/Browser/Meet/RoomControlsTest.php:288
/home/mollekopf/src/kolab/src/vendor/laravel/dusk/src/Concerns/ProvidesBrowser.php:68
/home/mollekopf/src/kolab/src/tests/Browser/Meet/RoomControlsTest.php:319

9) Tests\Browser\Meet\RoomQATest::testQA
Did not see expected text [John] within element [body #meet-queue .dropdown].
Failed asserting that false is true.

/home/mollekopf/src/kolab/src/vendor/laravel/dusk/src/Concerns/MakesAssertions.php:173
/home/mollekopf/src/kolab/src/tests/Browser/Meet/RoomQATest.php:92
/home/mollekopf/src/kolab/src/vendor/laravel/dusk/src/Concerns/ProvidesBrowser.php:68
/home/mollekopf/src/kolab/src/tests/Browser/Meet/RoomQATest.php:137

There clearly seems to be room for improvement from an overall request execution time of 0.3450s, with < 0.1s used by the sql queries.

[2021-05-26 15:21:56] testing.DEBUG: [SQL] select * from `oauth_clients` where `id` = ? limit 1 [1]: 0.0003 sec.
[2021-05-26 15:21:56] testing.DEBUG: [SQL] select * from `oauth_clients` where `id` = ? limit 1 [1]: 0.0003 sec.
[2021-05-26 15:21:57] testing.DEBUG: [SQL] select * from `oauth_refresh_tokens` where `id` = ? limit 1 [f3d79645d4a96494e6e7b0aeed949e592b1789cd033f42fe1ea29535e646b3d7e1c78c2ce17811ac]: 0.0006 sec.
[2021-05-26 15:21:57] testing.DEBUG: [SQL] update `oauth_access_tokens` set `revoked` = ? where `id` = ? [1, 836e5ce1a4f8b4088430c03747ec432fc65a3aabb503e68f5e6dff8523bd3894842ba9c81a267ab6]: 0.0088 sec.
[2021-05-26 15:21:57] testing.DEBUG: [SQL] update `oauth_refresh_tokens` set `revoked` = ? where `id` = ? [1, f3d79645d4a96494e6e7b0aeed949e592b1789cd033f42fe1ea29535e646b3d7e1c78c2ce17811ac]: 0.0108 sec.
[2021-05-26 15:21:57] testing.DEBUG: [SQL] insert into `oauth_access_tokens` (`id`, `user_id`, `client_id`, `scopes`, `revoked`, `created_at`, `updated_at`, `expires_at`) values (?, ?, ?, ?, ?, ?, ?, ?) [99bf7f410caee6075dce1cf9e8bac4be0da00b3d12dfc37a39448093899ba1faaba1bada6d91ab51, 667792948, 1, [], , 2021-05-26 03:21:57, 2021-05-26 03:21:57, 2021-05-26 16:21:56]: 0.0109 sec.
[2021-05-26 15:21:57] testing.DEBUG: [SQL] insert into `oauth_refresh_tokens` (`id`, `access_token_id`, `revoked`, `expires_at`) values (?, ?, ?, ?) [614c8bb2a2175c677266f1909dc96def3768e47db7dd796177a501972ebb00bb32db71887e44933f, 99bf7f410caee6075dce1cf9e8bac4be0da00b3d12dfc37a39448093899ba1faaba1bada6d91ab51, , 2021-06-25 15:21:56]: 0.0105 sec.
[2021-05-26 15:21:57] testing.DEBUG: C: POST http://localhost/api/auth/info?refresh=1 [11.5M]: 0.3450 sec.
  • The bulk of the request is spent in League\OAuth2\Server\AuthorizationServer::respondToAccessTokenRequest: 0.4621s out of 0.4741s total
    • League\OAuth2\Server\Grant\RefreshTokenGrant::respondToAccessTokenRequest: takes up ~300ms out of that
    • League\OAuth2\Server\ResponseTypes\BearerTokenResponse::generateHttpResponse: the remaining ~150ms

One culprit is certainly Crypto::decryptWithPassword, which takes ~150ms.

Perhaps we have another encrypt call someplace, which takes a similar amount of time?

We're not the first to encounter the crypto slowness:

I think we could customize the PassportServiceProvider to make use of the faster encryption version (we just can't switch back and forth with existing tokens).

  • phpstan
  • Addressed slow token issuing by customizing the PassportServiceProvider

To sum up the current state:

  1. All tests pass!
  2. Tokens refresh request is fast now, but /auth/login is still 0.3-0.4 sec. I didn't investigate how much of that is passport.
  3. We should probably add some code to Kernel as in https://laravel.com/docs/6.x/passport#purging-tokens
  4. @vanmeeuwen, should take a look at this regarding deployment of the oauth keys and client secret (and ./artisan passport:keys --force) - we need the same keys on every Kolab4 host.

To sum up the current state:

  1. All tests pass!
  2. Tokens refresh request is fast now, but /auth/login is still 0.3-0.4 sec. I didn't investigate how much of that is passport.

250 out of 300ms are spend in Hash::check in User::findAndValidateForPassport, so that is by design I think.

  1. We should probably add some code to Kernel as in https://laravel.com/docs/6.x/passport#purging-tokens

Either that or alternatively it an be done using an artisan command run by a cronjob or so.

  1. @vanmeeuwen, should take a look at this regarding deployment of the oauth keys and client secret (and ./artisan passport:keys --force) - we need the same keys on every Kolab4 host.

Yes

Now taking the ldap password into account

Added a test for the password validation

We still need to take care of:

Just checking if this diff is up to date

Just checking if this diff is up to date

The diff now shows the changes from the rebase to master, so dev/imapproxy now contains the correct changes.

We should probably add some code to Kernel as in https://laravel.com/docs/6.x/passport#purging-tokens

We are going to run this via openshift, just like we do for the rest.

The command to purge both revoke and expired keys is (could be separated):

artisan passport:purge

@vanmeeuwen, should take a look at this regarding deployment of the oauth keys and client secret (and ./artisan passport:keys --force) - we need the same keys on every Kolab4 host.

The keys generated by `artisan passport:keys --force```are located in files in storage/oauth-private.key storage/oauth-public.key

Include writing out the passport keys in the assemble script

src/.env.example
149

These are the values obtained from something like this?

$ ./artisan passport:client --password --name="Kolab Now Companion" --provider=users
Password grant client created successfully.
Client ID: 942cc5d9-5e93-4c4d-8405-f15d012eed05
Client secret: q6XLiyod6HIb6ct40Mbb7CCpTAHlONWKkLSd7qWb

I'm confused with the inclusion of \config('passport.private_key') in a config/passport.php I have (after dicking around to see if I could entertain UUIDs rather than auto-increment, of the following;

PASSPORT_PRIVATE_KEY
PASSPORT_PUBLIC_KEY
src/.s2i/bin/assemble
47 ↗(On Diff #7759)

Same here with regards to the PASSPORT_PRIVATE/PUBLIC_KEY.

machniak added inline comments.
src/app/Http/Controllers/API/AuthController.php
160

It should be $data->error_description, also are we sure it's always set?

This revision now requires changes to proceed.Aug 19 2021, 12:46 PM

Use error_description directly.

This revision is now accepted and ready to land.Aug 20 2021, 10:55 AM
This revision was automatically updated to reflect the committed changes.