Skip to content

fix(workflowengine): fix group not shown in Group membership check - #52048

Merged
blizzz merged 2 commits into
masterfrom
fix/noid/wfe-empty-group-in-check
May 5, 2025
Merged

fix(workflowengine): fix group not shown in Group membership check#52048
blizzz merged 2 commits into
masterfrom
fix/noid/wfe-empty-group-in-check

Conversation

@blizzz

@blizzzblizzz commented Apr 8, 2025

Copy link
Copy Markdown
Member

Summary

this might have occured on instances with

  • more than twenty groups, and
  • on rules with more than one Group membership checks
  • and at least one of them being not in the set of the first 20 groups
beforeafter
beforeafter

In the before image you see that in some Group membership checks are still the placeholders and only two groups, G13 and G14 are shown with their display names. The network console shows that only the first twenty groups were fetched.

In the after image you see that all groups are visible now. The groups G33 and G37 are used twice each, on in combination with themselves and once an a rule with another group. In the network console you see that these groups are fetched now, but just once, not more than necessary.

Checklist

@blizzzblizzz added this to the Nextcloud 32 milestone Apr 8, 2025
@blizzz
blizzz requested a review from juliusknorrApril 8, 2025 16:36
@blizzz
blizzz requested a review from a team as a code ownerApril 8, 2025 16:36
@blizzz
blizzz requested review from nfebe, susnux and szaimen and removed request for a teamApril 8, 2025 16:36
@blizzzblizzz changed the title Fix/noid/wfe empty group in checkfix(workflowengine): fix group not shown in Group membership checkApr 8, 2025
@szaimen
szaimen removed their request for review April 9, 2025 14:01
Comment on lines +138 to +147
let nextQuery
do {
nextQuery = this.wantedGroups.shift()
if (this.hasGroup(nextQuery)) {
nextQuery = undefined
}
} while (!nextQuery && this.wantedGroups.length > 0)
if (nextQuery) {
await this.searchAsync(nextQuery)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Easier to read.

Suggested change
let nextQuery
do {
nextQuery = this.wantedGroups.shift()
if (this.hasGroup(nextQuery)) {
nextQuery = undefined
}
} while (!nextQuery && this.wantedGroups.length > 0)
if (nextQuery) {
await this.searchAsync(nextQuery)
}
while (this.wantedGroups.length > 0) {
const groupId = this.wantedGroups.shift()
if (this.hasGroup(groupId)) {
continue
}
await this.searchAsync(groupId)
return
}

Also, the current implementation would only search 1 wanted group, I assuming this intended as this.findGroupByQueue() again called at the end of search

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was intentionally done as a queue in order to not DoS the server with group search requests, but have them coming sequentially.

@susnuxsusnux left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works but see Fons comments

@blizzz

Copy link
Copy Markdown
MemberAuthor

/compile amend /

This was referenced May 5, 2025
this might have occured on instances with
- more than twenty groups, and
- on rules with more than one Group membership checks
- and at least one of them being not in the set of the first 20 groups
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@nextcloud-command
nextcloud-commandforce-pushed the fix/noid/wfe-empty-group-in-check branch from f01afb0 to 9609606CompareMay 5, 2025 14:59
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
blizzzforce-pushed the fix/noid/wfe-empty-group-in-check branch from 9609606 to c051a5bCompareMay 5, 2025 15:01
@blizzz

Copy link
Copy Markdown
MemberAuthor

(Rebased and rebuild)

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@blizzz@susnux@nfebe@skjnldsv