Issue 101610043: b/15904132 Use SiteUser mapping to build ACLs when user is not available in member id mapping (Closed)
|
Created:
9 years, 10 months ago by Tanmay Vartak Modified:
9 years, 9 months ago CC:
connector-cr_google.com Visibility:
Public. |
DescriptionFix for b/15904132 Use SiteUser mapping to build ACLs when user is not available in member id mapping
Patch Set 1 #
Total comments: 13
Patch Set 2 : With code review comments implemented #
Total comments: 12
Patch Set 3 : Code review comments implemented #
MessagesTotal messages: 13
Thank you. In addition to comments below please chat with me to walk me through how the site user mapping updated fixes bug of ACLs at web level but not site level. https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java (right): https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:378: * held while waiting on I/O. i don't follow the comment. instead of which lock? https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:380: private final Object refreshSiteUserMappingLock = new Object(); why isn't this lock in SiteAdaptor? Shouldn't each SiteAdaptor have a lock since each SiteAdaptor has a mappings and mappings callable? https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1378: synchronized (refreshSiteUserMappingLock) { again, this lock seems like it is per SiteAdaptor https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1386: siteUserCache.invalidate(siteUrl); i need help understanding what is invalidated https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1388: return getSiteUserMapping(); why do we have to call again? We must have already just called, right? https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1978: newSiteUserMapping = refreshSiteUserMapping(siteUserMapping); why do we need to have newSiteUserMapping? Doesn't refresh update what getSiteUserMapping() will give? So why do we need the var?
Sign in to reply to this message.
On 2014/07/02 03:05:55, pjo wrote: > Thank you. > > In addition to comments below please chat with > me to walk me through how the site user mapping > updated fixes bug of ACLs at web level but not > site level. > > https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... > File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java > (right): > > https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:378: * held > while waiting on I/O. > i don't follow the comment. instead of which lock? > > https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:380: private > final Object refreshSiteUserMappingLock = new Object(); > why isn't this lock in SiteAdaptor? > > Shouldn't each SiteAdaptor have a lock since each > SiteAdaptor has a mappings and mappings callable? > > https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1378: > synchronized (refreshSiteUserMappingLock) { > again, this lock seems like it is per SiteAdaptor > > https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1386: > siteUserCache.invalidate(siteUrl); > i need help understanding what is invalidated > > https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1388: return > getSiteUserMapping(); > why do we have to call again? We must have already just called, right? > > https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1978: > newSiteUserMapping = refreshSiteUserMapping(siteUserMapping); > why do we need to have newSiteUserMapping? > Doesn't refresh update what getSiteUserMapping() > will give? So why do we need the var? +Eric Eric, thanks for taking some time to review this. Regards, Tanmay
Sign in to reply to this message.
https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java (right): https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:378: * held while waiting on I/O. On 2014/07/02 03:05:54, pjo wrote: > i don't follow the comment. instead of which lock? Versus any other lock (or 'this'). We know we want to do I/O while holding a lock, so we choose to have a lock specifically for the task. https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:380: private final Object refreshSiteUserMappingLock = new Object(); On 2014/07/02 03:05:54, pjo wrote: > why isn't this lock in SiteAdaptor? > > Shouldn't each SiteAdaptor have a lock since each > SiteAdaptor has a mappings and mappings callable? That seems true. It seems the same would apply to refreshMemberIdMappingLock. It was likely just an oversight originally. https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1386: siteUserCache.invalidate(siteUrl); On 2014/07/02 03:05:54, pjo wrote: > i need help understanding what is invalidated The invalidate() trashes the cache used by getSiteUserMapping() for the provided site. So when this call is made, we believe that 'mapping' is out-of-date because it lacked a user/group. We then do getSiteUserMapping() to see if it has changed since 'mapping' was retrieved. If it is still the same, we invalidate, which means the next time getSiteUserMapping() is called I/O will be done to get the new mappings. The reason for the lock, is to prevent a race between getSiteUserMapping() and the invalidate. We only want to invalidate 'mapping' once. https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1388: return getSiteUserMapping(); On 2014/07/02 03:05:54, pjo wrote: > why do we have to call again? We must have already just called, right? This is the call that actually blocks on the I/O to get the new mapping. The previous call returned the mapping we already had, so we don't want to use it. Since the invalidate is in-between, we are guaranteed to get a different MemberIdMapping this call. The invalidate() trashes the cache used by getSiteUserMapping(). https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1973: MemberIdMapping siteUserMapping = getSiteUserMapping(); This variable should really be outside of the for loop. We don't want to call getSiteUserMapping for every ACE. I think you could initialize it to null and do the same check for null that we do often here. https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1975: if (principal == null) { Could this be outside of the previous (principal == null) if? // check mapping if (principal == null) { // check newMapping } if (principal == null) { // check siteUserMapping } if (principal == null) { // check newSiteUserMapping } These ifs could all be nested, but then indentation gets outrageous. Really, we should check siteUserMapping before newMapping. Because in the case that a user really is only in siteUserMapping, if we check siteUserMapping before newMapping we are able to use the caches without trouble and not force any I/O. https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1978: newSiteUserMapping = refreshSiteUserMapping(siteUserMapping); On 2014/07/02 03:05:54, pjo wrote: > why do we need to have newSiteUserMapping? > Doesn't refresh update what getSiteUserMapping() > will give? So why do we need the var? This forces us to refreshSiteUserMapping at most once. We don't want to refresh over and over again.
Sign in to reply to this message.
On 2014/07/02 22:40:38, ejona wrote: > https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... > File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java > (right): > > https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:378: * held > while waiting on I/O. > On 2014/07/02 03:05:54, pjo wrote: > > i don't follow the comment. instead of which lock? > > Versus any other lock (or 'this'). We know we want to do I/O while holding a > lock, so we choose to have a lock specifically for the task. > > https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:380: private > final Object refreshSiteUserMappingLock = new Object(); > On 2014/07/02 03:05:54, pjo wrote: > > why isn't this lock in SiteAdaptor? > > > > Shouldn't each SiteAdaptor have a lock since each > > SiteAdaptor has a mappings and mappings callable? > > That seems true. It seems the same would apply to refreshMemberIdMappingLock. It > was likely just an oversight originally. > > https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1386: > siteUserCache.invalidate(siteUrl); > On 2014/07/02 03:05:54, pjo wrote: > > i need help understanding what is invalidated > > The invalidate() trashes the cache used by getSiteUserMapping() for the provided > site. > > So when this call is made, we believe that 'mapping' is out-of-date because it > lacked a user/group. We then do getSiteUserMapping() to see if it has changed > since 'mapping' was retrieved. If it is still the same, we invalidate, which > means the next time getSiteUserMapping() is called I/O will be done to get the > new mappings. > > The reason for the lock, is to prevent a race between getSiteUserMapping() and > the invalidate. We only want to invalidate 'mapping' once. > > https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1388: return > getSiteUserMapping(); > On 2014/07/02 03:05:54, pjo wrote: > > why do we have to call again? We must have already just called, right? > > This is the call that actually blocks on the I/O to get the new mapping. The > previous call returned the mapping we already had, so we don't want to use it. > Since the invalidate is in-between, we are guaranteed to get a different > MemberIdMapping this call. The invalidate() trashes the cache used by > getSiteUserMapping(). > > https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1973: > MemberIdMapping siteUserMapping = getSiteUserMapping(); > This variable should really be outside of the for loop. We don't want to call > getSiteUserMapping for every ACE. > > I think you could initialize it to null and do the same check for null that we > do often here. > > https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1975: if > (principal == null) { > Could this be outside of the previous (principal == null) if? > > // check mapping > if (principal == null) { > // check newMapping > } > if (principal == null) { > // check siteUserMapping > } > if (principal == null) { > // check newSiteUserMapping > } > > These ifs could all be nested, but then indentation gets outrageous. > > Really, we should check siteUserMapping before newMapping. Because in the case > that a user really is only in siteUserMapping, if we check siteUserMapping > before newMapping we are able to use the caches without trouble and not force > any I/O. > > https://codereview.appspot.com/101610043/diff/1/src/com/google/enterprise/ada... > src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1978: > newSiteUserMapping = refreshSiteUserMapping(siteUserMapping); > On 2014/07/02 03:05:54, pjo wrote: > > why do we need to have newSiteUserMapping? > > Doesn't refresh update what getSiteUserMapping() > > will give? So why do we need the var? > > This forces us to refreshSiteUserMapping at most once. We don't want to refresh > over and over again. Thanks for review. I will make required changes.
Sign in to reply to this message.
Looks great from my standpoint. https://codereview.appspot.com/101610043/diff/20001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java (right): https://codereview.appspot.com/101610043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1955: boolean memberIdMappingRefreshed = false; So there was a small point of checking both mappings (new and old). Since a new user could be added in the same time period one is removed, we could actually need _both_: old one for user removed and new one for user added. Granted, in that case the user was removed, so it isn't a big deal to drop them from the ACL, although then our WARNING logging is a false positive. https://codereview.appspot.com/101610043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1969: if (!memberIdMappingRefreshed) { Move this condition into the principal==null if? There is really no reason to do mapping.getPrincipal(id) again if you don't end up refreshing. Ditto for siteUserMapping.
Sign in to reply to this message.
Thank you. https://codereview.appspot.com/101610043/diff/20001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java (right): https://codereview.appspot.com/101610043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1382: MemberIdMapping maybeNewMapping = getSiteUserMapping(); So, we call getSiteUserMapping for every call to refreshSiteUserMapping. The thing that this synchronized is supposed to prevent is calling the pair of (invalidate, getSiteUserMapping) multiple times, yes? And the lock on refreshSiteUserMapping gives us update guarantees on maybeNewMapping, yes? https://codereview.appspot.com/101610043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1387: siteUserCache.invalidate(siteUrl); This invalidates one key, which is for this one SiteAdaptor instance, yes? https://codereview.appspot.com/101610043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1389: return getSiteUserMapping(); So this call is outside of synchronized. Say that there are 10 nearly simultenous call to refreshSiteUserMapping. 9 get blocked on synchronize, one goes through, that one that gets trhough calls invalidate. I wanna see if I get the following three cases right: (A) This getSiteUserMapping is done before any of the 9 proceed with invalidte (B) All 9 invalidate before this call happens (C) this getSiteUserMapping happens as another refreshSiteUserMapping thread is after getSiteUserMapping but before invalidate. (A) is fine because all the maybeNewMapping calls will get a new mapping and those calls will be fast because invalidate has been reset to valid. (B) is fine. Yes all 9 have also called invalidate, but calling invalidate is cheap and here we'll have 10 simulatenous calls to getSiteUserMapping, but only one will be from invalidated state and that's protected by the cache's locking. (C) So one thread is finished with synchronized, but this getSiteUserMapping hasn't been called yet. Another thread has entered synchornized block and called getSiteUserMapping there (to get the previous value), then this getSiteUserMapping happens, and then the synchronized block thread calls invalidate. So we could potentially still do 10 pairings of (invalidate, getSiteUserMapping), so I'm thinking now that this code isn't what we want. https://codereview.appspot.com/101610043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1971: memberIdMappingRefreshed = true; not sure why we want to keep these booleans, we don't have to, right? And calling getSiteUserMapping is cheap if we don't invalidate right before that, right? https://codereview.appspot.com/101610043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1990: principal = siteUserMapping.getPrincipal(id); kind of seems like refresh logic and getSiteUserMapping logic should be inside the cache object, instead of inside SiteAdaptor trying to wrap its use of the cache.
Sign in to reply to this message.
https://codereview.appspot.com/101610043/diff/20001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java (right): https://codereview.appspot.com/101610043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1382: MemberIdMapping maybeNewMapping = getSiteUserMapping(); On 2014/07/07 22:04:55, pjo wrote: > So, we call getSiteUserMapping for every call > to refreshSiteUserMapping. Yes. Note that it is cached, so we are just seeing "what is the current value in the cache." > The thing that this synchronized is supposed > to prevent is calling the pair of (invalidate, > getSiteUserMapping) multiple times, yes? Yes. Invalidate() is what we really care about, but whether to call invalidate depends on the current value of getSiteUserMapping(). > And the lock on refreshSiteUserMapping gives > us update guarantees on maybeNewMapping, yes? Yes. It provides a weak guarantee on maybeNewMapping being "current" until we call invalidate. (the cache itself could invalidate maybeNewMapping because it is old, but we aren't too concerned even if that happens) https://codereview.appspot.com/101610043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1387: siteUserCache.invalidate(siteUrl); On 2014/07/07 22:04:55, pjo wrote: > This invalidates one key, which is for this one > SiteAdaptor instance, yes? Yes. https://codereview.appspot.com/101610043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1389: return getSiteUserMapping(); On 2014/07/07 22:04:56, pjo wrote: > So this call is outside of synchronized. > > Say that there are 10 nearly simultenous call > to refreshSiteUserMapping. 9 get blocked on > synchronize, one goes through, that one that > gets trhough calls invalidate. And the invariants you want to ensure are 1) only one may call invalidate() and 2) all return a value different than the provided 'mapping'. > I wanna see if I get the following three cases > right: > (A) This getSiteUserMapping is done before any > of the 9 proceed with invalidte > (B) All 9 invalidate before this call happens > (C) this getSiteUserMapping happens as another > refreshSiteUserMapping thread is after > getSiteUserMapping but before invalidate. > > (A) is fine because all the maybeNewMapping calls > will get a new mapping and those calls will be fast > because invalidate has been reset to valid. The first call will block on getSiteUserMapping, the second call will block on the getSiteUserMapping in the synchronized block, and the other 8 will be waiting on the lock. The two getSiteUserMapping calls will return at almost the same time, and the other 8 will have getSiteUserMapping return immediately with mapping != maybeNewMapping. > (B) is fine. Yes all 9 have also called invalidate, > but calling invalidate is cheap and here we'll have > 10 simulatenous calls to getSiteUserMapping, but > only one will be from invalidated state and that's > protected by the cache's locking. No. Only one calls invalidate(). The rest will see a different value for maybeNewMapping and will return from within the synchronized block. invalidate() is the "expensive" operation, even though it completes quickly and causes no I/O, because it causes the next getSiteUserMapping to do I/O. getSiteUserMapping may be cheap if already cached. > (C) So one thread is finished with synchronized, but > this getSiteUserMapping hasn't been called yet. > Another thread has entered synchornized block and > called getSiteUserMapping there (to get the previous > value), No. The cache has been invalidated, so the call blocks until a new mapping is fetched. > then this getSiteUserMapping happens, Which note is highly likely to block, due to the I/O. > and > then the synchronized block thread calls invalidate. No. It would see a different value for mapping, because the 'mapping' provided to it was before the invalidate(). The is no way it could get that same one back from the cache. https://codereview.appspot.com/101610043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1971: memberIdMappingRefreshed = true; On 2014/07/07 22:04:56, pjo wrote: > not sure why we want to keep these booleans, > we don't have to, right? And calling getSiteUserMapping > is cheap if we don't invalidate right before that, right? Oh, I see, yeah that is possible. That is not very obvious (as if any of the rest of it was...). In general, we tried to touch the cache as little as possible as it represents I/O, just as a general "policy". It is actually possible for the cache to be invalidated by another thread after we invalidate it (because the member mapping changed _again_), which would cause us to block again. In the normal case, two consecutive calls to the cache may yield different results because the cache became stale between the two (which is reasonably likely to happen at some point). Although yes, that happening here is very unlikely because we just refreshed the cache. https://codereview.appspot.com/101610043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:1990: principal = siteUserMapping.getPrincipal(id); On 2014/07/07 22:04:55, pjo wrote: > kind of seems like refresh logic and > getSiteUserMapping logic should be inside > the cache object, instead of inside SiteAdaptor > trying to wrap its use of the cache. The problem is that the Cache is from Guava. It didn't seem like something that makes sense to place our own logic into, although I guess that would be possible. There would be a bit of a head-ache with managing the locks... hmmm... you probably shouldn't use 'mapping's lock... Maybe you do something that resembled double-checked locking with doing siteUserCache.get() once, if it is the same as mapping then entering synchronized on the result, call siteUserCache.get() again, check to see if it changed, and then invalidate if not.
Sign in to reply to this message.
On 2014/07/08 22:25:44, Tanmay Vartak wrote: I moved lookup in site user cache before refreshing memberid cache as per code review comment by Eric from patch 1.
Sign in to reply to this message.
That's what I was imagining. Still looks good from my standpoint.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
