Forbid = in group names
#5972
Forbid = in group names
#5972
Conversation
|
💥 Acceptance test localApiTests-apiCors-ocis failed. Further test are cancelled... |
There was a problem hiding this comment.
I think this there is some problem with escaping here (which is likely not limited to the '=' charcter). It would be way better to correctly understand that problem before we merge this "fix".
(BTW, this might also be a problem with the libregraph/idm and it might just work when using a proper LDAP server)
|
@rhafer I don't think it is related to escaping. When renaming a group we use the method ModifyDN which is expecting a string consisting of key-value pair separated by "=" Updating with group e.g. "grp=" leads to a sting like I tried escaping "=" or quoting the names, both without success. But you are right maybe this does not happen with another ldap server. |
|
Uff. You're right. There seems to be a bug in ldap.ParseDN() which causes it to choke on DN attribute value ending with We should definetly create an upstream issue for this. But if we want to disallow names with |
kobergj
force-pushed
the
ForbidEqualForGroupNames
branch
from
4fe10da to
1aba274
Compare
Signed-off-by: jkoberg <[email protected]>
kobergj
force-pushed
the
ForbidEqualForGroupNames
branch
from
1aba274 to
c9da494
Compare
Signed-off-by: jkoberg <[email protected]>
kobergj
force-pushed
the
ForbidEqualForGroupNames
branch
from
c9da494 to
15ca806
Compare
|
Kudos, SonarCloud Quality Gate passed! |
|
@rhafer followed your suggestion. Please double check |
| @@ -424,3 +427,19 @@ func parsePurgeHeader(h http.Header) bool { | |||
| } | |||
| return false | |||
| } | |||
|
|
|||
| // ldap is using `=` signs as delimiter for key-values when doing requests | |||
| // underlying go library is having problems when trying to parse names (keys) that contain `=` | |||
There was a problem hiding this comment.
It would be nice if you'd mention the upstream issue here. I just found: go-ldap/ldap#416 (even though it's talking about base64 encoded values, the issue is really about the = sign).
There was a problem hiding this comment.
@kobergj Look was just fixed a couple of days ago: go-ldap/ldap#425
Funny coincidence. I wonder if we should just wait for next go-ldap release. But I am also fine merging this. Would be nice if you'd mention the upstream issue in the code tough.
|
Let's test with new version. If it works we can close this PR |
|
Superior fix here: #6004 |
Since underlying
ldaplibrary panics when you do so, this is the best (aka quickest) way to solve this