Issue 13675043: feed wellknown group membership for users (Closed)
|
Created:
12 years, 10 months ago by Tanmay Vartak Modified:
12 years, 10 months ago CC:
connector-cr_google.com Visibility:
Public. |
Descriptionfeed wellknown group membership for users
Patch Set 1 #
Total comments: 3
Patch Set 2 : Approach 1 with localized string support #
Total comments: 6
Patch Set 3 : with code review comments suggested by Eric #
Total comments: 4
MessagesTotal messages: 14
Sign in to reply to this message.
On 2013/09/12 03:55:47, Tanmay Vartak wrote: + PJ and Ondra, Even though I was not able to feed groups to GSA because of "Group source is invalid" exception, I verified that users are assigned to "Everyone, Authenticated Users@NT Authority, Interactive@NT Authority" groups from debug logs. Please note "Everyone" group, this does not have any domain (same as AD groups connector). Will that work with new GSA onboard DB? Please review this CL. Regards, Tanmay
Sign in to reply to this message.
Thank you. What was the group source name? The allowed pattern is "[a-zA-Z_][a-zA-Z0-9_-]*". Cheers, PJ On 2013/09/12 04:06:31, Tanmay Vartak wrote: > On 2013/09/12 03:55:47, Tanmay Vartak wrote: > > + PJ and Ondra, > > Even though I was not able to feed groups to GSA because of "Group source is > invalid" exception, I verified that users are assigned to "Everyone, > Authenticated Users@NT Authority, Interactive@NT Authority" groups from debug > logs. > > Please note "Everyone" group, this does not have any domain (same as AD groups > connector). Will that work with new GSA onboard DB? > > Please review this CL. > > Regards, > Tanmay
Sign in to reply to this message.
Hi, I would propose to structure this differently - the onboard database is size constrained, so we should not create a three groups containing all users and waste the space. Instead it should be done like this Everyone should contain all users "NT AUTHORITY\Authenticated users" should have Everyone as only member "NT AUTHORITY\Interactive" should have Everyone as only member This might be intended, but I want to point out, that this code handles only well known groups not builtin groups. Ondra https://codereview.appspot.com/13675043/diff/1/src/com/google/enterprise/adap... File src/com/google/enterprise/adaptor/ad/AdAdaptor.java (right): https://codereview.appspot.com/13675043/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/ad/AdAdaptor.java:152: everyone = new AdEntity("S-1-1-0", "CN=Everyone"); The strings Everyone NT AUTHORITY Interactive Authenticated Users BUILTIN need to be user defined in the config file. We cannot hardcode the entity names as they are locale dependent (b/9873970) https://codereview.appspot.com/13675043/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/ad/AdAdaptor.java:291: + domain.get(entity), namespace); Can you correct this - I didn't notice it in the original CL - this should be domain.get(member). Thanks
Sign in to reply to this message.
On 2013/09/12 07:37:07, ondrejnovak wrote: > Hi, > > I would propose to structure this differently - the onboard database is size > constrained, so we should not create a three groups containing all users and > waste the space. > > Instead it should be done like this > > Everyone should contain all users > "NT AUTHORITY\Authenticated users" should have Everyone as only member > "NT AUTHORITY\Interactive" should have Everyone as only member > > This might be intended, but I want to point out, that this code handles only > well known groups not builtin groups. > > Ondra > > https://codereview.appspot.com/13675043/diff/1/src/com/google/enterprise/adap... > File src/com/google/enterprise/adaptor/ad/AdAdaptor.java (right): > > https://codereview.appspot.com/13675043/diff/1/src/com/google/enterprise/adap... > src/com/google/enterprise/adaptor/ad/AdAdaptor.java:152: everyone = new > AdEntity("S-1-1-0", "CN=Everyone"); > The strings > > Everyone > NT AUTHORITY > Interactive > Authenticated Users > BUILTIN > > need to be user defined in the config file. We cannot hardcode the entity names > as they are locale dependent (b/9873970) > > https://codereview.appspot.com/13675043/diff/1/src/com/google/enterprise/adap... > src/com/google/enterprise/adaptor/ad/AdAdaptor.java:291: + domain.get(entity), > namespace); > Can you correct this - I didn't notice it in the original CL - this should be > domain.get(member). > > Thanks Hi Ondra, Thanks for review. I will merge all well known groups under everyone. That was on things to do list but I wanted basic part working fine. This CL was targeted for wellknown groups similar to connector. We can also include support for BUILTIN groups. Is there anything specific that connector does for "BUILTIN" groups? Eric : We might need to revisit claims decoding in SP Adaptor for localized strings for Everyone and Authenticated users. Regards, Tanmay
Sign in to reply to this message.
Hi, you need to change the domain for BUILTIN groups and also have an option not to feed them - default should be not to feed BUILTIN groups (b/7417736). Especially for customers which use filesystem and SharePoint it would be beneficial if they can override the names in SP connector as well. Ondra https://codereview.appspot.com/13675043/diff/1/src/com/google/enterprise/adap... File src/com/google/enterprise/adaptor/ad/AdAdaptor.java (right): https://codereview.appspot.com/13675043/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/ad/AdAdaptor.java:194: domain.put(e, server.getnETBIOSName()); e.getSid().startsWith("S-1-5-32-") ? "BUILTIN" : server.getnETBIOSName()
Sign in to reply to this message.
Sign in to reply to this message.
On 2013/09/13 16:17:47, Tanmay Vartak wrote: This CL is specifically targeted to support well known groups \ entities. I can put a separate CL for BUILTIN. Approach 1: With this patch we will support only 1 version of well known identities. So this might not be sufficient to feed English as well as some other language principal names for well known groups as same feed. Approach 2: We always feed English well known groups and we allow users to configure additional well known group principals names to be used. I am working on this now. If required we can take this as a separate CL as an enhancement to this CL.
Sign in to reply to this message.
LGTM I am happy with only one version of well known identities. SP connector, FS connector and SP adaptor feed localized names. I'm not sure about FS adaptor, but it will probably feed localized names as well. So you will just need to rewrite your well known entities in the AD adaptor config file and be done with it. In case there will be environment with SharePoint in German local and FS in Russian locale you could configure two adgroups adaptors. All of the normal groups will get overwritten apart from the well known groups which will get appended. That's good enough for version one. Ondra https://codereview.appspot.com/13675043/diff/9001/src/com/google/enterprise/a... File src/com/google/enterprise/adaptor/ad/AdAdaptor.java (right): https://codereview.appspot.com/13675043/diff/9001/src/com/google/enterprise/a... src/com/google/enterprise/adaptor/ad/AdAdaptor.java:184: // memeber to "iteractive" and "authenticated users" groups. s/memeber/member
Sign in to reply to this message.
On 2013/09/13 16:36:14, ondrejnovak wrote: > SP connector, FS connector and SP adaptor feed localized names. I'm not sure > about FS adaptor, but it will probably feed localized names as well. SP Adaptor (today) will feed an English name in a few cases when claims-based authentication is enabled. Tanmay and I discussed figuring out the localized version and it shouldn't be hard, but did want to mention that.
Sign in to reply to this message.
https://codereview.appspot.com/13675043/diff/9001/src/com/google/enterprise/a... File src/com/google/enterprise/adaptor/ad/AdAdaptor.java (right): https://codereview.appspot.com/13675043/diff/9001/src/com/google/enterprise/a... src/com/google/enterprise/adaptor/ad/AdAdaptor.java:110: localizedStrings.putAll( This should be: localizedStrings = context.getConfig().getvaluesWithPrefix("ad.localized.")) (and better, would be to also use Collections.unmodifiableMap()) There is then also no point to initializing localizedStrings with new HashMap. Adaptors can currently be restarted, which means this configuration would stay after a restart as it is currently written. https://codereview.appspot.com/13675043/diff/9001/src/com/google/enterprise/a... src/com/google/enterprise/adaptor/ad/AdAdaptor.java:161: final Map<AdEntity, Set<String>> wellKnownMemebership; s/Memebership/Membership/ https://codereview.appspot.com/13675043/diff/9001/src/com/google/enterprise/a... src/com/google/enterprise/adaptor/ad/AdAdaptor.java:164: everyone = new AdEntity("S-1-1-0", Feel free to construct these three AdEntitys at their definition (outside of the constructor). You would need to move getLocalizedVersion() to AdAdaptor, or remove it like I suggest elsewhere, but it is preferred to construct things at their definition when possible. Actually, if you move these definitions to AdAdaptor, then GroupCatalog.add() will work with looping. https://codereview.appspot.com/13675043/diff/9001/src/com/google/enterprise/a... src/com/google/enterprise/adaptor/ad/AdAdaptor.java:210: String getLocalizedValue(String key, String defaultValue) { This doesn't seem necessary to me. Each of the strings you use will be in the config because you did addKey()s, so localizedStrings.get(key) is enough. That also means there is no reason to have static final defines for the English strings. https://codereview.appspot.com/13675043/diff/9001/src/com/google/enterprise/a... src/com/google/enterprise/adaptor/ad/AdAdaptor.java:303: members.putAll(wellKnownMemebership); Eww... So we do a modification in makeDefs... Maybe: Map<AdEntity, Set<String>> allMembers = new HashMap<AdEntity, Set<String>>(members); allMembers.putAll(wellKnownMemebership); And then allMembers instead of members below?
Sign in to reply to this message.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/13675043/diff/16001/src/com/google/enterprise/... File src/com/google/enterprise/adaptor/ad/AdAdaptor.java (right): https://codereview.appspot.com/13675043/diff/16001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/ad/AdAdaptor.java:17: import com.google.common.base.Strings; Unused import. https://codereview.appspot.com/13675043/diff/16001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/ad/AdAdaptor.java:165: public GroupCatalog() { Newline before constructor, please.
Sign in to reply to this message.
Message was sent while issue was closed.
LGTM. Thank you. https://codereview.appspot.com/13675043/diff/16001/src/com/google/enterprise/... File src/com/google/enterprise/adaptor/ad/AdAdaptor.java (right): https://codereview.appspot.com/13675043/diff/16001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/ad/AdAdaptor.java:172: // member to "Interactive" and "authenticated users" groups. does casing matter? Should authenticated users be Authenticated Users. https://codereview.appspot.com/13675043/diff/16001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/ad/AdAdaptor.java:339: * returns principal name for ADEntity object. if domain is available return s/if/If/
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
