Page MenuHomekolab.org

NGINX Controller, 2fa for client connections and companion app support
ClosedPublic

Authored by mollekopf on Jul 16 2021, 10:12 AM.

Diff Detail

Repository
rK kolab
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 36073
Build 14167: arc lint + arc unit

Event Timeline

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

Update diff to latest state

mollekopf updated this revision to Diff 7762.Aug 16 2021, 3:39 PM
mollekopf marked 27 inline comments as done.

Addressed comments

machniak requested changes to this revision.Aug 17 2021, 8:46 AM

I miss tests for console commands. I get they are simple and CLI has the worse coverage, but still it shouldn't be that hard.

src/app/AuthAttempt.php
64

Wouldn't be better to have/use isAccepted() and isExpired() methods instead?

191

Actually I don't see why you would need a refresh here.

src/app/Console/Commands/AuthAttempt/PurgeCommand.php
33

\App\ prefix not needed here.

src/app/Http/Controllers/API/V4/AuthAttemptsController.php
23

We're not using findOrFail() method in http controllers, because the error is not that nice.

52

Should that set a specific $authAttempt->reason?

73

This debug message is not very helpful.

src/app/Http/Controllers/API/V4/CompanionAppsController.php
21

I prefer to use $this->guard(), one use clause less.

23

Here you can just use $this->errorResponse(401); And anyway, is that check needed. I think it's controlled by the middleware. BTW, you could test that.

33

There's a max too, isn't it?

56

I don't think we should return/display the other user ID to the requesting user. This should be just $this->errorResponse(401), I guess.

src/app/Http/Controllers/API/V4/NGINXController.php
100

if you set reason before deny(), save() will become redundant. I think it would make sense if we could just do $attempt->deny(\App\AuthAttempt::REASON_GEOLOCATION);.

src/tests/Feature/Controller/AuthAttemptsTest.php
33

This is not a controller test. Should be in src/tests/Feature/AuthAttemptTest.php, imo.

50

I used to comment controller tests with something like this:

/**
   * Test domain config update (POST /api/v4/domains/<domain>/config)
   */
73

All controller tests should include tests for all 403/404 conditions.

src/tests/Feature/Controller/CompanionAppsTest.php
49

A test for failing input validation and one for use of other user's device would be nice.

src/tests/Feature/Controller/NGINXTest.php
38

2fa_enabled = true in the seeder. So, change it here or there.

47

I miss tests for error conditions, i.e. all byebye() cases.

This revision now requires changes to proceed.Aug 17 2021, 8:46 AM
machniak added inline comments.Aug 17 2021, 9:38 AM
src/tests/Feature/Controller/AuthAttemptsTest.php
97

This can be removed.

99

This can be $this->assertCount(2, $json);

100

This assertion sometimes fail. It's because there's orderBy('updated_at', 'desc'), so I think the order of assertions needs to be reverted here.

machniak added inline comments.Aug 17 2021, 10:11 AM
src/app/AuthAttempt.php
152

Found by phpstan, should be $clientIP

vanmeeuwen requested changes to this revision.Aug 17 2021, 11:24 AM
vanmeeuwen added a subscriber: vanmeeuwen.
vanmeeuwen added inline comments.
src/routes/api.php
152

If it's all the same to you, I would have the nginx webhook fall under 'services' -- access to those is limited to internal networks.

src/tests/Feature/Controller/NGINXTest.php
58

Please use \App\Utils::generatePassphrase(), which in development and with APP_PASSPHRASE creates predictable passwords.

mollekopf added inline comments.Aug 17 2021, 2:32 PM
src/config/imap.php
9

I agree, but the alternative would be to get rid of uri I think

src/database/migrations/2021_03_25_144555_create_auth_attempts_table.php
20

The user_id index? You mean because of the [user_id, ip] index blow?

mollekopf updated this revision to Diff 7837.Aug 17 2021, 5:01 PM
mollekopf marked 17 inline comments as done.

Addressed comments.

I didn't do the move to services.$domain yet because I can't get the tests to pass so far.

mollekopf marked 2 inline comments as done.Aug 17 2021, 5:02 PM
mollekopf added inline comments.
src/app/AuthAttempt.php
64

I don't think so. We're not currently using isExpired() (but we could of course), and the idea is that that the entry isAccepted until it expires (at which point it is no longer accepted.

191

You're right, we no longer need it because we now refresh $this.

src/tests/Feature/Controller/AuthAttemptsTest.php
100

The timestamp is sometimes the same, so I changed it so the order doesn't matter).

src/tests/Feature/Controller/NGINXTest.php
38

We're now using ned in the seeder.

machniak requested changes to this revision.Aug 17 2021, 6:32 PM
machniak added inline comments.
src/app/Http/Controllers/API/V4/CompanionAppsController.php
50

I think 403 will be more appropriate. We don't use 401 outside of the AuthController.

src/tests/Feature/Controller/AuthAttemptsTest.php
129

This however will also pass if $json[0][id] == $json[1][id] ;) so, it could be more precise.

This revision now requires changes to proceed.Aug 17 2021, 6:32 PM
mollekopf updated this revision to Diff 7852.Aug 18 2021, 10:25 AM
mollekopf marked 6 inline comments as done and an inline comment as not done.

Addressed comments, moved to services. url

mollekopf marked 3 inline comments as done.Aug 18 2021, 10:28 AM
mollekopf added inline comments.
src/app/Http/Controllers/API/V4/AuthAttemptsController.php
52

That's just the default case.

machniak requested changes to this revision.Aug 18 2021, 12:15 PM
machniak added inline comments.
src/app/Http/Controllers/API/V4/CompanionAppsController.php
27

device_id column size is 100 in the database. The notification_token column has no limit set, which it should because otherwise it uses the default length, which value I never remember.

This revision now requires changes to proceed.Aug 18 2021, 12:15 PM
mollekopf updated this revision to Diff 7891.Aug 18 2021, 12:28 PM

Included console tests

mollekopf updated this revision to Diff 7897.Aug 18 2021, 12:47 PM

Adjusted notification token and device id size limits

mollekopf marked an inline comment as done.Aug 18 2021, 12:47 PM
machniak requested changes to this revision.Aug 19 2021, 10:33 AM
1) Tests\Feature\Controller\AuthAttemptsTest::testDetails
ErrorException: Undefined index: ip

/home/alec/repos/kolab/src/tests/Feature/Controller/AuthAttemptsTest.php:97

2) Tests\Feature\Controller\AuthAttemptsTest::testList
ErrorException: Use of undefined constant id - assumed 'id' (this will throw an Error in a future version of PHP)

/home/alec/repos/kolab/src/tests/Feature/Controller/AuthAttemptsTest.php:129
src/app/AuthAttempt.php
178

Return value is not self explanatory, please add some description.

src/app/CompanionApp.php
29

I'd add @throws info.

65

Add @throws

src/app/Http/Controllers/API/V4/NGINXController.php
43

This info message is not very useful.

src/database/migrations/2021_05_05_134357_create_companion_apps_table.php
21

This line exceeds 120 characters limit

This revision now requires changes to proceed.Aug 19 2021, 10:33 AM
mollekopf updated this revision to Diff 7933.Aug 19 2021, 11:35 AM
mollekopf marked 5 inline comments as done.

Addressed comments, the nginx backend is always without ssl

mollekopf updated this revision to Diff 7942.Aug 19 2021, 1:21 PM

Rebased on master

mollekopf updated this revision to Diff 7948.Aug 19 2021, 2:38 PM

Addressed comment and fixed flakyness of purge command test

mollekopf updated this revision to Diff 7957.Aug 19 2021, 3:03 PM

Removed part that accidentally ended up in the wrong patch.

To begin with, Kacey Alexander Gaulden is a celebrity kid. Further, he is the son of a famous rapper. His father’s stage name is NBA Youngboy. But Youngboy’s actual name is Kentrell DeSean Gaulden. Lastly, Kacey is only two years old.

To begin with, Cody Alan Williams is the son of a famous actor. He is the son of Robin Williams. Further, his mother’s name is Marsha Graces. Robin Williams is one of the best comedians in America. On the other hand, his mother, Marsha is a film producer. Lastly, Robin Williams passed away six years ago. Read through the article to learn more about Cody William's life.

mollekopf reclaimed this revision.Aug 19 2021, 7:27 PM
This revision was not accepted when it landed; it landed in state Needs Review.Aug 20 2021, 1:22 PM
This revision was automatically updated to reflect the committed changes.