Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ocis vs ldap. 500 error when adding multiple users to a group #6170

Closed
ScharfViktor opened this issue Apr 27, 2023 · 4 comments · Fixed by #6214
Closed

Ocis vs ldap. 500 error when adding multiple users to a group #6170

ScharfViktor opened this issue Apr 27, 2023 · 4 comments · Fixed by #6214
Assignees
@rhafer
Labels
Priority:p1-urgent Consider a hotfix release with only that fix Type:Bug

Comments

@ScharfViktor
Copy link
Contributor

#6105 (comment)

ocis against ldap using https://github.com/owncloud/ocis/blob/master/deployments/examples/ocis_ldap/docker-compose.yml

  • open user manager
  • using batch action add users to one or more group

Actual: only first user added. 500 error
log:

{"level":"info","service":"proxy","proto":"HTTP/1.1","request-id":"8274a111-4d0c-4736-bd8e-f31cefea247e","remote-addr":"172.29.0.1","method":"POST","status":204,"path":"/graph/v1.0/groups/be2bdfad-348e-431a-b016-29362024d748/members/$ref","duration":23.627458,"bytes":0,"time":"2023-04-27T12:29:27.500430042Z","line":"github.com/owncloud/ocis/v2/services/proxy/pkg/middleware/accesslog.go:28","message":"access-log"}

{"level":"info","service":"graph","request-id":"0484eedc-a675-46f8-8b80-940d94d5651c","error":"LDAP Result Code 16 \"No Such Attribute\": modify/delete: member: no such value","time":"2023-04-27T12:29:27.500694958Z","line":"github.com/owncloud/ocis/v2/services/graph/pkg/identity/ldap_group.go:335","message":"Failed to modify group member entries on PATCH group"}

{"level":"info","service":"graph","request-id":"cc2771a2-ec62-41fe-be2f-6c6039d851de","error":"LDAP Result Code 16 \"No Such Attribute\": modify/delete: member: no such value","time":"2023-04-27T12:29:27.500871875Z","line":"github.com/owncloud/ocis/v2/services/graph/pkg/identity/ldap_group.go:335","message":"Failed to modify group member entries on PATCH group"}

{"level":"info","service":"proxy","proto":"HTTP/1.1","request-id":"cc2771a2-ec62-41fe-be2f-6c6039d851de","remote-addr":"172.29.0.1","method":"POST","status":500,"path":"/graph/v1.0/groups/be2bdfad-348e-431a-b016-29362024d748/members/$ref","duration":24.471458,"bytes":204,"time":"2023-04-27T12:29:27.501266958Z","line":"github.com/owncloud/ocis/v2/services/proxy/pkg/middleware/accesslog.go:28","message":"access-log"}

{"level":"info","service":"proxy","proto":"HTTP/1.1","request-id":"0484eedc-a675-46f8-8b80-940d94d5651c","remote-addr":"172.29.0.1","method":"POST","status":500,"path":"/graph/v1.0/groups/be2bdfad-348e-431a-b016-29362024d748/members/$ref","duration":24.708583,"bytes":204,"time":"2023-04-27T12:29:27.501528833Z","line":"github.com/owncloud/ocis/v2/services/proxy/pkg/middleware/accesslog.go:28","message":"access-log"}

{"RemoteAddr":"","User":"","URL":"","Method":"","UserAgent":"","Time":"","App":"admin_audit","Message":"user 'ddc2004c-0977-11eb-9d3f-a793888cd0f8' added user 'be2bdfad-348e-431a-b016-29362024d748' was added to group '91b3705c-6df0-47bb-b98f-2b4eac51ff92'","Action":"group_member_added","CLI":false,"Level":1,"GroupID":"be2bdfad-348e-431a-b016-29362024d748","UserID":"91b3705c-6df0-47bb-b98f-2b4eac51ff92"}

cc @rhafer
@ScharfViktor ScharfViktor mentioned this issue Apr 27, 2023
Closed
89 tasks
@micbar micbar added the Priority:p2-high Escalation, on top of current planning, release blocker label May 2, 2023
@micbar micbar moved this from Qualification to Prio 2 in Infinite Scale Team Board May 2, 2023
@micbar
Copy link
Contributor

micbar commented May 2, 2023

@rhafer known issue?

@rhafer rhafer self-assigned this May 2, 2023
@rhafer
Copy link
Contributor

rhafer commented May 2, 2023

No. I'll take a look

@rhafer
Copy link
Contributor

rhafer commented May 2, 2023 •
edited
Loading

Hm, there is a race that happens when multiple request to delete groupmember (or to add groupmembers) are in flight in parallel. It happens when either the first group member is added, or when the last one is removed.

Not sure yet how to fix it, still investigating.

@rhafer rhafer moved this from Prio 2 to Prio 1 in Infinite Scale Team Board May 3, 2023
@rhafer rhafer added Priority:p1-urgent Consider a hotfix release with only that fix and removed Priority:p2-high Escalation, on top of current planning, release blocker labels May 3, 2023
rhafer added a commit to rhafer/ocis that referenced this issue May 3, 2023
@rhafer
graph-ldap: Fix possible races when editing group membership in parallel
8862a98 
As the standard LDAP groups (groupOfNames) require at least one "member"
value to be present in a group, we have workarounds in place that add an
empty member ("") when creating a new group or when removing the last
member from the group. This can cause a race condition when e.g. multiple
request to remove members from a group an running in parallel, as we need
to read the group before we can construct the modification request. If
some other request modified the group (e.g. deleted the 2nd last member)
after we read it, we create non-working modification request.

These changes try to catch those errors and retry the modification
request once.

Fixes: owncloud#6170
rhafer added a commit to rhafer/ocis that referenced this issue May 3, 2023
@rhafer
graph-ldap: Fix possible races when editing group membership in parallel
3e071c9 
As the standard LDAP groups (groupOfNames) require at least one "member"
value to be present in a group, we have workarounds in place that add an
empty member ("") when creating a new group or when removing the last
member from the group. This can cause a race condition when e.g. multiple
request to remove members from a group an running in parallel, as we need
to read the group before we can construct the modification request. If
some other request modified the group (e.g. deleted the 2nd last member)
after we read it, we create non-working modification request.

These changes try to catch those errors and retry the modification
request once.

Fixes: owncloud#6170
@rhafer rhafer mentioned this issue May 3, 2023
Merged
@rhafer
Copy link
Contributor

rhafer commented May 3, 2023

#6214 should fix the problem. In the long run however it might be better if we switch our groups to use the groupOfEntries objectClass for setups where we have control over the used LDAP schema (i.e. where the graph backend in writeable). I'll create a separate issue for that with some background.

@rhafer rhafer mentioned this issue May 3, 2023
Open
@rhafer rhafer moved this from Prio 1 to In progress in Infinite Scale Team Board May 3, 2023
butonic pushed a commit that referenced this issue May 3, 2023
@rhafer
graph-ldap: Fix possible races when editing group membership in paral…
f1dbe43 
…lel (#6214)

As the standard LDAP groups (groupOfNames) require at least one "member"
value to be present in a group, we have workarounds in place that add an
empty member ("") when creating a new group or when removing the last
member from the group. This can cause a race condition when e.g. multiple
request to remove members from a group an running in parallel, as we need
to read the group before we can construct the modification request. If
some other request modified the group (e.g. deleted the 2nd last member)
after we read it, we create non-working modification request.

These changes try to catch those errors and retry the modification
request once.

Fixes: #6170
@github-project-automation github-project-automation bot moved this from In progress to Done in Infinite Scale Team Board May 3, 2023
ownclouders pushed a commit that referenced this issue May 3, 2023
@rhafer
graph-ldap: Fix possible races when editing group membership in paral…
160db9c 
…lel (#6214)

As the standard LDAP groups (groupOfNames) require at least one "member"
value to be present in a group, we have workarounds in place that add an
empty member ("") when creating a new group or when removing the last
member from the group. This can cause a race condition when e.g. multiple
request to remove members from a group an running in parallel, as we need
to read the group before we can construct the modification request. If
some other request modified the group (e.g. deleted the 2nd last member)
after we read it, we create non-working modification request.

These changes try to catch those errors and retry the modification
request once.

Fixes: #6170
@ScharfViktor ScharfViktor mentioned this issue May 4, 2023
Closed
86 tasks
fschade pushed a commit that referenced this issue Jul 10, 2023
@rhafer @fschade
graph-ldap: Fix possible races when editing group membership in paral…
fd4a2ae 
…lel (#6214)

As the standard LDAP groups (groupOfNames) require at least one "member"
value to be present in a group, we have workarounds in place that add an
empty member ("") when creating a new group or when removing the last
member from the group. This can cause a race condition when e.g. multiple
request to remove members from a group an running in parallel, as we need
to read the group before we can construct the modification request. If
some other request modified the group (e.g. deleted the 2nd last member)
after we read it, we create non-working modification request.

These changes try to catch those errors and retry the modification
request once.

Fixes: #6170
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rhafer rhafer

Labels
Priority:p1-urgent Consider a hotfix release with only that fix Type:Bug
Projects
Infinite Scale Team Board
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

graph-ldap: Fix possible races when editing group membership in parallel rhafer/ocis
3 participants
@rhafer @micbar @ScharfViktor