CA-252876: AD group name with parenthesis not work as expect in Xense… #3031

fillzero · 2017-05-09T10:17:49Z

…rver 7.0 pool

If the AD group name has parenthesis, the original code parses the wrong sid as below:

Group[1 of 2] name = NVC\testg(ab) (gid = 564135020, sid = S-1-5-21-1171552557-368733809-2946345504-1132)
=> Group[1 of 2] name = NVC\testg(ab| (gid = 564135020, sid = S-1-5-21-1171552557-368733809-2946345504-1132|
=> Group[1 of 2] name = NVC\testg(ab| (gid = 564135020, | S-1-5-21-1171552557-368733809-2946345504-1132|
=> | (gid = 564135020, |

lindig · 2017-05-09T13:46:25Z

I have some reservation about the general design how information is here extracted from program output by re-writing a string but I understand that it is not the role of this fix to improve it.
To convince us that the change is working, could you show output of subject_attrs before and after the change? The commit message includes the wrong output but not the correct one. If you don't want to include it in the commit message, please show it here.

fillzero · 2017-05-10T05:06:01Z

@lindig,

For example, https://github.com/fillzero/xen-api/blob/535d72b81cc878f9d19e1e78a12ec3b6c1e16eaa/ocaml/xapi/extauth_plugin_ADpbis.ml#L489
Before the change, log output:
Resolved 2 group sids for subject NVC\testd (S-1-5-21-1171552557-368733809-2946345504-1141): (gid = 564135020,,S-1-5-21-1171552557-368733809-2946345504-513

After the change, log output:
Resolved 2 group sids for subject NVC\testd (S-1-5-21-1171552557-368733809-2946345504-1141): S-1-5-21-1171552557-368733809-2946345504-1115,S-1-5-21-1171552557-368733809-2946345504-513

coveralls · 2017-05-10T06:41:58Z

Coverage increased (+0.01%) to 13.945% when pulling 535d72b on fillzero:CA-252876 into 9569f07 on xapi-project:master.

lindig

I'm fine with this for now but strongly believe this should be refactored to use regular expression-matching or scanf-matching to extract the desired identifiers from the string directly without rewriting the string. I'd like to see a comment from @jonludlam.

jonludlam · 2017-05-12T04:03:06Z

I'm not that bothered about using regexps, but I do want to see unit tests for this module. This PR definitely shouldn't be merged without a unit test for at least the function being changed (and the functions it in turn calls).

fillzero · 2017-05-19T12:21:03Z

@lindig @jonludlam
thanks a lot for your comments and suggestions.
I've updated accordingly.
Would you please help review?
Thanks.

mseri · 2017-05-19T12:30:11Z

ocaml/xapi/test_extauth_plugin_ADpbis.ml

+       ("", "Group[2 of 2] name = testdomain\\domain+users (gid = 580911617, sid = S-1-5-21-791009147-1041474540-2433379237-513)")],
+      ["S-1-5-21-791009147-1041474540-2433379237-1102"; "S-1-5-21-791009147-1041474540-2433379237-513"];
+    ]
+  end)


Can you add a test for a couple of malformed entries as well?

@mseri
Thanks for your comment.
Updated. Are you happy to review again?

Actually, can you add a test over [("blah", "1"); ("", "0 | 1 | 2 | 3")] as well?

Is it supposed to get '1' really? I would imagine that that should be considered broken input... and get [] or an error.

@mseri , here is the output using utop:

utop # extract_sid_from_group_list [("blah", "1"); ("", "0 | 1 | 2 | 3")];; - : bytes list = ["1"]

And log from make test

2017-05-19T14:57:55+00:00 607ae1f1e5f0#00 I: Test base_suite:0:test_extauth_ADpbis:1:test_pbis_extract_sid:1:[("blah", "1"); ("", "0 | 1 | 2 | 3")] -> ["1"] is successful.

Yes, that is exactly my point. It should not. You should extract the sid, and there is no sid in that string. Also I believe you would get something like:

(*Note: I did not test the following, I might be wrong *) extract_sid_from_group_list [("Number of groups found for user 'cnk3@UN'", "1"); ("", "Group[1 of 1] name = a|b|c|d\\UN\\KnmOJ (gid = 492513842, sid = S-1-5-31-5921451325-154521381-3135732118-4527)")];; ["b"];

as well, which would be clearly wrong.

coveralls · 2017-05-19T12:40:14Z

Coverage increased (+0.01%) to 13.999% when pulling 5ac126c on fillzero:CA-252876 into 3cfbf6d on xapi-project:master.

mseri

I am happy with it. What do you think @lindig @jonludlam ?

EDIT: can you add one more test?

coveralls · 2017-05-19T13:26:07Z

Coverage increased (+0.006%) to 13.993% when pulling 267c860 on fillzero:CA-252876 into 3cfbf6d on xapi-project:master.

fillzero · 2017-05-19T15:04:07Z

@mseri
Thanks for the suggestion.
Updated accordingly.

mseri · 2017-05-19T15:07:48Z

I am a bit concerned about the new test input result: it should not return ["1"] for that.
I think it's unlikely (maybe even impossible) that we get such input, but if I have a group name with such chars I would break the system:

[("Number of groups found for user 'cnk3@UN'", "1");
       ("", "Group[1 of 1] name = a|b|c|d\\UN\\KnmOJ (gid = 492513842, sid = S-1-5-31-5921451325-154521381-3135732118-4527)")],
      ["b"];

coveralls · 2017-05-19T15:21:07Z

Coverage increased (+0.006%) to 13.993% when pulling 79c8fb4 on fillzero:CA-252876 into 3cfbf6d on xapi-project:master.

fillzero · 2017-05-19T15:35:23Z

@mseri
https://msdn.microsoft.com/en-us/library/bb726984.aspx

Logon names can't contain certain characters. Invalid characters are
" / \ [ ] : ; | = , + * ? < >

So, it's impossible to have a group\username like a|b|c|d\UN\KnmOJ .

BTW, I will try to use regular expression-matching or scanf-matching suggested by @lindig and update this PR later.
Thanks.

mseri · 2017-05-19T15:42:05Z

@fillzero Ok, good. Thanks for clarifying!

Then you can remove that specific line from the test.
We can have the regexp approach in a separate PR for what concerns me. I am fine with this pull request.

mseri · 2017-05-19T16:07:27Z

It looks as if this PR supersedes #3034
I think we can close that PR and add

This also fixes CP-22274 Replace string_trim with built-in String.trim

to the commit message of the commit in this one.

…rver 7.0 pool This also fixes CP-22274 Replace string_trim with built-in String.trim Signed-off-by: Liang Dai <liang.dai1@citrix.com>

fillzero · 2017-05-20T01:00:22Z

@mseri
I removed that specific line and updated the message accordingly.
Could you please take a look?
Thanks.

coveralls · 2017-05-20T01:25:50Z

Coverage increased (+0.03%) to 14.006% when pulling b445af3 on fillzero:CA-252876 into 4691dca on xapi-project:master.

mseri · 2017-05-22T10:14:25Z

@jonludlam is it fine to merge?

jonludlam · 2017-05-22T14:36:54Z

Looks lovely now thanks! We've broken the 14% mark too :-)

lindig approved these changes May 10, 2017

View reviewed changes

thomassa mentioned this pull request May 16, 2017

CP-22274: Replace string_trim with built-in String.trim #3034

Closed

fillzero force-pushed the CA-252876 branch from 535d72b to 5ac126c Compare May 19, 2017 12:18

mseri reviewed May 19, 2017

View reviewed changes

fillzero force-pushed the CA-252876 branch from 5ac126c to 267c860 Compare May 19, 2017 13:04

mseri approved these changes May 19, 2017

View reviewed changes

fillzero force-pushed the CA-252876 branch from 267c860 to 79c8fb4 Compare May 19, 2017 15:00

CA-252876: AD group name with parenthesis not work as expect in XenSe…

b445af3

…rver 7.0 pool This also fixes CP-22274 Replace string_trim with built-in String.trim Signed-off-by: Liang Dai <liang.dai1@citrix.com>

fillzero force-pushed the CA-252876 branch from 79c8fb4 to b445af3 Compare May 20, 2017 00:56

jonludlam merged commit 4128788 into xapi-project:master May 22, 2017

CA-252876: AD group name with parenthesis not work as expect in Xense… #3031

CA-252876: AD group name with parenthesis not work as expect in Xense… #3031

Uh oh!

Conversation

fillzero commented May 9, 2017

Uh oh!

lindig commented May 9, 2017

Uh oh!

fillzero commented May 10, 2017

Uh oh!

coveralls commented May 10, 2017

Uh oh!

lindig left a comment

Choose a reason for hiding this comment

Uh oh!

jonludlam commented May 12, 2017

Uh oh!

fillzero commented May 19, 2017

Uh oh!

mseri May 19, 2017

Choose a reason for hiding this comment

Uh oh!

fillzero May 19, 2017

Choose a reason for hiding this comment

Uh oh!

mseri May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mseri May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fillzero May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mseri May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented May 19, 2017

Uh oh!

mseri left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented May 19, 2017

Uh oh!

fillzero commented May 19, 2017

Uh oh!

mseri commented May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented May 19, 2017

Uh oh!

fillzero commented May 19, 2017

Uh oh!

mseri commented May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mseri commented May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fillzero commented May 20, 2017

Uh oh!

coveralls commented May 20, 2017

Uh oh!

mseri commented May 22, 2017

Uh oh!

jonludlam commented May 22, 2017

Uh oh!

Uh oh!

mseri May 19, 2017 •

edited

Loading

mseri May 19, 2017 •

edited

Loading

fillzero May 19, 2017 •

edited

Loading

mseri May 19, 2017 •

edited

Loading

mseri left a comment •

edited

Loading

mseri commented May 19, 2017 •

edited

Loading

mseri commented May 19, 2017 •

edited

Loading

mseri commented May 19, 2017 •

edited

Loading