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ò" <[email protected]> > An: [email protected] > 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ò" <[email protected]> > >> An: [email protected] > >> 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/ > >
