Page MenuHomekolab.org

MFA via CompanionApp
ClosedPublic

Authored by mollekopf on Nov 4 2022, 3:43 PM.

Details

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mollekopf requested review of this revision.Nov 4 2022, 3:43 PM
mollekopf created this revision.
mollekopf updated this revision to Diff 11246.Nov 4 2022, 4:54 PM
mollekopf retitled this revision from 2FA stuff to MFA via CompanionApp.
mollekopf edited the summary of this revision. (Show Details)

Fixed linting issues

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.
mollekopf updated this revision to Diff 11252.Nov 4 2022, 5:00 PM
mollekopf edited the summary of this revision. (Show Details)

Cleanup

mollekopf updated this revision to Diff 11264.Mon, Nov 7, 9:44 AM

Now with tests.

machniak requested changes to this revision.Mon, Nov 7, 11:34 AM
machniak added a subscriber: machniak.
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.Mon, Nov 7, 11:34 AM
mollekopf updated this revision to Diff 11270.Mon, Nov 7, 10:13 PM
mollekopf marked 15 inline comments as done.

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.
mollekopf updated this revision to Diff 11276.Mon, Nov 7, 10:18 PM

Cleanup of a bunch of accidentally added changes

machniak requested changes to this revision.Tue, Nov 8, 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.Tue, Nov 8, 9:02 AM
mollekopf marked 3 inline comments as done.Wed, Nov 9, 8:59 AM
mollekopf updated this revision to Diff 11282.Wed, Nov 9, 8:59 AM

deduplicate sql queries, router reload

mollekopf updated this revision to Diff 11285.Wed, Nov 9, 9:01 AM

Fixed lint issues

mollekopf edited the test plan for this revision. (Show Details)Thu, Nov 10, 1:02 PM
mollekopf updated this revision to Diff 11291.Mon, Nov 14, 11:07 AM

Fixed unittest

vanmeeuwen edited the summary of this revision. (Show Details)Mon, Nov 14, 2:28 PM
mollekopf updated this revision to Diff 11297.Tue, Nov 15, 10:25 AM
mollekopf edited the summary of this revision. (Show Details)

String based uuid, fixed test

machniak accepted this revision.Wed, Nov 16, 3:15 PM

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.Wed, Nov 16, 3:15 PM
mollekopf marked 2 inline comments as done.Wed, Nov 16, 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 Christian Mollekopf <mollekopf@apheleia-it.ch>). · Explain WhyWed, Nov 16, 4:27 PM
This revision was automatically updated to reflect the committed changes.