Fixes permission bug with deleting metrics.

[mknowlton89]

Features and Changes

When deleting metrics, we were checking for createAnalyses permissions, but not passing in the projects the metric is associated with, so it was defaulting to checking the user's global role.

This PR also updates the userHasPermission logic for READ_ONLY_TYPE permissions so that it's less fragile around if an empty string or an empty array is passed in for the project prop.

I'd like to do a follow up PR that updates the signature of userHasPermission so it only accepts an array of strings (or its undefined).

This will remove the need for engineers to pass in an empty string or empty array for any resource that can be in multiple projects (e.g. metrics, datasources, etc). And for resources that can only be a part of a single project (feature, experiment, idea) - the engineer will either need to pass in the resource's project, if there is one, or any empty array.

Testing

• Ensure that analysts and experimenters can delete metrics they have created or any metrics that have the ability to edit (e.g. metrics where they have create/edit metrics permissions in every one of the projects the metric is in)
• Ensure that users with a global role of noaccess and project specific experimenter permissions can create/test metrics that are connected to a datasource that is in "All Projects".

[github-actions]

Your preview environment pr-2195-bttf has been deployed.

Preview environment endpoints are available at:

• https://growthbook-3000-pr-2195-bttf.growthbook.okteto.dev
• https://growthbook-3100-pr-2195-bttf.growthbook.okteto.dev

[jdorn]

Is using an empty string here vs array intentional?

[mknowlton89]

The difference between passing in an empty string and an empty array only matters (currently) if the permission that's being checked is of the READ_ONLY_TYPE (e.g. runQueries, readData, or viewEvents).

In those cases, we're checking if the checkProjects that's defined in userHasPermission is equal to [undefined] - and if so, we're adding all of the projects the user has project-specific permissions for before checking hasPermission.

Looking at it this morning, the fix I have for this PR resolves the issue, however, I think refactoring userHasPermission to make it less opinionated on "" vs [] might be the better route.

Changing the logic in userHasPermission from

if (
      checkProjects.length === 1 &&
      !checkProjects[0] === undefined &&
      Object.keys(userPermissions.projects).length
    ) 

to

if (
      checkProjects.length === 1 &&
      !checkProjects[0] &&
      Object.keys(userPermissions.projects).length
    ) 

This update makes it so it doesn't matter if the first index is an empty string or undefined,

Alternatively, we could update the signature of userHasPermission so projects can only be undefined or a string[].

In that case, for resources that can be in multiple projects (metrics, datasources, etc), we don't need the ternary. The only resources that would need to use the ternary are features, experiments, or ideas. In that case, we'd have to check for the existence of a project, and then either pass in an array with the project, or undefined.

This would make it so the engineer doesn't have to concern themselves with what they pass it. (Updating the logic so it just checks for !checkProjects[0] also removes that need, but an engineer still might wonder if they should pass in "" or [], even though it won't matter.

Digesting these options now...

[mknowlton89]

After consideration - for this PR, I think updating the check to simply check for !checkProjects[0] reduces the fragility.

I'd like to explore the larger fix that improves the developer experience, but would prefer to do so while there isn't a bug to ensure we test thoroughly.

[lukesonnet]

I think that change ("updating the check to simply check for !checkProjects[0]") makes sense to just do in this PR, with perhaps a bit more testing.

In the short-run, unless it breaks a bunch of permissions checks by making them more permissive when engineers built something and thought "" would block permission, I would say we make it to reduce the possibility of these kinds of bugs. In any case, seeing as this distinction is only for READ_ONLY_TYPE permissions, then the potential blast radius of incorrectly making "" pass more often seems small.

[mknowlton89]

This is an unrelated bug - that no one has ran into yet, but.... if a user has permission to runQueries, they can open the EditSqlModal which renders the SchemaBrowser.

And if the informationSchema is over 30 days old, we try to refresh it in the background, and we were checking for editDatasourceSettings, which is a permission that the user must either have globally (if the resource doesn't have any projects).

So if the user tried to edit a query on a datasource that was in "All Projects" and the user had a global role that doesn't permit editing a data source, the whole app would crash.

[lukesonnet]

Agreed with this change.

[lukesonnet]

Agreed with this change.

[lukesonnet]

I think that change ("updating the check to simply check for !checkProjects[0]") makes sense to just do in this PR, with perhaps a bit more testing.

In the short-run, unless it breaks a bunch of permissions checks by making them more permissive when engineers built something and thought "" would block permission, I would say we make it to reduce the possibility of these kinds of bugs. In any case, seeing as this distinction is only for READ_ONLY_TYPE permissions, then the potential blast radius of incorrectly making "" pass more often seems small.