| machniak | |
| vanmeeuwen |
| Restricted Project |
which will then succeed/fail future authentication attempts from the
same ip/username pair.
| No Linters Available |
| No Unit Test Coverage |
| Buildable 36238 | |
| Build 14296: arc lint + arc unit |
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 | ||
| 34 | \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 | ||
| 101 | 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 | ||
| 50 | A test for failing input validation and one for use of other user's device would be nice. | |
| src/tests/Feature/Controller/NGINXTest.php | ||
| 39 | 2fa_enabled = true in the seeder. So, change it here or there. | |
| 48 | I miss tests for error conditions, i.e. all byebye() cases. | |
| src/app/AuthAttempt.php | ||
|---|---|---|
| 153 | Found by phpstan, should be $clientIP | |
| 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 | ||
| 59 | Please use \App\Utils::generatePassphrase(), which in development and with APP_PASSPHRASE creates predictable passwords. | |
Addressed comments.
I didn't do the move to services.$domain yet because I can't get the tests to pass so far.
| 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 | ||
| 39 | We're now using ned in the seeder. | |
| src/app/Http/Controllers/API/V4/AuthAttemptsController.php | ||
|---|---|---|
| 53 | That's just the default case. | |
| 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. | |
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 | |
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.