Page MenuHomekolab.org

Laravel Passport support
Needs ReviewPublic

Authored by mollekopf on Thu, Apr 22, 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 34150
Build 13606: arc lint + arc unit

Event Timeline

mollekopf requested review of this revision.Thu, Apr 22, 12:13 PM
mollekopf created this revision.
mollekopf added reviewers: Restricted Project, machniak.Thu, Apr 22, 12:16 PM

I think all tests that pass for me on master pass here, but not all tests pass for me. I'll investigate this further, but feel free to point out failing tests.

machniak requested changes to this revision.Fri, Apr 23, 9:02 AM

Just code review. I didn't look closer to this yet.

src/app/Http/Controllers/API/AuthController.php
95–101

What if the user or token does not exist here?

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

This suggests existence of config/passport.php file. It is not needed or you forgot to commit it?

src/tests/Feature/Controller/AuthTest.php
86–87

If actingAs() then do we need withHeaders()?

148

So, maybe make the value configurable.

This revision now requires changes to proceed.Fri, Apr 23, 9:02 AM
mollekopf updated this revision to Diff 6991.Tue, Apr 27, 10:39 AM
mollekopf marked 3 inline comments as done.

Configurable token expiry, minor test adjustments

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

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
86–87

Nope

machniak requested changes to this revision.Tue, Apr 27, 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.Tue, Apr 27, 10:55 AM
mollekopf updated this revision to Diff 7012.Wed, Apr 28, 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.Wed, Apr 28, 11:21 AM
mollekopf edited the summary of this revision. (Show Details)

Correctly ignore the default migrations

machniak requested changes to this revision.EditedTue, May 4, 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.

122

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.Tue, May 4, 11:50 AM
mollekopf updated this revision to Diff 7072.Wed, May 12, 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.Wed, May 12, 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.Wed, May 12, 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.