> On Aug. 7, 2017, 8:21 p.m., Jared Stewart wrote:
> > geode-cq/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ExecuteCQ61.java
> > Lines 140 (patched)
> > <https://reviews.apache.org/r/61480/diff/1/?file=1791025#file1791025line141>
> >
> >     Can you clarify something for me?  The change looks like it's addding a 
> > check for CLUSTER:MANAGE:QUERY.  But from reading GEODE-3330, I thought we 
> > already checked for that and instead needed to add a check for 
> > DATA:READ:REGION.  But it looks like that might have already been happening 
> > on line 126 (122 of the original code).
> 
> Jinmei Liao wrote:
>     line 126 is the authorization for old AccessControl interface, it does 
> nothing for the new integrated security. it was there in case user is still 
> using the old security model.
>     
>     I was orignally confused as well. Turns out we are checking 
> DATA:READ:regionName already for both execute and executeWithInitialResult, 
> we are just adding the CLUSTER:MANAGE.QUERY check.
> 
> Jared Stewart wrote:
>     Would you mind pointing me to where the existing check happens for my own 
> edification?  Thanks!
> 
> Jinmei Liao wrote:
>     It's in line 184 processQuery, which eventually leads to BaseCommandQuery 
> line 90.
> 
> Jared Stewart wrote:
>     Good to know.  Thanks again.

My understanding was that DATA:READ:regionName was to be required only for 
executeWithInitialResult, not for execute. As it is now this change requires 
both permissions for both execute and executeWithInitialResult. Is this what we 
want?


- Ken


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61480/#review182317
-----------------------------------------------------------


On Aug. 8, 2017, 9:10 p.m., Jinmei Liao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61480/
> -----------------------------------------------------------
> 
> (Updated Aug. 8, 2017, 9:10 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
> Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> * cq.execute() and cq.executeWithInitialResult() all would still require 
> DATA:READ because it will send the result back to the client either initially 
> or later.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/test/java/org/apache/geode/security/SecurityTestUtil.java 
> 5d5c2148e2eba8054df305942e04f43ea69c0a79 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java
>  a455aff315cd26abd398630ff63d8b54b0d1d12b 
>   
> geode-cq/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/CloseCQ.java
>  6748f7d3193babfa668a7ff2846f974f0cdc1cbd 
>   
> geode-cq/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/ExecuteCQ61.java
>  77a608c7e65a2030c0037e9f327cf8c17e9313db 
>   
> geode-cq/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/StopCQ.java
>  a3d51edcc141391e9d818fc0ed7e514d3cb5d6d0 
>   
> geode-cq/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/CloseCQTest.java
>  PRE-CREATION 
>   
> geode-cq/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/ExecuteCQ61Test.java
>  PRE-CREATION 
>   
> geode-cq/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/StopCQTest.java
>  PRE-CREATION 
>   geode-cq/src/test/java/org/apache/geode/security/CQClientAuthDUnitTest.java 
> 89167258ddbc06315068c849255a8530faefe060 
>   
> geode-cq/src/test/java/org/apache/geode/security/CQPostProcessorDunitTest.java
>  45f45abd17cf4f90f96163ebe4f01e67b3b53633 
>   geode-cq/src/test/java/org/apache/geode/security/ClientCQAuthDUnitTest.java 
> PRE-CREATION 
>   
> geode-cq/src/test/java/org/apache/geode/security/ClientQueryAuthDUnitTest.java
>  cc5dde409c561522ae3739eeaee892079c509ae8 
>   
> geode-cq/src/test/java/org/apache/geode/test/dunit/rules/CQUnitTestRule.java 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/61480/diff/2/
> 
> 
> Testing
> -------
> 
> precheckin running
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Reply via email to