Browse Source

iam: address PR review comments for group management

- Fix XSS vulnerability in groups.templ: replace innerHTML string
  concatenation with DOM APIs (createElement/textContent) for rendering
  member and policy lists
- Use userGroups reverse index in embedded IAM ListGroupsForUser for
  O(1) lookup instead of iterating all groups
- Add buildUserGroupsIndex helper in standalone IAM handlers; use it
  in ListGroupsForUser and removeUserFromAllGroups for efficient lookup
- Add note about gRPC store load-modify-save race condition limitation
pull/8560/head
Chris Lu 2 days ago
parent
commit
07f6734e80
  1. 54
      weed/admin/view/app/groups.templ
  2. 2
      weed/admin/view/app/groups_templ.go
  3. 5
      weed/credential/grpc/grpc_group.go
  4. 38
      weed/iamapi/iamapi_group_handlers.go
  5. 15
      weed/s3api/s3api_embedded_iam.go

54
weed/admin/view/app/groups.templ

@ -267,29 +267,59 @@ templ Groups(data dash.GroupsPageData) {
if (!response.ok) throw new Error('Failed to fetch group');
const group = await response.json();
// Render members
let membersHtml = '<table class="table table-sm"><tbody>';
// Render members using DOM APIs to prevent XSS
const membersList = document.getElementById('membersList');
membersList.innerHTML = '';
const membersTable = document.createElement('table');
membersTable.className = 'table table-sm';
const membersTbody = document.createElement('tbody');
if (group.members && group.members.length > 0) {
for (const member of group.members) {
membersHtml += '<tr><td>' + member + '</td><td><button class="btn btn-sm btn-outline-danger" onclick="removeMember(\'' + member + '\')"><i class="fas fa-times"></i></button></td></tr>';
const tr = membersTbody.insertRow();
const td1 = tr.insertCell();
td1.textContent = member;
const td2 = tr.insertCell();
const btn = document.createElement('button');
btn.className = 'btn btn-sm btn-outline-danger';
btn.onclick = () => removeMember(member);
btn.innerHTML = '<i class="fas fa-times"></i>';
td2.appendChild(btn);
}
} else {
membersHtml += '<tr><td class="text-muted">No members</td></tr>';
const tr = membersTbody.insertRow();
const td = tr.insertCell();
td.className = 'text-muted';
td.textContent = 'No members';
}
membersHtml += '</tbody></table>';
document.getElementById('membersList').innerHTML = membersHtml;
membersTable.appendChild(membersTbody);
membersList.appendChild(membersTable);
// Render policies
let policiesHtml = '<table class="table table-sm"><tbody>';
// Render policies using DOM APIs to prevent XSS
const policiesList = document.getElementById('policiesList');
policiesList.innerHTML = '';
const policiesTable = document.createElement('table');
policiesTable.className = 'table table-sm';
const policiesTbody = document.createElement('tbody');
if (group.policy_names && group.policy_names.length > 0) {
for (const policy of group.policy_names) {
policiesHtml += '<tr><td>' + policy + '</td><td><button class="btn btn-sm btn-outline-danger" onclick="detachPolicy(\'' + policy + '\')"><i class="fas fa-times"></i></button></td></tr>';
const tr = policiesTbody.insertRow();
const td1 = tr.insertCell();
td1.textContent = policy;
const td2 = tr.insertCell();
const btn = document.createElement('button');
btn.className = 'btn btn-sm btn-outline-danger';
btn.onclick = () => detachPolicy(policy);
btn.innerHTML = '<i class="fas fa-times"></i>';
td2.appendChild(btn);
}
} else {
policiesHtml += '<tr><td class="text-muted">No policies attached</td></tr>';
const tr = policiesTbody.insertRow();
const td = tr.insertCell();
td.className = 'text-muted';
td.textContent = 'No policies attached';
}
policiesHtml += '</tbody></table>';
document.getElementById('policiesList').innerHTML = policiesHtml;
policiesTable.appendChild(policiesTbody);
policiesList.appendChild(policiesTable);
// Update status toggle
document.getElementById('groupEnabledSwitch').checked = (group.status === 'enabled');

2
weed/admin/view/app/groups_templ.go
File diff suppressed because it is too large
View File

5
weed/credential/grpc/grpc_group.go

@ -7,6 +7,11 @@ import (
"github.com/seaweedfs/seaweedfs/weed/pb/iam_pb"
)
// NOTE: The gRPC store uses a load-modify-save pattern for all operations,
// which is inherently subject to race conditions under concurrent access.
// This matches the existing pattern used for identities and policies.
// A future improvement would add dedicated gRPC RPCs for atomic group operations.
func (store *IamGrpcStore) CreateGroup(ctx context.Context, group *iam_pb.Group) error {
config, err := store.LoadConfiguration(ctx)
if err != nil {

38
weed/iamapi/iamapi_group_handlers.go

@ -228,21 +228,32 @@ func (iama *IamApiServer) ListGroupsForUser(s3cfg *iam_pb.S3ApiConfiguration, va
if !userFound {
return resp, &IamError{Code: iam.ErrCodeNoSuchEntityException, Error: fmt.Errorf("user %s does not exist", userName)}
}
for _, g := range s3cfg.Groups {
for _, m := range g.Members {
if m == userName {
name := g.Name
resp.ListGroupsForUserResult.Groups = append(resp.ListGroupsForUserResult.Groups, &iam.Group{GroupName: &name})
break
}
}
// Build reverse index for efficient lookup
userGroupsIndex := buildUserGroupsIndex(s3cfg)
for _, gName := range userGroupsIndex[userName] {
name := gName
resp.ListGroupsForUserResult.Groups = append(resp.ListGroupsForUserResult.Groups, &iam.Group{GroupName: &name})
}
return resp, nil
}
// removeUserFromAllGroups removes a user from all groups they belong to.
// Uses a reverse index for efficient lookup of which groups to modify.
func removeUserFromAllGroups(s3cfg *iam_pb.S3ApiConfiguration, userName string) {
userGroupsIndex := buildUserGroupsIndex(s3cfg)
groupNames, found := userGroupsIndex[userName]
if !found {
return
}
// Build a set for fast group name lookup
targetGroups := make(map[string]bool, len(groupNames))
for _, gn := range groupNames {
targetGroups[gn] = true
}
for _, g := range s3cfg.Groups {
if !targetGroups[g.Name] {
continue
}
for i, m := range g.Members {
if m == userName {
g.Members = append(g.Members[:i], g.Members[i+1:]...)
@ -276,6 +287,17 @@ func isPolicyAttachedToAnyGroup(s3cfg *iam_pb.S3ApiConfiguration, policyName str
return "", false
}
// buildUserGroupsIndex builds a reverse index mapping usernames to group names.
func buildUserGroupsIndex(s3cfg *iam_pb.S3ApiConfiguration) map[string][]string {
index := make(map[string][]string)
for _, g := range s3cfg.Groups {
for _, m := range g.Members {
index[m] = append(index[m], g.Name)
}
}
return index
}
// policyNameFromArn extracts policy name from ARN for standalone handlers.
func policyNameFromArn(policyArn string) string {
return strings.TrimPrefix(policyArn, policyArnPrefix)

15
weed/s3api/s3api_embedded_iam.go

@ -1657,14 +1657,13 @@ func (e *EmbeddedIamApi) ListGroupsForUser(s3cfg *iam_pb.S3ApiConfiguration, val
if !userFound {
return resp, &iamError{Code: iam.ErrCodeNoSuchEntityException, Error: fmt.Errorf("user %s does not exist", userName)}
}
for _, g := range s3cfg.Groups {
for _, m := range g.Members {
if m == userName {
name := g.Name
resp.ListGroupsForUserResult.Groups = append(resp.ListGroupsForUserResult.Groups, &iam.Group{GroupName: &name})
break
}
}
// Use the in-memory reverse index for O(1) lookup
e.iam.m.RLock()
groupNames := e.iam.userGroups[userName]
e.iam.m.RUnlock()
for _, gName := range groupNames {
name := gName
resp.ListGroupsForUserResult.Groups = append(resp.ListGroupsForUserResult.Groups, &iam.Group{GroupName: &name})
}
return resp, nil
}

Loading…
Cancel
Save