refactor LDAP driver in userprovider #2133
Conversation
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
|
This pull request introduces 1 alert when merging 7763348 into c40fcc8 - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging 5851c05 into 67f39be - view on LGTM.com new alerts:
|
rhafer
force-pushed
the
ldap-refactor
branch
7 times, most recently
from
18d2416
to
c4adc4c
Compare
|
@labkode Could you please take a look at this? |
Move some common code into helper functions. Also don't assume that the userid in reva matches the user's username. So add an additional LDAP lookup to get the username before looking up groupmembership.
The entryUUID attribute is generated by the LDAP server. By using this me can make sure that the the users get different IDs with every test. So that can avoid setting DELETE_USER_DATA_CMD to delete the USER data after each test in the long run. Also this seems to uncover some issue in the WebDAV code which still assumes that the userid always match the username.
Return a proper NOT_FOUND error instead of INTERNAL when user is not found.
GetUser() is for getting users by ID only. Also try to handle the "User does not exist" case with a more helpful error message.
GetUserByClaim is suppose to return exactly one user. Return a more helpful error if the filter matches multiple users.
| Status: status.NewInternal(ctx, err, "error getting user"), | ||
| res := &userpb.GetUserResponse{} | ||
| if _, ok := err.(errtypes.NotFound); ok { | ||
| res.Status = status.NewNotFound(ctx, "user not found") |
There was a problem hiding this comment.
Can you also add req.UserId to the error
| } | ||
|
|
||
| // Check if we got a Username | ||
| res, err := gatewayClient.GetUserByClaim(ctx, &userpb.GetUserByClaimRequest{ |
There was a problem hiding this comment.
These calls can be avoided by checking what is being specified in the template. If we know that the calls will always be to the /dav/files/$username/$path and the namespace template asks for username only, then we can return early. Also, these need to be cached. Going to the userprovider service for every storage operation would be costly.
There was a problem hiding this comment.
I don't understand why we want to support requests to both /dav/files/$username/$path and /dav/files/$uid/$path
| }) | ||
| HandleWebdavError(appctx.GetLogger(r.Context()), w, b, err) | ||
| } | ||
| r.URL.Path = newPath |
There was a problem hiding this comment.
Nice! But this shouldn't have worked previously. I'd recommend having integration tests for both scenarios files_namespace = "/users/{{.Id.OpaqueId}}" and files_namespace = "/users"
| @@ -405,7 +420,7 @@ func (m *manager) getFindFilter(query string) string { | |||
| return strings.ReplaceAll(m.c.FindFilter, "{{query}}", ldap.EscapeFilter(query)) | |||
| } | |||
|
|
|||
| func (m *manager) getGroupFilter(uid *userpb.UserId) string { | |||
| func (m *manager) getGroupFilter(uid interface{}) string { | |||
There was a problem hiding this comment.
Can we go back to uid being *userpb.UserId?
|
This has landed in edge a while ago. I am going to close this for now. (Trying to bring back the edge an master branch in sync with another PR) |
Move some common code into helper functions. Also don't assume that the
userid in reva matches the user's username. So add an additional LDAP
lookup to get the username before looking up groupmembership.