fix: removed user membership access filter based on current user memb…#29
Open
SeanBerrieHRI wants to merge 1 commit intomainfrom
Open
fix: removed user membership access filter based on current user memb…#29SeanBerrieHRI wants to merge 1 commit intomainfrom
SeanBerrieHRI wants to merge 1 commit intomainfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRemoves over-restrictive filtering of available memberships in EntitySettings so that all roles returned from the backend are shown in the role selection dropdown, and cleans up the now-unused helper. Sequence diagram for fetching memberships in EntitySettingssequenceDiagram
actor User
participant EntitySettingsComponent
participant BackendAPI
User->>EntitySettingsComponent: Open access management view
EntitySettingsComponent->>BackendAPI: GET entity, members, users, memberships
BackendAPI-->>EntitySettingsComponent: entity, members, users, memberships
EntitySettingsComponent->>EntitySettingsComponent: buildGraph(entity)
EntitySettingsComponent->>EntitySettingsComponent: sort members
EntitySettingsComponent->>EntitySettingsComponent: createUsers(users, members)
EntitySettingsComponent->>EntitySettingsComponent: set memberships = memberships.data
EntitySettingsComponent->>EntitySettingsComponent: set inviteForm.membershipUuid = first memberships entry
EntitySettingsComponent-->>User: Render dropdown with all memberships from backend
Class diagram for EntitySettings component membership handlingclassDiagram
class EntitySettingsComponent {
+any config
+Array~any~ members
+Array~any~ users
+Array~any~ memberships
+any inviteForm
+any status
+any subject
+any breadcrumbs
+fetchData() Promise~void~
+buildGraph(entityData any) void
+createUsers(usersData Array~any~, members Array~any~) Array~any~
+submitInvite() Promise~void~
}
class BackendAPI {
+getEntityData(uuid string) Promise~any~
+getMembers(uuid string) Promise~Array~any~~
+getUsers() Promise~Array~any~~
+getMemberships() Promise~Array~any~~
}
EntitySettingsComponent --> BackendAPI : uses
class Membership {
+string uuid
+string name
+Array~string~ allowedEntities
}
EntitySettingsComponent "*" --> "*" Membership : displays_all_from_backend
%% Removed behavior (for comparison of structure only)
class RemovedBehavior {
-createMemberships(memberships Array~any~) Array~any~
}
EntitySettingsComponent ..> RemovedBehavior : helper_removed
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- By removing the
createMembershipsfilter entirely,this.membershipsmay now include roles that aren’t valid for the current entity type; consider ensuring the backend already scopesmemberships.dataappropriately or reintroducing a more precise, entity-safe filter that still shows all valid roles. - With
inviteForm.membershipUuidnow defaulting to the first unfiltered membership, it might be safer to explicitly pick a sensible default role (e.g., by name or priority) rather than relying on array order, to avoid accidentally defaulting to an overly privileged membership.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- By removing the `createMemberships` filter entirely, `this.memberships` may now include roles that aren’t valid for the current entity type; consider ensuring the backend already scopes `memberships.data` appropriately or reintroducing a more precise, entity-safe filter that still shows all valid roles.
- With `inviteForm.membershipUuid` now defaulting to the first unfiltered membership, it might be safer to explicitly pick a sensible default role (e.g., by name or priority) rather than relying on array order, to avoid accidentally defaulting to an overly privileged membership.
## Individual Comments
### Comment 1
<location path="src/components/EntitySettings/index.vue" line_range="157" />
<code_context>
this.members = _.orderBy(members.data, ['user.firstName', 'user.lastName'], ['asc'])
this.users = this.createUsers(users.data, this.members)
- this.memberships = this.createMemberships(memberships.data)
+ this.memberships = memberships.data
this.inviteForm.membershipUuid = _.get(this.memberships, '0.uuid')
this.breadcrumbs = this.config.createBreadcrumbsWithSelf(meta.data.path, this.subject)
</code_context>
<issue_to_address>
**🚨 question (security):** Dropping the `allowedEntities` filter changes which memberships are available and may widen access scope.
`createMemberships` previously limited `this.memberships` to entries whose `allowedEntities` included `this.config.uuid`. Using `memberships.data` directly means the UI (and default `inviteForm.membershipUuid`) may now surface memberships for other entities. Unless the backend enforces the same entity constraint, this could allow invites with unintended memberships. If expanding scope is intentional, can you confirm the server is enforcing the necessary entity-level checks so the client-side filter is truly redundant?
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| this.members = _.orderBy(members.data, ['user.firstName', 'user.lastName'], ['asc']) | ||
| this.users = this.createUsers(users.data, this.members) | ||
| this.memberships = this.createMemberships(memberships.data) | ||
| this.memberships = memberships.data |
There was a problem hiding this comment.
🚨 question (security): Dropping the allowedEntities filter changes which memberships are available and may widen access scope.
createMemberships previously limited this.memberships to entries whose allowedEntities included this.config.uuid. Using memberships.data directly means the UI (and default inviteForm.membershipUuid) may now surface memberships for other entities. Unless the backend enforces the same entity constraint, this could allow invites with unintended memberships. If expanding scope is intentional, can you confirm the server is enforcing the necessary entity-level checks so the client-side filter is truly redundant?
hcvdwerf
requested changes
Apr 2, 2026
| @@ -184,9 +183,6 @@ export default defineComponent({ | |||
| fullName: `${u.firstName} ${u.lastName}`, | |||
| })), ['firstName', 'lastName'], ['asc']) | |||
| }, | |||
There was a problem hiding this comment.
This looks too broad. The current code filters memberships by allowedEntities, and the backend still returns that field with entity-specific values. With the local API, Data Provider is only allowed for Catalog, but this PR would also show it for Dataset, Distribution, and Data Service. So this removes a real guard rather than fixing the underlying issue. Can we confirm whether allowedEntities is still authoritative before merging this?
Suggestion:
this.memberships = memberships.data.filter((m) => !Array.isArray(m.allowedEntities) || m.allowedEntities.length === 0 || m.allowedEntities.includes(this.config.uuid) )
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Actual:
When managing access for a metadata entity (e.g., a Dataset), the role selection dropdown is being limited to only those roles already held by existing members.
For example, if the only person with access to a Dataset is an Owner, the user can only invite new users as an Owner. The User role, despite being a valid system role, is hidden because no current member holds it.
Expected:
When managing access for a metadata entity (e.g., a Dataset), the role selection dropdown should show all available memberships for selection.
Cause:
In the FairDataPoint-Client > src > components > EntitySettings > Index.vue:
The memberships list is filtered based on the memberships provided to user entities in the list.
fixes #
Removing the filter function allows for all the available Memberships to be shown, allowign you to assign any role to any user.
Summary by Sourcery
Bug Fixes: