Page MenuHomePhorge

CompanionApp support
ClosedPublic

Authored by mollekopf on Nov 15 2021, 10:04 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 27, 12:42 AM
Unknown Object (File)
Mon, Mar 18, 5:58 AM
Unknown Object (File)
Mon, Mar 18, 1:45 AM
Unknown Object (File)
Mon, Mar 11, 12:36 PM
Unknown Object (File)
Tue, Mar 5, 3:06 PM
Unknown Object (File)
Sat, Mar 2, 7:26 PM
Unknown Object (File)
Sat, Mar 2, 7:25 PM
Unknown Object (File)
Wed, Feb 28, 6:19 AM
Subscribers
Restricted Project

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
Branch
master
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 38551
Build 15601: arc lint + arc unit

Event Timeline

mollekopf created this revision.

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.

Create an oauth client in the seeder, other small fixes

machniak subscribed.
machniak added inline comments.
src/app/Http/Controllers/API/V4/CompanionAppsController.php
103

We usually do something like this:

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

This line should be removed.

174

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
72

I'd move this line, one line up

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

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

104

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

124

I miss a test for 403 case.

134

Just testPairing.

144

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 marked 18 inline comments as done.

Addressed comments and moved the companion app ui to beta

src/routes/api.php
72

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.

Fixed token revocation + test

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.