Add app device access verification to ACL manager #3074
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3074 +/- ##
==========================================
- Coverage 39.83% 39.76% -0.07%
==========================================
Files 573 578 +5
Lines 20692 21020 +328
Branches 4929 5037 +108
==========================================
+ Hits 8242 8359 +117
- Misses 12450 12661 +211
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
more aligned with nr-project-nodes readme
forge/ee/lib/projectComms/index.js
Outdated
| // Send message to other project | ||
| // - ff/v1/<team>/p/+/in/+/# | ||
| // Send message to specific project | ||
| // A device ID is exactly 10 chars long, so any topic with an ID longer than that |
There was a problem hiding this comment.
It's a bit hard to figure out due to the way the git diff is showing the changes - but I'd expect any future support for a publish targeting a device to be on a /d/ topic, not a /p/ topic - so not sure this reliance on the length of this topic component is something we should rely on.
It would be helpful to a have a summary of the topics/rules being introduced in the PR description for ease of reference. I haven't got to the other related PRs to know if you've documented them elsewhere, but as this is the PR dealing with changing the platform ACLs, it would be handy to have here.
There was a problem hiding this comment.
It was an absolute melon twister Nick 😄
I will attempt to answer the your questions in a meaningful manner (I am also working on suitable documentation for future reference)
but I'd expect any future support for a publish targeting a device to be on a /d/ topic, not a /p/ topic
Agreed and considered and will likely be how it is handled for instance/device → device, but at this point in time, it is not supported and is in effect interlocked by insisting on the topic being 11 or more characters. I suspect I will remove these once I have a working schema for instance/device → device
However, it was necessary to differentiate a PUB originating from an instance device vs PUB originating from an app assigned device in some scenarios and therefore an alternate verification routine was needed
Consider these
"Send message to specific project"
instance assigned device publishes direct to a project
- The device has access to a
projectIdand can form the topicff/v1/7N152GxG2p/p/23d79df8-183c-4104-aa97-8915e1897326/in/a/b- this is the topic it uses to PUB direct to project
23d79df8-183c-4104-aa97-8915e1897326
- this is the topic it uses to PUB direct to project
- The receiving projects' Project-In node subscribes to
a/bwhich equates toff/v1/7N152GxG2p/p/23d79df8-183c-4104-aa97-8915e1897326/in/a/b
application assigned device publishes direct to a project
- As the Project In Node will subscribe to
ff/v1/7N152GxG2p/p/23d79df8-183c-4104-aa97-8915e1897326/in/a/bthe device must also publish on this topic
In summary - the /p/ remains even though it is a device publishing
"Broadcast a message"
instance assigned device
- The device has access to a
projectIdand can form the topicff/v1/7N152GxG2p/p/23d79df8-183c-4104-aa97-8915e1897326/out/a/b- this is the topic it uses to PUB broadcasts
23d79df8-183c-4104-aa97-8915e1897326
- this is the topic it uses to PUB broadcasts
Project In subscription
- A receiving projects' Project-In node subscribes to
a/bwhich equates toff/v1/7N152GxG2p/p/+/out/a/b - Therefore a device (both instance and app owned) Project-In node must also subscribe to
ff/v1/7N152GxG2p/p/+/out/a/b
application assigned device
- The device does not have a
projectId, only its owndeviceIdor theapplicationId - Since the code already knows which device is which, I supplant the
projectIdwith theapplicationId - This means we get the topic
ff/v1/7N152GxG2p/p/1234567890/out/a/b(where1234567890is the app id) - This means we cannot use the same verification function since we are not verifying a project ID but an application ID
In summary - the /p/ remains even though it is an application device publishing
To get over this hump, I chose to leave the Project In node as-is and differentiate the verification logic path by the /ID/ provided.
The alternative solution I considered was to modify the Project-In node to either subscribe to both ff/v1/7N152GxG2p/p/... and ff/v1/7N152GxG2p/d/... OR subscribe to ff/v1/7N152GxG2p/+/...
Tomorrow, I hope to have a clearer picture of the direct instance/device → device and a full summary of current existing condition, this PR condition and our final stance.
I think it may be prudent to hang fire and evaluate this before any merges?
PS: I appreciate that ↑ wasn't entirely as clear as I had hoped! Imagine how my head was noodled doing the work ;)
Ensure old flowforge module is removed
|
I have got all tests passing and managed to update the PR OP with a list of ACLs (existing/new/changes) in a format that doesnt mess up regex (harder than you might imagine 😆 ) Anyhow, I have one remaining reservation and that is, as noted before in the review comment, this PR is publishing app-owned-devices broadcasts out on As discussed before, this was to minimise multiple subs or using a wildcard. I am now thinking should we make separate entries in the [ All instances ] This would permit us the change the topic ACL to |
|
Hi @knolleary I have updated the topics and ACLs to use the I will update the OP with the new topic/ACLs for reference shortly. |
|
@knolleary - topic prefix adjusted as per discussion. Had an odd Postrgre only tests fail (re-running it) |
|
Approved pending the postgres test passing |
|
@knolleary I think I clicked Request Approval milliseconds after you clicked "Approved" - can you do it again please? |
|
@Steve-Mcl Postgres test failed again. Need to understand that. |
|
Postgres failure is can be reproduced locally: |
|
The failing test is because it's checking a device cannot broadcase if it doesn't provide the |
knolleary
deleted the
3018-permit-app-assigned-device-project-node-access
branch
part of #3018
Description
This pull request adds device access verification to the ACL manager, allowing for devices assigned to an application to use the nr-project-nodes
The changes include new functions for checking device access to projects and applications, as well as updates to existing functions for verifying device access to topics.
ACL changes
Updated to use
dev:topic prefix 22/11/2023Click to reveal
Receive broadcasts from other projects in the team
ff/v1/<team>/p/+/out/+/#checkTeamIdReceive messages sent to this project
ff/v1/<team>/p/<project>/in/+/#checkTeamAndObjectIdsReceive link-call response messages sent to this project
ff/v1/<team>/p/<project>/res[-id]/+/#checkTeamAndObjectIdsSend message to other project
ff/v1/<team>/p/+/in/+/#checkTeamIdSend link-call response messages to other project
ff/v1/<team>/p/+/res/+/#checkTeamIdSend broadcast messages
ff/v1/<team>/p/<project>/out/+/#checkTeamAndObjectIdsReceive broadcasts from other projects in the team
ff/v1/<team>/p/+/out/+/#checkDeviceIsAssignedReceive messages sent to this device
ff/v1/<team>/p/<project>/in/+/#checkDeviceCanAccessProjectverifychanged...- old:
checkDeviceAssignedToProject- new:
checkDeviceCanAccessProjectReceive link-call response messages sent to this device (instance owned)
ff/v1/<team>/p/<project>/res/+/#checkDeviceCanAccessProjecttopicchanged...- old:
/^ff\/v1\/([^\/]+)\/p\/([^\/]+)\/res\/[^\/]+($|\/.*$)$/- new:
/^ff\/v1\/([^\/]+)\/p\/(?!dev:)([^\/]+)\/res\/[^\/]+($|\/.*$)$/❌
verifychanged...- old:
checkDeviceAssignedToProject- new:
checkDeviceCanAccessProjectSend message to specific project
ff/v1/<team>/p/<project>/in/+/#checkDeviceCanAccessProjecttopicchanged...- old:
/^ff\/v1\/([^\/]+)\/p\/([^\/]+)\/in\/[^\/]+($|\/.*$)/- new:
/^ff\/v1\/([^\/]+)\/p\/(?!dev:)([^\/]+)\/in\/[^\/]+($|\/.*$)/Send broadcast messages (from instance owned devices in the team)
ff/v1/<team>/p/<project>/out/+/#checkDeviceCanAccessProjecttopicchanged...- old:
/^ff\/v1\/([^\/]+)\/p\/([^\/]+)\/out\/[^\/]+($|\/.*$)/- new:
/^ff\/v1\/([^\/]+)\/p\/(?!dev:)([^\/]+)\/out\/[^\/]+($|\/.*$)/❌
verifychanged...- old:
checkDeviceAssignedToProject- new:
checkDeviceCanAccessProjectSend link-call response messages back to source instance
ff/v1/<team>/p/+/res/+/#checkDeviceCanAccessProjecttopicchanged...- old:
/^ff\/v1\/([^\/]+)\/p\/([^\/]+)\/res\/[^\/]+($|\/.*$)/- new:
/^ff\/v1\/([^\/]+)\/p\/(?!dev:)([^\/]+)\/res\/[^\/]+($|\/.*$)/Receive link-call response messages sent to this device (app owned)
ff/v1/<team>/p/dev:<deviceid>/res/+/#checkDeviceIsAssignedtopic:
/^ff\/v1\/([^\/]+)\/p\/dev:([^\/]+)\/res\/[^\/]+($|\/.*$)$/verify:
checkDeviceIsAssignedshared:
falseSend broadcast messages (from app owned devices in the team)
ff/v1/<team>/p/dev:<deviceid>/out/+/#checkDeviceIsAssignedtopic:
/^ff\/v1\/([^\/]+)\/p\/dev:([^\/]+)\/out\/[^\/]+($|\/.*$)/verify:
checkDeviceIsAssignedshared:
falseVerification
Tester will need an instance project with an instance device AND an application assigned device.
Flow for instance/instance device:
Flow for App assigned device
After importing the flows, you will need to select the right targets in the "project-in/out" nodes for your setup.
The test is to operate the injects from instance device reach app device and vice-versa.
This is a demo showing what you should get:
app-device-proj-link.mp4
TODO:
Related Issue(s)
#3018
Checklist
flowforge.yml?FlowFuse/helmto update ConfigMap TemplateFlowFuse/CloudProjectto update values for Staging/ProductionLabels
backportlabelarea:migrationlabel