Incorrect remote instance count on team homepage

[knolleary]

Current Behavior

Noticed this on a customers team. It feels familiar, but couldn't immediately find an open issue.

The customer has a very large number of remote instances registered, however the team home page summary shows:

Remote instance view:

---

In other teams, such as our own internal team that has a moderate number of remote instances registered, the counts match up as expected.

Can share a link to the team in question directly.

Expected Behavior

The counts to match

Steps To Reproduce

I will share the team url privately to whoever picks this up.

Environment

• FlowFuse version:
• Node.js version:
• npm version:
• Platform/OS:
• Browser:

Have you provided an initial effort estimate for this issue?

I have provided an initial effort estimate

[knolleary]

@cstns I have a feeling you're going to tell me exactly what's happening here and that it's something we identified when updating the home page queries a few months ago as a tricky problem to solve due to the way the queries work at scale?

[cstns]

It took me a while to realize what's happening.

The described issue only occurs to visiting admins i.e. an administrator viewing a team that he is not a member of.

The cause of the problem is the 'hasPermission' check from forge/db/models/Device.js:738 receives a null 'membership' object because the visiting admin is not a member of the actual team (the first thing the 'hasPermission' method checks is for the presence of the membership object and returns false if null).

@knolleary, I'd like your input on this, because my first thought was to alter the countByState methods on both device an project models to ignore permission checks when filtering devices/instances but then realized that more of these types of situations might pop up or already exist but we just haven't caught on to them yet (also, filtering resources based on granular rbac permissions is going to be used extensively moving forward).

That being said, the proper solution seems to be to mock admin users as being part of every team by hard-coding the admin role in each getTeamMembership call on the User model:

     getTeamMembership: async function (teamId, includeTeam) {
          // mock admin users to be a member of any team since the 'hasPermission' method first checks for
          // teamMembership and would exclude admins otherwise
          if (this.admin) return { role: Roles.Admin }

          return M.TeamMember.getTeamMembership(this.id, teamId, includeTeam)
      },

[knolleary]

My pause here is that this change would potentially allow admins to start doing things we didn't intend. For example, admins have read-only access to instances (editor) - and that needs to still be the case. We would need to review the impact - not a quick task.