Page MenuHomekolab.org

CompanionApp support
ClosedPublic

Authored by mollekopf on Nov 15 2021, 10:04 AM.

Details

Summary

The CompanionAppsController provides the API for pairing a companion app.

For the UI this patch adds a companion-app dashboard component that allows for:

  • Pairing a companion-app via qr-code
  • Listing paired companion apps
  • Forgetting paired companion apps.

Please note that there is currently no mechanism in place to block a companion app. Forgetting the app will delete it, but the app can simply reconnect via the oauth token that is still on the device.

For blocking we could:

  • invalidate the oauth credentials so the app simply no longer has access to the api
    • The device could obtain a new token given the password is provided again.
  • Not delete the device entry, but instead have a "blocked" state.

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 15 2021, 10:04 AM
mollekopf created this revision.
mollekopf updated this revision to Diff 9283.Jan 11 2022, 8:54 PM

Fixed some phpstan complaints

mollekopf retitled this revision from CompanionApp WIP to CompanionApp support.Jan 12 2022, 12:28 AM
mollekopf edited the summary of this revision. (Show Details)
mollekopf added a reviewer: Restricted Project.
mollekopf edited the summary of this revision. (Show Details)
mollekopf added a subscriber: Restricted Project.
mollekopf updated this revision to Diff 9622.Feb 14 2022, 11:33 AM

Create an oauth client in the seeder, other small fixes

mollekopf updated this revision to Diff 9628.Feb 14 2022, 11:55 AM

Controller tests

mollekopf edited the summary of this revision. (Show Details)Feb 14 2022, 11:56 AM
mollekopf edited the summary of this revision. (Show Details)Feb 14 2022, 12:36 PM
machniak requested changes to this revision.Feb 15 2022, 12:55 PM
machniak added a subscriber: machniak.
machniak added inline comments.
src/app/Http/Controllers/API/V4/CompanionAppsController.php
106

We usually do something like this:

return response()->json([
                'status' => 'success',
                'message' => \trans("app.{$this->label}-delete-success"),
        ]);
112

This line should be removed.

177

It does not return void. The description also is wrong.

src/resources/js/fontawesome.js
61

These lists are in an alphabetic order, please.

src/resources/vue/CompanionApp.vue
6

I'd add <small><sup class="badge bg-primary">{{ $t('dashboard.beta') }}</sup></small> to the title.

9

All text needs to be moved to localization.

52

There's no modal on the page.

65

Instead, we can just add @click="$root.tab" to the nav-link elements.

src/resources/vue/Dashboard.vue
42

I'd add <span class="badge bg-primary">{{ $t('dashboard.beta') }}</span> here.

src/resources/vue/Widgets/CompanionappList.vue
14

I'm not sure it make sense to display the creation date here. And I would not display it as a first column. Shouldn't we display a name?

20

Why not just the "usual" red trash-alt icon?

51

I guess we should refresh the list, and display the usual confirmation message.

src/routes/api.php
67

I'd move this line, one line up

src/tests/Feature/Controller/CompanionAppsTest.php
94

This user should be deleted in setUp() and tearDown().

106

This user should be deleted in setUp() and tearDown().

126

I miss a test for 403 case.

136

Just testPairing.

146

We should test that it is a data: URI. Maybe even also that it is a valid image content.

This revision now requires changes to proceed.Feb 15 2022, 12:55 PM
mollekopf updated this revision to Diff 9736.Feb 25 2022, 4:47 PM
mollekopf marked 18 inline comments as done.

Addressed comments and moved the companion app ui to beta

mollekopf updated this revision to Diff 9739.Feb 25 2022, 4:51 PM

alphabetic ordering

mollekopf added inline comments.Feb 25 2022, 5:22 PM
src/routes/api.php
67

We can't because registering 'companion/pairing' after the resource will not work (it returns a 404). Doing it this way around works though.

While it doesn't necessarily block this patch, a major issue that we'll have to resolve eventually is that we currently have no means of revoking an individual device.

In theory we could revoke the access token, and thus require the app to log-in again (via password), but in practice I haven't found a way to associate the token + refresh-token with a specific companion app,
because both tokens (as far as I understand), will eventually change and be replaced, with no link to what we had before.

One way to get there would be to create an oauth client per device, but that doesn't seem like a great approach and would require some magic creating clients.

Another approach would be to treat App::CompanionApp instances instead App::User as the authenticable. This would allow us to only make parts of the API available for companionapps, and would also allow us revoking the tokens of a specific device (instead of just all devices).
However, I'm not sure how invasive the change is going to be.

mollekopf updated this revision to Diff 9916.Mar 21 2022, 9:51 AM

Rebased on the laravel v9

Harbormaster completed remote builds in B38698: Diff 9916.
mollekopf updated this revision to Diff 9922.Mar 21 2022, 11:45 AM

Fixed token revocation + test

Harbormaster completed remote builds in B38704: Diff 9922.
mollekopf updated this revision to Diff 9925.Mar 21 2022, 11:46 AM

Fixed long line

This revision was not accepted when it landed; it landed in state Needs Review.Apr 21 2022, 1:33 PM
This revision was automatically updated to reflect the committed changes.