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

Remote device access, editing and snapshotting with Auth enabled #2042

Merged
merged 40 commits into from
May 10, 2023

Conversation

Steve-Mcl
Copy link
Contributor

@Steve-Mcl Steve-Mcl commented Apr 25, 2023

Description

Support remote device access, editing and snapshotting with NR token Auth to prevent local access

TODO:

  • documentation
  • tests
  • audit log entries - open a task to capture this

Notes for debugging and comprehension

  1. The auth method is essentially as described in the issue mermaid diagram except for the route names as described by Bens comment in the following comment n the issue
  2. Search the code for "Enable Device Editor" to see the 12 steps of acheiving a successful tunnel
    1. image
    2. Each step indicates source and destination, protocol and a brief description
    3. This will also aid completing documentation
  3. I stand on the shoulders of giants - Ben. The tunnelling was first realised by Ben (thanks @hardillb) BUT adding Auth and controlling the tunnel, reigning in the many of async problems (had to add cmnd/resp support!), head twisting, torturous job to get it all working (to prevent local access but permit tunnelled access). So my apologies for the jumbled tired comments, lateness of this PR and the size of it but after working ~48h SOLID (Fri ~ Now) and debugging 401s, 503s, xhr cancelled, no response, no auto-login, wrong token, wrong auth header, missing auth headers, alternative auth methods and a thousand other problems, I am really really t...

Related PRs

FlowFuse/device-agent#77

Related Issue(s)

#1821

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 flowforge/helm to update ConfigMap Template
    • Issue/PR raised on flowforge/CloudProject to update values for Staging/Production

Labels

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

hardillb and others added 14 commits April 14, 2023 15:33
 to get positive response to a command (ie not fire and forget)
  necessary for interlocking and correct representation of tunnel state
  before user attempts access
  Done in a generic way to permit future command/response opportunities

Adds callback to MQTT publish and `device.sendCommand`
Adds response topic to device MQTT
Updates MQTT ACLs for response topic
Adds `comms.device` > `sendCommandAsync()` and `sendCommandAwaitReply()`
Adds mode flag and API for setting Device Mode (autonomous/programmer)
Updates device view to include mode and tunnel info
Add permissions preHandlers
Make the whole process more interlocked by using the new TunnelManager
  and utelising the new Cmnd/Resp MQTT calls
Document the 12 steps to connection (please keep comments for future ref
Ensure WS connections close on error (add include WS error codes)
Document APIs
support head and options
dont look for `projectSettings` on a tunnelled device (fixes crash)
improve tracing (through comments) of the 12 steps to achieve a tunnel
fix auth by switching to x-access-token
Steve-Mcl and others added 3 commits May 2, 2023 17:15
non enterprise should not show elements of this feature
add Developer Mode Only adjacent to the section title
flask tooltip to read "Developer Mode"
re-word "Device Mode" choice dialog
@Steve-Mcl
Copy link
Contributor Author

Following dev-demo live (p)review, additional pushes have been made to address ui/ux and security.

Security

  • URL for Admin is /device-editor/ ✅
  • access_token=xxxx appended to editor url ✅
  • FF Platform account necessary to access (browser should have session)
    • I.e. only operable when logged in to FF ✅
    • I.e. not operable from another browser using proxy URL or local access ✅

Enterprise Only

  • Elements are hidden for community edition ✅

UI / UX

  • Developer Options - add smaller, lighter “Developer Mode Only” text to the right ✅
  • Device Editor should be 3 columns ✅
    • Label
    • State
    • Toggle Button
  • Create Snapshot should be 3 columns ✅
    • Label
    • Blank
    • Create Snapshot button
  • Tooltip for the flask icon
    • Should say “Developer Mode” ✅
  • Device Mode Modal
    • Remove mention of “Autonomous mode” from UI - essentially, Developer mode on/off ✅

Still todo (either this or later iteration)

  • Timeout of enabling tunnel should revert state to show actual state
    • We witnessed the device being "broken" but tunnel still showed "enabled". State should reflect condition.
    • NOTE: This is slightly different now we only have a "toggle" button for enabling/disabling device editor access

@knolleary
Copy link
Member

Timeout of enabling tunnel should revert state to show actual state
We witnessed the device being "broken" but tunnel still showed "enabled". State should reflect condition.
NOTE: This is slightly different now we only have a "toggle" button for enabling/disabling device editor access

I think we definitely need something in the first iteration then ensures the state doesn't get out of sync if the device is offline. Otherwise it will say the editor is enabled, but the button never comes active - without any indication what is wrong.

@Steve-Mcl
Copy link
Contributor Author

New Tests passing (except the common Reached heap limit Allocation failed - JavaScript heap out of memory)

@Steve-Mcl Steve-Mcl marked this pull request as ready for review May 4, 2023 13:33
@Steve-Mcl Steve-Mcl requested a review from knolleary May 5, 2023 12:55
@knolleary
Copy link
Member

Is there a summary of all of the new api routes this PR adds? It's quite hard to get the big picture trying to pick them all out individually and the external apis are a key part of the design.

PRs of this size and complexity should ideally summarise their key changes in the description, including db changes, api changes etc.

Given this is EE licensed functionality, some of this should be under the ee src tree - such as the DeviceTunnelManager and api/deviceEditor - so the relevant routes only get added if a license is applied.

Timeout of enabling tunnel should revert state to show actual state
We witnessed the device being "broken" but tunnel still showed "enabled". State should reflect condition.
NOTE: This is slightly different now we only have a "toggle" button for enabling/disabling device editor access

Has this been resolved in the subsequent commits?

@knolleary knolleary added the area:migration Involves a database migration label May 9, 2023
@Steve-Mcl
Copy link
Contributor Author

@knolleary

I have merged and performed local testing (approx 1hr, enable, disable, with MQTT down, with device up/down etc)
Much more solid with sendOrQueue mod to agent. (thanks for that)

fixed up button text wrapping in f82ad24

chrome_1WShKzm9vF

Other than that, ready for review/merge.

Co-authored-by: Nick O'Leary <nick.oleary@gmail.com>
reply.send(app.db.views.ProjectSnapshot.snapshot(snapShot))
})

async function assignDeviceToProject (device, project) {
Copy link
Contributor Author

@Steve-Mcl Steve-Mcl May 10, 2023

Choose a reason for hiding this comment

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

Suggested change
async function assignDeviceToProject (device, project) {
// #region Helpers
async function assignDeviceToProject (device, project) {

Missing #region marker

@knolleary knolleary merged commit b2f882b into main May 10, 2023
3 of 4 checks passed
@knolleary knolleary deleted the device-editor branch May 10, 2023 09:43
@knolleary knolleary linked an issue May 10, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:migration Involves a database migration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FlowForge should show device agent version in the dashboard Access the Node-RED Editor on Devices
3 participants