Project Tags

None

Referenced Files

	F117762286: D2494.1775216191.diff
	Thu, Apr 2, 11:36 AM

	F117760694: D2494.1775213290.diff
	Thu, Apr 2, 10:48 AM

	F117756973: D2494.1775206706.diff
	Thu, Apr 2, 8:58 AM

	F117754970: D2494.1775201564.diff
	Thu, Apr 2, 7:32 AM

	F117754969: D2494.1775201563.diff
	Thu, Apr 2, 7:32 AM

	F117754880: D2494.1775201200.diff
	Thu, Apr 2, 7:26 AM

	F117754878: D2494.1775201186.diff
	Thu, Apr 2, 7:26 AM

	F117753781: D2494.1775196472.diff
	Thu, Apr 2, 6:07 AM

Subscribers

alec

vanmeeuwen

Details

Reviewers

machniak
vanmeeuwen

Group Reviewers

Restricted Project

Commits

rK1eac065869c2: Laravel Passport support
rK761b273937d9: Laravel Passport support
rK2b7a74400512: Laravel Passport support
rK6b017cad0c7e: Laravel Passport support
rK226150e4d192: Laravel Passport support
rKe705808eb940: Laravel Passport support
rK9070a213b72e: Laravel Passport support
rK54a24077910c: Laravel Passport support
rKfc74f7e86eff: Laravel Passport support
rKaae8725448c7: Laravel Passport support
rK955cf3215c1c: Laravel Passport support
rKedbf36211e18: Laravel Passport support

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 Lint Coverage

Unit

No Test Coverage

Build Status

Buildable 34150
Build 13606: arc lint + arc unit

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

Harbormaster completed remote builds in B34138: Diff 7072.May 12 2021, 7:23 AM

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

Got most tests to pass

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

Harbormaster completed remote builds in B34150: Diff 7084.May 12 2021, 4:41 PM

In D2494#29305, @machniak wrote:

See inline comments

Done

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.

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.

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.

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.

We should think how deploy encryption keys across multiple hosts.

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

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
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
730	I think we can remove this error logging here and below.
731	Is this user facing error message? Should we localize it?

This revision now requires changes to proceed.May 19 2021, 8: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
95–101	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
731	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.

Addressed comments

Included the sql logging patch

Harbormaster completed remote builds in B34210: Diff 7120.May 20 2021, 9:46 AM

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
36	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, 10:44 AM

Ignore exceptions

Harbormaster completed remote builds in B34216: Diff 7126.May 20 2021, 12:01 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

Harbormaster completed remote builds in B34270: Diff 7174.May 26 2021, 11:33 AM

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

Harbormaster completed remote builds in B34288: Diff 7192.May 26 2021, 5:22 PM

To sum up the current state:

All tests pass!
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.
We should probably add some code to Kernel as in https://laravel.com/docs/6.x/passport#purging-tokens
@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.

In D2494#30205, @machniak wrote:

To sum up the current state:

All tests pass!

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.

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.

@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

Harbormaster completed remote builds in B34342: Diff 7240.Jun 1 2021, 2:21 PM

phpstan

Harbormaster completed remote builds in B34348: Diff 7246.Jun 1 2021, 3:20 PM

Added a test for the password validation

Harbormaster completed remote builds in B34354: Diff 7252.Jun 2 2021, 8:16 AM

We still need to take care of:

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

@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.

Just checking if this diff is up to date

Harbormaster completed remote builds in B35641: Diff 7546.Jul 16 2021, 7:43 AM

In D2494#31492, @mollekopf wrote:

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

Harbormaster completed remote builds in B36070: Diff 7759.Aug 16 2021, 1:39 PM

mollekopf mentioned this in D2710: Seed oauth public/private keys from env variable in assemble script.Aug 16 2021, 1:44 PM

vanmeeuwen added inline comments.Aug 17 2021, 12:44 PM

src/.env.example
132	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.

Moved to uuids for clients

Harbormaster completed remote builds in B36160: Diff 7849.Aug 18 2021, 8:25 AM

Removed assemble changes

Harbormaster completed remote builds in B36196: Diff 7888.Aug 18 2021, 10:28 AM

machniak requested changes to this revision.Aug 19 2021, 10:46 AM

machniak added inline comments.

src/app/Http/Controllers/API/AuthController.php
157	It should be `$data->error_description`, also are we sure it's always set?

This revision now requires changes to proceed.Aug 19 2021, 10:46 AM

Rebased on master

Harbormaster completed remote builds in B36244: Diff 7939.Aug 19 2021, 11:21 AM

Use error_description directly.

Harbormaster completed remote builds in B36253: Diff 7954.Aug 19 2021, 1:03 PM

Green light from me.

vanmeeuwen accepted this revision.Aug 20 2021, 8:55 AM

This revision is now accepted and ready to land.Aug 20 2021, 8:55 AM

Closed by commit rK1eac065869c2: Laravel Passport support. · Explain WhyAug 20 2021, 8:56 AM

This revision was automatically updated to reflect the committed changes.

Laravel Passport support
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 7084

bin/quickstart.sh

src/.env.example

src/app/Auth/SecondFactor.php

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

src/app/Http/Controllers/API/PasswordResetController.php

src/app/Http/Controllers/API/SignupController.php

src/app/Providers/AppServiceProvider.php

src/app/Providers/AuthServiceProvider.php

src/app/User.php

src/composer.json

src/config/app.php

src/config/auth.php

src/config/jwt.php

src/config/swoole_http.php

src/database/migrations/2021_04_28_090011_create_oauth_tables.php

src/database/seeds/DatabaseSeeder.php

src/database/seeds/local/OauthClientSeeder.php

src/resources/js/app.js

src/resources/vue/App.vue

src/resources/vue/PasswordReset.vue

src/routes/api.php

src/tests/Feature/Controller/AuthTest.php

Laravel Passport supportClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 7084

bin/quickstart.sh

src/.env.example

src/app/Auth/SecondFactor.php

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

src/app/Http/Controllers/API/PasswordResetController.php

src/app/Http/Controllers/API/SignupController.php

src/app/Providers/AppServiceProvider.php

src/app/Providers/AuthServiceProvider.php

src/app/User.php

src/composer.json

src/config/app.php

src/config/auth.php

src/config/jwt.php

src/config/swoole_http.php

src/database/migrations/2021_04_28_090011_create_oauth_tables.php

src/database/seeds/DatabaseSeeder.php

src/database/seeds/local/OauthClientSeeder.php

src/resources/js/app.js

src/resources/vue/App.vue

src/resources/vue/PasswordReset.vue

src/routes/api.php

src/tests/Feature/Controller/AuthTest.php

Laravel Passport support
ClosedPublic
Actions

Revision Contents
Changeset List