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

WebRelay support for devicemeshrouterlinks.extralinks #5801

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SecareLupus
Copy link

This pull request may affect Issues #4452 and #4886

Company I work for wanted to be able to add custom WebRelay Links for accessing (possibly multiple concurrent) web services on fleet devices. I do not have a good way to test these changes, as I'm not sure the best way to spin up a development instance. Advice for doing so is welcome, and I'd be happy to test these changes myself. Until then, I believe the changes to be likely to work as intended.

Changes I made (in order from least to most important):

  • Moved two comments so they were attached to the top of the code they were referencing. (It appears additional code was inserted between the comment and the commented code at some point)
  • Updated the config.json schema document to clarify the intended use of my changes to configuration.
    • Added two additional legal protocol values, 'wr_http' & 'wr_https'. These values use the same 'p' integer as 'http' and 'https', respectively, for backwards compatibility and to limit the changes needed.
    • Added an optional property, 'path', which is intended to provide endpoint information to the remote web service. Assumed to be compatible with the preconditions needed for the 'addr' argument for the function 'p10WebRouter()' Those preconditions do not appear to be documented in 'default.handlebars'.
  • Modified two sections of code which produce node links based on 'extralinks' configuration values.
    • If the value of r.protocol is set to one of the new values, and WebRouterPort != 0 it calls p10WebRouter(), passing the relevant information as provided by the configuration values.
    • Else, it falls back on current behavior.

The code is not expected to produce any differences from current behavior unless an extralink entry is provided which uses the 'wr_http' or 'wr_https' protocol.

Example config.js based on our usage:

"deviceMeshRouterLinks": {
        "rdp": true,
        "ssh": true,
        "scp": true,
        "extralinks": [
          {
            "name": "NodeRed Flows",
            "protocol": "wr_http",
            "port": 1880,
            "filter": "tag:has-nodered"
          },
          {
            "name": "NodeRed Dashboard",
            "protocol": "wr_http",
            "path": "/ui"
            "port": 1880,
            "filter": "tag:has-nodered"
          },
          {
            "name": "Simple Third Example",
            "protocol": "wr_http",
            "port": 80
          }
       ]
}

@SecareLupus
Copy link
Author

I have some questions about the duplication of code related to this PR. I did my best to make no dramatic changes to structure, but that meant editing two seemingly similar chunks of code. They were not identical, so I was not willing to blindly merge them.

It appears that the function getShortRouterLinks() at line 4795 performs roughly the same task as the block of code starting at 7613, which appears to be a part of the function goToDevice()

They even shared the same misplaced comment, indicating a probable copy-paste at some point. I'm not sure I see why the code at 7613 can't just call getShortRouterLinks(), possibly something to do with the way that node permissions are determined? In any case, I opted not to merge them, and instead applied the changes to each of them separately.

One line that was particularly different between them seems to limit the visibility of Desktop and File Manager links to devices which have a proper agent.
Line 4828 (getShortRouterLinks):
if ((node.mtype == 3) && ((r.protocol == 'mcrdesktop') || (r.protocol == 'mcrfiles'))) continue;
Line 7644 (gotoDevice):
if (((p == 6) || (p == 7)) && (node.mtype != 2)) continue;

Their placement is also different, as one happens before an integer value is set for 'p' and one happens after. This explains most differences, but I'm curious about the difference between node.mtype == 3 to node.mtype != 2. Is this difference intentional, or are they intended to do the same thing and just do it different ways? Should one be changed?

@SecareLupus
Copy link
Author

If it wasn't clear, I'd love eyes and advice from someone more familiar with the project on what next steps should look like. Particularly whether my approach makes sense, whether I'm missing anything obvious, and suggestions for how to test code changes.

@Ylianst
Copy link
Owner

Ylianst commented Mar 4, 2024

Apologies for the delay. Yes, I will take a look at this hopefully in the next few days.

@SecareLupus
Copy link
Author

I did some testing in a javascript console; I think I've misunderstood the use of the 'addr' argument in the p10WebRouter function, and don't expect this to work in its current form. Not sure yet how to fix it, but ready to put in more work on it if pointed in the right direction. Thank you for your time!

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.

None yet

2 participants