Page MenuHomekolab.org

Laravel Passport support
Needs ReviewPublic

Authored by mollekopf on Apr 22 2021, 12:13 PM.

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
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 35641
Build 13993: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Configurable token expiry, minor test adjustments

mollekopf added inline comments.Apr 27 2021, 10:39 AM
src/app/Http/Controllers/API/AuthController.php
95–98

This is an authenticated api call, so that doesn't seems like something we can run into, no?

src/database/migrations/2016_06_01_000001_create_oauth_auth_codes_table.php
60 ↗(On Diff #6958)

I don't have one either, so doesn't seem to be required.

src/tests/Feature/Controller/AuthTest.php
89–90

Nope

machniak requested changes to this revision.Apr 27 2021, 10:55 AM
machniak added inline comments.
src/database/migrations/2016_06_01_000001_create_oauth_auth_codes_table.php
35 ↗(On Diff #6991)

Shouldn't the user_id column be a foreign key with on delete cascade on update cascade? Here and in other tables.

This revision now requires changes to proceed.Apr 27 2021, 10:55 AM
mollekopf updated this revision to Diff 7012.Apr 28 2021, 11:13 AM
mollekopf marked an inline comment as done.

Adjusted migrations (merged, foreign key constraint on user id pk, matching types)

mollekopf updated this revision to Diff 7018.Apr 28 2021, 11:21 AM
mollekopf edited the summary of this revision. (Show Details)

Correctly ignore the default migrations

machniak requested changes to this revision.EditedMay 4 2021, 11:50 AM
  1. See inline comments
  2. 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.

  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.
  2. 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
  1. Look like a few browser test failures are also related to Undefined property: stdClass::$access_token.
  2. We should think how deploy encryption keys across multiple hosts.
  3. Could https://github.com/laravel/passport/issues/379 be a problem for us in any way considering our future use of Passport?
src/app/Http/Controllers/API/AuthController.php
94

I think we should check if user exists before the proxy request. And we should return 401 response not throw an exception.

135

Some documentation to this method would be nice.

src/config/auth.php
122

Please, make these configurable via .env file.

src/database/migrations/2021_04_28_090011_create_oauth_tables.php
59

Shouldn't client_id be a foreign key? Here and above.

74

Shouldn't access_token_id be a foreign key?

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

This method might be useful in other places. How about moving it to TestCase or TestCaseTrait?

This revision now requires changes to proceed.May 4 2021, 11:50 AM
mollekopf updated this revision to Diff 7072.May 12 2021, 9:23 AM
mollekopf marked 3 inline comments as done.
  • 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

mollekopf added inline comments.May 12 2021, 9:23 AM
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.

mollekopf updated this revision to Diff 7084.May 12 2021, 6:41 PM

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: `
  2. Tests\Feature\Controller\PasswordResetTest::testPasswordResetValidInput ErrorException: Undefined property: stdClass::$access_token
  3. Tests\Feature\Controller\SignupTest::testSignupValidInput ErrorException: Undefined property: stdClass::$access_token
  4. 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.

machniak requested changes to this revision.May 19 2021, 10:39 AM
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
40

$e is not needed here.

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

Descriptions should also be aligned with each other.

156

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
767

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

768

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

This revision now requires changes to proceed.May 19 2021, 10:39 AM
mollekopf marked 9 inline comments as done.May 19 2021, 3:13 PM

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
95–98

Then the authenticated call shouldn't have been possible.

156

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
768

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 updated this revision to Diff 7108.May 19 2021, 3:14 PM
mollekopf marked 2 inline comments as done.

Addressed comments

mollekopf updated this revision to Diff 7120.May 20 2021, 11:46 AM

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
mollekopf updated this revision to Diff 7126.May 20 2021, 2:01 PM

Ignore exceptions

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
mollekopf updated this revision to Diff 7174.May 26 2021, 1:33 PM
  • 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).

mollekopf updated this revision to Diff 7192.May 26 2021, 7:22 PM
  • 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

mollekopf updated this revision to Diff 7240.Jun 1 2021, 4:21 PM

Now taking the ldap password into account

mollekopf updated this revision to Diff 7252.Jun 2 2021, 10:16 AM

Added a test for the password validation

We still need to take care of:

mollekopf updated this revision to Diff 7546.Fri, Jul 16, 9:43 AM

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