Page MenuHomePhorge

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

Authored by mollekopf on Jul 16 2021, 10:12 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 15, 11:30 PM
Unknown Object (File)
Fri, Mar 8, 12:14 AM
Unknown Object (File)
Thu, Mar 7, 3:00 AM
Unknown Object (File)
Thu, Mar 7, 3:00 AM
Unknown Object (File)
Thu, Mar 7, 3:00 AM
Unknown Object (File)
Thu, Mar 7, 3:00 AM
Unknown Object (File)
Thu, Mar 7, 3:00 AM
Unknown Object (File)
Thu, Mar 7, 2:47 AM

Diff Detail

Repository
rK kolab
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 35656
Build 14014: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
mollekopf marked 27 inline comments as done.

Addressed comments

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
65

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

192

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

src/app/Console/Commands/AuthAttempt/PurgeCommand.php
33 ↗(On Diff #7762)

\App\ prefix not needed here.

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

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

53

Should that set a specific $authAttempt->reason?

74

This debug message is not very helpful.

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

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

24

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.

34

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

57

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 ↗(On Diff #7762)

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
34

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

51

I used to comment controller tests with something like this:

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

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

src/tests/Feature/Controller/CompanionAppsTest.php
49 ↗(On Diff #7762)

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 ↗(On Diff #7762)

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

47 ↗(On Diff #7762)

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

This revision now requires changes to proceed.Aug 17 2021, 8:46 AM
src/tests/Feature/Controller/AuthAttemptsTest.php
98

This can be removed.

100

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

101

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.

src/app/AuthAttempt.php
153

Found by phpstan, should be $clientIP

vanmeeuwen subscribed.
vanmeeuwen added inline comments.
src/routes/api.php
150

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 ↗(On Diff #7762)

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

src/config/imap.php
9 ↗(On Diff #7627)

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 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 added inline comments.
src/app/AuthAttempt.php
65

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.

192

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

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

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

src/tests/Feature/Controller/NGINXTest.php
38 ↗(On Diff #7762)

We're now using ned in the seeder.

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 marked 6 inline comments as done and an inline comment as not done.

Addressed comments, moved to services. url

mollekopf added inline comments.
src/app/Http/Controllers/API/V4/AuthAttemptsController.php
53

That's just the default case.

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

Adjusted notification token and device id size limits

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
42 ↗(On Diff #7897)

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 marked 5 inline comments as done.

Addressed comments, the nginx backend is always without ssl

Addressed comment and fixed flakyness of purge command test

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.

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.