Page MenuHomePhorge

MFA via CompanionApp
ClosedPublic

Authored by mollekopf on Nov 4 2022, 3:43 PM.
Tags
None
Referenced Files
F13944612: D3932.id11246.diff
Mon, Jul 22, 8:25 AM
F13937598: D3932.id11297.diff
Sun, Jul 21, 9:37 PM
F13937595: D3932.id11297.diff
Sun, Jul 21, 9:37 PM
F13937592: D3932.id11297.diff
Sun, Jul 21, 9:37 PM
F13937589: D3932.id11297.diff
Sun, Jul 21, 9:37 PM
F13937586: D3932.id11297.diff
Sun, Jul 21, 9:37 PM
F13937583: D3932.id11297'[0].diff
Sun, Jul 21, 9:37 PM
F13937580: D3932.id11297'[0].diff
Sun, Jul 21, 9:37 PM
Subscribers
Restricted Project

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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

mollekopf created this revision.
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 test plan for this revision. (Show Details)
mollekopf added a reviewer: Restricted Project.
mollekopf added a subscriber: Restricted Project.
mollekopf edited the summary of this revision. (Show Details)

Cleanup

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
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.

Cleanup of a bunch of accidentally added changes

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

deduplicate sql queries, router reload

mollekopf edited the summary of this revision. (Show Details)

String based uuid, fixed test

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 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.

This revision was automatically updated to reflect the committed changes.