Details

Reviewers

Group Reviewers

Restricted Project

Commits

rKf1a6d6b2c90e: MFA via CompanionApp
rKa80808b33a66: MFA via CompanionApp
rKc12d5964b57c: MFA via CompanionApp
rKc9634fe7791b: MFA via CompanionApp
rK13e0389f9b5a: MFA via CompanionApp
rKa9506b3c553a: MFA via CompanionApp

Summary

API is protected by either api (full access), or mfa scopes
The second factor only interacts via the mfa scope
Each companion app get's it's own oauth_client, which enforces the allowed scope via the new allowed_scopes column + the TokenObserver.
MFA is not checked for MFA device interactions. This is to allow pairing & using a new device if an existing one was e.g. lost.
The QR-Code is now printable and includes the generated oauth_client secret. Together with the relaxation of mfa-checking for the mfa scope, this allows to create & print recovery qr-codes.

Further changes:

The companion app is now offered for direct download via configurable download link.
The companion app primary key is now a uuid (in binary form), since it's exposed in the api.

Notes:

The companion app table is truncated on migration because nothing is currently relying on it and no important data is lost.

Builds on D3698

Test Plan

phpunit

Diff Detail

Repository

rK kolab

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

mollekopf requested review of this revision.Nov 4 2022, 3:43 PM

mollekopf created this revision.

Harbormaster completed remote builds in B40484: Diff 11237.Nov 4 2022, 3:43 PM

Fixed linting issues

Harbormaster completed remote builds in B40493: Diff 11246.Nov 4 2022, 4:54 PM

mollekopf edited the summary of this revision. (Show Details)Nov 4 2022, 4:56 PM

mollekopf edited the test plan for this revision. (Show Details)

mollekopf added a reviewer: Restricted Project.

mollekopf added a subscriber: Restricted Project.

Cleanup

Harbormaster completed remote builds in B40496: Diff 11252.Nov 4 2022, 5:00 PM

Now with tests.

Harbormaster completed remote builds in B40508: Diff 11264.Nov 7 2022, 9:44 AM

machniak requested changes to this revision.Nov 7 2022, 11:34 AM

machniak subscribed.

machniak added inline comments.

src/app/Http/Controllers/API/V4/CompanionAppsController.php
61	Name validation?
66	I guess mfa_enabled=false and device_id='' could become defaults on the database level, so we could skip it here.
99–102	I'd add \|string\| to all these rules.
src/app/Observers/TokenObserver.php
3 ↗	(On Diff #11264)	`App\Observers\Passport` would be better.
src/app/PassportClient.php
3 ↗	(On Diff #11264)	I wonder whether `App\Auth` would be a better location for this file.
src/app/Providers/AuthServiceProvider.php
47	API.
src/database/migrations/2022_11_04_120000_companion_app_uuids_oauth_client.php
23	I think it would be better to call it `oauth_client_id`. Then it would be good to define this relation in CompanionApp model.
src/resources/lang/en/ui.php
46	I think "recovery code" would be better.
47	Remove "for".
54	"confirm if it was you, or deny otherwise"?
55	"IP address"
74	"as" -> "as a "
src/resources/vue/CompanionApp/Info.vue
24	`icon="print"`, I'd also make the button using btn-outline-secondary (or -primary).
60	require('@fortawesome/free-solid-svg-icons/faPrint').definition
122	We definitely should make the pairing/device creation process a two-step operation, i.e. don't jump to the list after the first step.

This revision now requires changes to proceed.Nov 7 2022, 11:34 AM

Addressed the comments

I didn't manage to use the relations magic in laravel. It seems specific table names are expected and required.
The location.reload() that I had to introduce seems wrong.

Harbormaster completed remote builds in B40514: Diff 11270.Nov 7 2022, 10:13 PM

Cleanup of a bunch of accidentally added changes

Harbormaster completed remote builds in B40517: Diff 11276.Nov 7 2022, 10:18 PM

machniak requested changes to this revision.Nov 8 2022, 9:02 AM

machniak added inline comments.

src/app/Http/Controllers/API/V4/CompanionAppsController.php
39	You call passportClient() two times here.
256	Again, a redundant sql query.
src/resources/vue/CompanionApp/Info.vue
125	There's a trick to do that. See `beforeRouteUpdate()` in `resources/vue/Admin/User.vue`.

This revision now requires changes to proceed.Nov 8 2022, 9:02 AM

mollekopf marked 3 inline comments as done.Nov 9 2022, 8:59 AM

deduplicate sql queries, router reload

Harbormaster completed remote builds in B40535: Diff 11282.Nov 9 2022, 8:59 AM

Fixed lint issues

Harbormaster completed remote builds in B40538: Diff 11285.Nov 9 2022, 9:01 AM

mollekopf edited the test plan for this revision. (Show Details)Nov 10 2022, 1:02 PM

Fixed unittest

Harbormaster completed remote builds in B40553: Diff 11291.Nov 14 2022, 11:07 AM

vanmeeuwen edited the summary of this revision. (Show Details)Nov 14 2022, 2:28 PM

String based uuid, fixed test

Harbormaster completed remote builds in B40580: Diff 11297.Nov 15 2022, 10:25 AM

Three small suggestions added, but it's acceptable anyway.

src/resources/lang/en/ui.php
72	"but you can re-pair the device later." or "but you can pair the device again."
src/resources/vue/CompanionApp/Info.vue
23	I think we can remove this h5 element. It looks better without it.
108	I think we could improve the printing feature so it prints only the qr code, and no other page elements, but it's not a show-stopper.

This revision is now accepted and ready to land.Nov 16 2022, 3:15 PM

mollekopf marked 2 inline comments as done.Nov 16 2022, 4:11 PM

mollekopf added inline comments.

src/resources/vue/CompanionApp/Info.vue
108	Yeah, I looked briefly into this, but couldn't get it to work quickly, so I'll leave it at this for now.

Closed by commit rKf1a6d6b2c90e: MFA via CompanionApp (authored by mollekopf). · Explain WhyNov 16 2022, 4:27 PM

This revision was automatically updated to reflect the committed changes.

MFA via CompanionApp
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 11306

src/.env.example

src/app/Auth/PassportClient.php

src/app/CompanionApp.php

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

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

src/app/Http/Kernel.php

src/app/Observers/Passport/TokenObserver.php

src/app/Providers/AuthServiceProvider.php

src/app/User.php

src/app/Utils.php

src/config/app.php

src/database/migrations/2022_11_04_120000_companion_app_uuids_oauth_client.php

src/database/migrations/2022_11_04_130000_oauth_client_scopes.php

src/database/seeds/local/OauthClientSeeder.php

src/resources/js/user/routes.js

src/resources/lang/en/app.php

src/resources/lang/en/ui.php

src/resources/vue/CompanionApp.vue

src/resources/vue/CompanionApp/Info.vue

src/resources/vue/CompanionApp/List.vue

src/resources/vue/CompanionApp/ListWidget.vue

src/resources/vue/Dashboard.vue

src/routes/api.php

src/tests/Feature/Controller/CompanionAppsTest.php

src/tests/TestCase.php

MFA via CompanionAppClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 11306

src/.env.example

src/app/Auth/PassportClient.php

src/app/CompanionApp.php

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

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

src/app/Http/Kernel.php

src/app/Observers/Passport/TokenObserver.php

src/app/Providers/AuthServiceProvider.php

src/app/User.php

src/app/Utils.php

src/config/app.php

src/database/migrations/2022_11_04_120000_companion_app_uuids_oauth_client.php

src/database/migrations/2022_11_04_130000_oauth_client_scopes.php

src/database/seeds/local/OauthClientSeeder.php

src/resources/js/user/routes.js

src/resources/lang/en/app.php

src/resources/lang/en/ui.php

src/resources/vue/CompanionApp.vue

src/resources/vue/CompanionApp/Info.vue

src/resources/vue/CompanionApp/List.vue

src/resources/vue/CompanionApp/ListWidget.vue

src/resources/vue/Dashboard.vue

src/routes/api.php

src/tests/Feature/Controller/CompanionAppsTest.php

src/tests/TestCase.php

MFA via CompanionApp
ClosedPublic
Actions

Revision Contents
Changeset List