Improves hasReadAccess to better handle difference between empty string and empty array #2076
Conversation
…mtpy string and empty array.
|
Your preview environment pr-2076-bttf has been deployed. Preview environment endpoints are available at: |
| // if the user doesn't have project specific roles, fallback to user's global role | ||
| if (!filter.projects.length) { | ||
| // if the user doesn't have project specific roles or resource doesn't have a project (project is an empty string), fallback to user's global role | ||
| if (!filter.projects.length || projects === "") { |
There was a problem hiding this comment.
Could we not accomplish the same by doing the following? That way we could keep projects as string | string[] | null | undefined, and avoid having to pass in an empty string everywhere.
| if (!filter.projects.length || projects === "") { | |
| if (!filter.projects.length || !projects) { |
There was a problem hiding this comment.
We're treating an empty string differently than we're treating an empty array, which is why we've gone the route of not allowing projects to be undefined.
If we were to introduce the change you're suggesting, we'd lose the ability to do this differentiation.
Some background:
Some resources (metrics, data sources, fact tables, SDK connections) use a projects array as sort of a filter. If empty, there's no filter (all projects). If specified, it's filtering down to only the listed projects.
Other resources (feature flags, experiments) use a singular project field. It can either be undefined or an empty string (not in a project) or set to a specific id.
So being able to differentiate whether it's an empty string or an empty array allows us to control the read access filtering more precisely.
There was a problem hiding this comment.
Could you double-check? An empty array and undefined are not equal in value so I still think what I suggested would work.
console.log(Boolean([])); // true
console.log(Boolean(undefined)) // false
console.log(Boolean('')) // false
There was a problem hiding this comment.
Sorry - I think I misunderstood your request. I initially thought you were asking me to replace both of the following instances
hasReadAccess(...., resource.projects || [])
AND
hasReadAccess(..., resource.project || "")
So that we never had to use the || [] or || "" syntax. In that case, we'd not be able to differentiate whether it was undefined because the projects array was undefined or the project string was undefined.
But, correct me if I'm wrong, you were only asking me to update the hasReadAccess signature so we didn't have to pass in an empty string?
In that case, I've pushed up this commit with those changes. Let me know if that's what you intended.
There was a problem hiding this comment.
Yep, my comment was just regarding the signature of hasReadAccess. Changes look good 👍🏾
Features and Changes
Per a conversation with Jeremy, we're updating the logic of
hasReadAccessand updating the signature to no longer accessnullorundefinedvalues.When evaluating read access, an empty
projectsarray is different from an emptyprojectstring.Empty array = "all projects" = everyone has read access, even with "noaccess" global role
Empty string = "no project" = read access determined by the global role
Some resources (metrics, data sources, fact tables, SDK connections) use a plural projects field as sort of a filter. If empty, there's no filter (all projects). If specified, it's filtering down to only the listed projects.
Other resources (feature flags, experiments) use a singular project field. It can either be empty (not in a project) or set to a specific id.
So thinking about a global "noaccess" user who only has access to Project A, it feels like they should be able to see unfiltered (empty array) resources. It also feels like they should NOT be able to access "not in project" resources (empty string).