Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Quick Connect device to the core #3388

Merged
merged 18 commits into from
Feb 2, 2024

Conversation

Steve-Mcl
Copy link
Contributor

@Steve-Mcl Steve-Mcl commented Jan 25, 2024

Description

Adds support for Quick Connect feature whereby a device-agent (V2.1+) can be executed with a simple 3 word phrase (OTC aka One-Time-Code)

Following discussion, an additional API was not implemented but instead, built upon the POST route that Device Provisioning takes (POST api/v1/devices/). The validation schema was updated to permit only one of name + type (creating a device) OR setup (as in spending the OTC and setting up a device).

The OTC is stored in the AccessTokens (base64 format) and is intended to be used by the Device-Agent as an authorization header (bearer token) that, through existing logic, retrieves (or expires) the OTC token, validates it, sets up the route session (with device ID and a flag setup) and immediately destroys the OTC token

Details

The OTC is stored as a token in the AccessTokens table. It has a scope of device:otc and there can only ever be one per device. Regenerating credentials also refreshes the OTC (this was not extra work but a happy accident that I am leaving in because it improves the overall UX since anyone re-generating device credentials will be offered a new OTC setup command - win win)

Tests:

All new except regenerates a devices credentials which was updated

 AccessToken controller
    Device Provisioning Tokens
      Device Quick Connect One-Time-Code
        ✔ creates a one-time-code token and a device access token (83ms)
        ✔ recreates one-time-code token, there can only be one
        ✔ does not create a one-time-code when refreshing device tokens with `refreshOTC: false` (85ms)

  db utils
    randomStrings
      ✔ should be a function
      ✔ should return an array of 3 strings, any length
      ✔ should return an array of 5 strings, min length 8
      ✔ should return an array of strings, min length 3, max length 5
    randomPhrase
      ✔ should be a function
      ✔ should return a string
      ✔ should return a string of 8 or more characters by default
      ✔ should return a string with 3 words by default
      ✔ should return a string with 5 words
      ✔ should return a string with 3 words, min length 8
      ✔ should return a string with 3 words, min length 8, max length 10
      ✔ should return a string with 3 words, min length 8, max length 10, using _ as separator

  Device API
    Generate Device credentials
      ✔ regenerates a devices credentials (175ms)  // << Updated only
      ✔ regenerates a devices credentials when a device quick connects (151ms)

Related Issue(s)

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on FlowFuse/helm to update ConfigMap Template
    • Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production

Labels

  • Backport needed? -> add the backport label
  • Includes a DB migration? -> add the area:migration label

@Steve-Mcl Steve-Mcl linked an issue Jan 25, 2024 that may be closed by this pull request
Copy link

codecov bot commented Jan 25, 2024

Codecov Report

Attention: 33 lines in your changes are missing coverage. Please review.

Comparison is base (749fe26) 41.31% compared to head (eb9c354) 41.37%.
Report is 45 commits behind head on main.

Files Patch % Lines
...s/team/Devices/dialogs/DeviceCredentialsDialog.vue 0.00% 24 Missing ⚠️
forge/routes/api/device.js 74.28% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3388      +/-   ##
==========================================
+ Coverage   41.31%   41.37%   +0.05%     
==========================================
  Files         608      610       +2     
  Lines       22825    22942     +117     
  Branches     5515     5544      +29     
==========================================
+ Hits         9431     9493      +62     
- Misses      13394    13449      +55     
Flag Coverage Δ
backend 78.25% <90.32%> (+<0.01%) ⬆️
frontend 2.03% <0.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Steve-Mcl Steve-Mcl marked this pull request as ready for review January 26, 2024 13:53
@Steve-Mcl Steve-Mcl marked this pull request as draft January 29, 2024 13:03
@Steve-Mcl
Copy link
Contributor Author

Converted to draft while requested redesign is carried out

@Steve-Mcl Steve-Mcl removed the request for review from knolleary January 29, 2024 13:03
@Steve-Mcl Steve-Mcl marked this pull request as ready for review January 29, 2024 16:04
Copy link
Member

@knolleary knolleary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One rewording suggested in the dialog.

As for tests, I would like to see a test that verifies an invalid token is rejected when used with POST /api/v1/devices.
Otherwise, looks good.

@Steve-Mcl
Copy link
Contributor Author

As for tests, I would like to see a test that verifies an invalid token is rejected when used with POST /api/v1/devices.

Added tests in add tests for bad otc behaviour 0027116:

test/unit/forge/db/controllers/AccessToken_spec.js

  AccessToken controller
    Device Provisioning Tokens
      Device Quick Connect One-Time-Code
        ✔ returns null for unknown token (86ms)

test/unit/forge/routes/api/device_spec.js

  Device API
    Generate Device credentials
      ✔ returns 404 for invalid OTC (85ms)

@Steve-Mcl
Copy link
Contributor Author

@knolleary addressed comments but before you merge, please consider comment here: FlowFuse/device-agent#226 (comment)

@knolleary knolleary self-requested a review January 31, 2024 10:59
Copy link
Member

@knolleary knolleary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in my previous review, there were other instances of -u that needed fixing - have proposed those changes in this review and will apply them.

docs/device-agent/quickstart.md Outdated Show resolved Hide resolved
docs/device-agent/quickstart.md Outdated Show resolved Hide resolved
@knolleary knolleary merged commit 3f2ac1c into main Feb 2, 2024
10 checks passed
@knolleary knolleary deleted the 3256-quick-connect-device branch February 2, 2024 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Device Onboarding - device.yml improvements
2 participants