You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@syncope.apache.org by Guido Wimmel <gu...@gmx.net> on 2015/05/12 17:20:17 UTC
Re: getAdminRolesFilter-query
Hi Francesco,
finally I had some time to investigate further and create a branch.
See https://issues.apache.org/jira/browse/SYNCOPE-667
Cheers,
Guido
> Gesendet: Freitag, 17. April 2015 um 11:06 Uhr
> Von: "Francesco Chicchiriccò" <il...@apache.org>
> An: dev@syncope.apache.org
> Betreff: Re: getAdminRolesFilter-query
>
> On 17/04/2015 10:41, Guido Wimmel wrote:
> > Hi Francesco,
> >
> > ok, thanks for the clarification. I can see if I can investigate further and provide a patch.
> >
> > I did run the integration tests but I do not think the admin roles filtering is covered too well.
> > It seems that SearchTestITCase performs the rest calls with the global admin user, who owns all entitlements
> > (so nothing is filtered out).
>
> It seems that there is some test coverage, but not in SearchTestITCase:
>
> https://github.com/apache/syncope/blob/1_2_X/core/src/test/java/org/apache/syncope/core/rest/AuthenticationTestITCase.java#L217
>
> https://github.com/apache/syncope/blob/1_2_X/core/src/test/java/org/apache/syncope/core/rest/AuthenticationTestITCase.java#L229
>
> Regards.
>
> >> Gesendet: Freitag, 17. April 2015 um 09:05 Uhr
> >> Von: "Francesco Chicchiriccò" <il...@apache.org>
> >> An: dev@syncope.apache.org
> >> Betreff: Re: getAdminRolesFilter-query
> >>
> >> On 16/04/2015 21:20, Guido Wimmel wrote:
> >>> Hi Francesco,
> >>>
> >>> I'm only talking about the part of the query generated by
> >>> SubjectSearchDAOImpl.getAdminRolesFilter().
> >>> The FIQL string is not used for generating this part, only type and
> >>> adminRoles.
> >>> The part I wrote about is only used when type==SubjectType.USER.
> >>>
> >>> I was considering if the original query
> >>>
> >>> SELECT syncopeUser_id AS subject_id
> >>> FROM Membership M1 WHERE syncopeRole_id IN
> >>> (SELECT syncopeRole_id FROM Membership M2
> >>> WHERE M2.syncopeUser_id=M1.syncopeUser_id AND
> >>> syncopeRole_id NOT IN
> >>> (SELECT id AS syncopeRole_id FROM
> >>> SyncopeRole WHERE id=x1 OR id=x2 OR ...)
> >>> )
> >>>
> >>> can be simplified to
> >>>
> >>> SELECT syncopeUser_id AS subject_id FROM
> >>> Membership M2 WHERE syncopeRole_id NOT IN
> >>> (SELECT id AS syncopeRole_id FROM
> >>> SyncopeRole WHERE id=x1 OR id=x2 OR ...)
> >>>
> >>>
> >>> If the original query works with all supported databases, the
> >>> simplified version should work as well.
> >> I might agree with you, but you need to provide a proof - e.g. a patch,
> >> and consider that the same getAdminRolesFilter() method is invoked when
> >> searching for groups (besides users) as well.
> >>
> >>> I changed the code accordingly and ran the normal build with HSQL, and
> >>> there are no errors.
> >>> But it seems that the use cases of getAdminRolesFilter() are not
> >>> covered too well, as the build also seems to run
> >>> successfully if I replace the query by "SELECT syncopeUser_id WHERE
> >>> 0=1" (i.e. just return nothing).
> >> Entitlements and then consequently real admin filters come into the game
> >> only when Spring Security is involved, e.g. not during unit tests, but
> >> only with integration tests.
> >>
> >> If you really want to check your modifications, you'll need to follow
> >> the steps in
> >>
> >> http://syncope.apache.org/building.html
> >>
> >> e.g.
> >>
> >> "mvn -PskipTests" from root then "mvn clean verify" from core/
> >> (this for checking with H2, for other RDBMS see [3] as said below).
> >>
> >> FYI, the test class with particular reference to search features is
> >> SearchTestITCase.
> >>
> >>> I can try and investigate further when I have time.
> >>> I only wanted to know first if somebody maybe still can reproduce why
> >>> the query with M1 and M2 was designed the way it is.
> >> Not really: I can only argue that it reached the current form as
> >> consequence of several incremental changes, with reference to RDBMS
> >> support and different subject (users, roles).
> >>
> >> This is why I believe it is definitely not the best option, but anyway
> >> working: if you are able to improve such query, while keeping all
> >> integration tests happy with all supported RDBMS, that would be great.
> >>
> >> Regards.
> >>
> >>> On 16.04.2015 11:52, Francesco Chicchiriccò wrote:
> >>>> On 16/04/2015 11:00, Guido Wimmel wrote:
> >>>>> Hi,
> >>>>>
> >>>>> minor detail, but I recently stubled upon the query generated by
> >>>>> SubjectSearchDAOImpl.getAdminRolesFilter(),
> >>>>> used by search() and count(). Maybe it could be simplified.
> >>>>>
> >>>>> It seems to determine all users which have roles not in adminRoles
> >>>>> (to be able to filter them out
> >>>>> by WHERE subject_id NOT IN ... ).
> >>>>>
> >>>>> The generated query looks like (for type==USER and adminRoles={1,2}):
> >>>>>
> >>>>> (SELECT syncopeUser_id AS subject_id
> >>>>> FROM Membership M1
> >>>>> WHERE syncopeRole_id IN
> >>>>> (SELECT syncopeRole_id
> >>>>> FROM Membership M2
> >>>>> WHERE M2.syncopeUser_id=M1.syncopeUser_id
> >>>>> AND syncopeRole_id NOT IN
> >>>>> (SELECT id AS syncopeRole_id
> >>>>> FROM SyncopeRole
> >>>>> WHERE id=1 OR id=2
> >>>>> )
> >>>>> )
> >>>>>
> >>>>>
> >>>>> I don't really understand why two (inter-dependent) subqueries on
> >>>>> membership are needed.
> >>>>> I don't see a difference to
> >>>>>
> >>>>> WHERE subject_id NOT IN (
> >>>>> SELECT syncopeUser_id AS subject_id FROM
> >>>>> Membership M2 WHERE syncopeRole_id NOT IN (
> >>>>> SELECT id AS
> >>>>> syncopeRole_id FROM SyncopeRole WHERE id=1 OR id=2
> >>>>> )
> >>>>> )
> >>>>>
> >>>>> ... but probably I'm overlooking something?
> >>>> Hi Guido,
> >>>> this query is generated by SubjectSearchDAOImpl [1] (1_2_X) /
> >>>> JPASubjectSearchDAO [2] (master) as part of translating the FIQL
> >>>> query string received via REST into a native SQL query that can work
> >>>> reliably on all supported RDBMS (MySQL, MariaDB, Oracle, SQL Server,
> >>>> PostgreSQL and H2).
> >>>>
> >>>> Naturally, things can always be improved: if you find a way to
> >>>> simplify the subquery above while keeping everything working an all
> >>>> supported RDBMS, this is more than welcome!
> >>>>
> >>>> You can find information about how to check Syncope against all
> >>>> supported RDBMS at [3]: if you'd like to propose a change but you
> >>>> only have chance to verify it on a subset (say H2 and MySQL), please
> >>>> create a separate feature branch (or a pull request on GitHub) and I
> >>>> can help you completing all verifications needed.
> >>>>
> >>>> Please let me know.
> >>>> Regards.
> >>>>
> >>>> [1]
> >>>> https://github.com/apache/syncope/blob/1_2_X/core/src/main/java/org/apache/syncope/core/persistence/dao/impl/SubjectSearchDAOImpl.java
> >>>> [2]
> >>>> https://github.com/apache/syncope/blob/master/core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPASubjectSearchDAO.java
> >>>> [3] http://syncope.apache.org/building.html#DBMSes
>
> --
> Francesco Chicchiriccò
>
> Tirasa - Open Source Excellence
> http://www.tirasa.net/
>
> Involved at The Apache Software Foundation:
> member, Syncope PMC chair, Cocoon PMC, Olingo PMC
> http://people.apache.org/~ilgrosso/
>
>
Re: getAdminRolesFilter-query
Posted by Francesco Chicchiriccò <il...@apache.org>.
On 12/05/2015 17:20, Guido Wimmel wrote:
> Hi Francesco,
>
> finally I had some time to investigate further and create a branch.
> See https://issues.apache.org/jira/browse/SYNCOPE-667
Cool: I've commented there.
Thanks for taking this further.
Regards.
>> Gesendet: Freitag, 17. April 2015 um 11:06 Uhr
>> Von: "Francesco Chicchiriccò" <il...@apache.org>
>> An: dev@syncope.apache.org
>> Betreff: Re: getAdminRolesFilter-query
>>
>> On 17/04/2015 10:41, Guido Wimmel wrote:
>>> Hi Francesco,
>>>
>>> ok, thanks for the clarification. I can see if I can investigate further and provide a patch.
>>>
>>> I did run the integration tests but I do not think the admin roles filtering is covered too well.
>>> It seems that SearchTestITCase performs the rest calls with the global admin user, who owns all entitlements
>>> (so nothing is filtered out).
>> It seems that there is some test coverage, but not in SearchTestITCase:
>>
>> https://github.com/apache/syncope/blob/1_2_X/core/src/test/java/org/apache/syncope/core/rest/AuthenticationTestITCase.java#L217
>>
>> https://github.com/apache/syncope/blob/1_2_X/core/src/test/java/org/apache/syncope/core/rest/AuthenticationTestITCase.java#L229
>>
>> Regards.
>>
>>>> Gesendet: Freitag, 17. April 2015 um 09:05 Uhr
>>>> Von: "Francesco Chicchiriccò" <il...@apache.org>
>>>> An: dev@syncope.apache.org
>>>> Betreff: Re: getAdminRolesFilter-query
>>>>
>>>> On 16/04/2015 21:20, Guido Wimmel wrote:
>>>>> Hi Francesco,
>>>>>
>>>>> I'm only talking about the part of the query generated by
>>>>> SubjectSearchDAOImpl.getAdminRolesFilter().
>>>>> The FIQL string is not used for generating this part, only type and
>>>>> adminRoles.
>>>>> The part I wrote about is only used when type==SubjectType.USER.
>>>>>
>>>>> I was considering if the original query
>>>>>
>>>>> SELECT syncopeUser_id AS subject_id
>>>>> FROM Membership M1 WHERE syncopeRole_id IN
>>>>> (SELECT syncopeRole_id FROM Membership M2
>>>>> WHERE M2.syncopeUser_id=M1.syncopeUser_id AND
>>>>> syncopeRole_id NOT IN
>>>>> (SELECT id AS syncopeRole_id FROM
>>>>> SyncopeRole WHERE id=x1 OR id=x2 OR ...)
>>>>> )
>>>>>
>>>>> can be simplified to
>>>>>
>>>>> SELECT syncopeUser_id AS subject_id FROM
>>>>> Membership M2 WHERE syncopeRole_id NOT IN
>>>>> (SELECT id AS syncopeRole_id FROM
>>>>> SyncopeRole WHERE id=x1 OR id=x2 OR ...)
>>>>>
>>>>>
>>>>> If the original query works with all supported databases, the
>>>>> simplified version should work as well.
>>>> I might agree with you, but you need to provide a proof - e.g. a patch,
>>>> and consider that the same getAdminRolesFilter() method is invoked when
>>>> searching for groups (besides users) as well.
>>>>
>>>>> I changed the code accordingly and ran the normal build with HSQL, and
>>>>> there are no errors.
>>>>> But it seems that the use cases of getAdminRolesFilter() are not
>>>>> covered too well, as the build also seems to run
>>>>> successfully if I replace the query by "SELECT syncopeUser_id WHERE
>>>>> 0=1" (i.e. just return nothing).
>>>> Entitlements and then consequently real admin filters come into the game
>>>> only when Spring Security is involved, e.g. not during unit tests, but
>>>> only with integration tests.
>>>>
>>>> If you really want to check your modifications, you'll need to follow
>>>> the steps in
>>>>
>>>> http://syncope.apache.org/building.html
>>>>
>>>> e.g.
>>>>
>>>> "mvn -PskipTests" from root then "mvn clean verify" from core/
>>>> (this for checking with H2, for other RDBMS see [3] as said below).
>>>>
>>>> FYI, the test class with particular reference to search features is
>>>> SearchTestITCase.
>>>>
>>>>> I can try and investigate further when I have time.
>>>>> I only wanted to know first if somebody maybe still can reproduce why
>>>>> the query with M1 and M2 was designed the way it is.
>>>> Not really: I can only argue that it reached the current form as
>>>> consequence of several incremental changes, with reference to RDBMS
>>>> support and different subject (users, roles).
>>>>
>>>> This is why I believe it is definitely not the best option, but anyway
>>>> working: if you are able to improve such query, while keeping all
>>>> integration tests happy with all supported RDBMS, that would be great.
>>>>
>>>> Regards.
>>>>
>>>>> On 16.04.2015 11:52, Francesco Chicchiriccò wrote:
>>>>>> On 16/04/2015 11:00, Guido Wimmel wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> minor detail, but I recently stubled upon the query generated by
>>>>>>> SubjectSearchDAOImpl.getAdminRolesFilter(),
>>>>>>> used by search() and count(). Maybe it could be simplified.
>>>>>>>
>>>>>>> It seems to determine all users which have roles not in adminRoles
>>>>>>> (to be able to filter them out
>>>>>>> by WHERE subject_id NOT IN ... ).
>>>>>>>
>>>>>>> The generated query looks like (for type==USER and adminRoles={1,2}):
>>>>>>>
>>>>>>> (SELECT syncopeUser_id AS subject_id
>>>>>>> FROM Membership M1
>>>>>>> WHERE syncopeRole_id IN
>>>>>>> (SELECT syncopeRole_id
>>>>>>> FROM Membership M2
>>>>>>> WHERE M2.syncopeUser_id=M1.syncopeUser_id
>>>>>>> AND syncopeRole_id NOT IN
>>>>>>> (SELECT id AS syncopeRole_id
>>>>>>> FROM SyncopeRole
>>>>>>> WHERE id=1 OR id=2
>>>>>>> )
>>>>>>> )
>>>>>>>
>>>>>>>
>>>>>>> I don't really understand why two (inter-dependent) subqueries on
>>>>>>> membership are needed.
>>>>>>> I don't see a difference to
>>>>>>>
>>>>>>> WHERE subject_id NOT IN (
>>>>>>> SELECT syncopeUser_id AS subject_id FROM
>>>>>>> Membership M2 WHERE syncopeRole_id NOT IN (
>>>>>>> SELECT id AS
>>>>>>> syncopeRole_id FROM SyncopeRole WHERE id=1 OR id=2
>>>>>>> )
>>>>>>> )
>>>>>>>
>>>>>>> ... but probably I'm overlooking something?
>>>>>> Hi Guido,
>>>>>> this query is generated by SubjectSearchDAOImpl [1] (1_2_X) /
>>>>>> JPASubjectSearchDAO [2] (master) as part of translating the FIQL
>>>>>> query string received via REST into a native SQL query that can work
>>>>>> reliably on all supported RDBMS (MySQL, MariaDB, Oracle, SQL Server,
>>>>>> PostgreSQL and H2).
>>>>>>
>>>>>> Naturally, things can always be improved: if you find a way to
>>>>>> simplify the subquery above while keeping everything working an all
>>>>>> supported RDBMS, this is more than welcome!
>>>>>>
>>>>>> You can find information about how to check Syncope against all
>>>>>> supported RDBMS at [3]: if you'd like to propose a change but you
>>>>>> only have chance to verify it on a subset (say H2 and MySQL), please
>>>>>> create a separate feature branch (or a pull request on GitHub) and I
>>>>>> can help you completing all verifications needed.
>>>>>>
>>>>>> Please let me know.
>>>>>> Regards.
>>>>>>
>>>>>> [1] https://github.com/apache/syncope/blob/1_2_X/core/src/main/java/org/apache/syncope/core/persistence/dao/impl/SubjectSearchDAOImpl.java
>>>>>> [2] https://github.com/apache/syncope/blob/master/core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPASubjectSearchDAO.java
>>>>>> [3] http://syncope.apache.org/building.html#DBMSes
--
Francesco Chicchiriccò
Tirasa - Open Source Excellence
http://www.tirasa.net/
Involved at The Apache Software Foundation:
member, Syncope PMC chair, Cocoon PMC, Olingo PMC
http://people.apache.org/~ilgrosso/