LPS-74827 For site role, it should be added into index field "groupRoleId" so that it can match with search query. #342
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…by the subsequent one
…namicQuery with services in different modules
…model bean in the future. This is because of dependencies to the taglib. Once changed in the subrepo, the model bean parameter will be completely removed
…istener would see users with twitter accounts, and trying to connect to twitter which is against the no network access rule in test. This was not a problem before, because scheduler is disabled in portal-impl integration tests, but enabled in modules integration tests. This is caused by 2dcb43a
… is staging groupId, we should save live groupId because local staging site is not real site and we can't assign users to staging Site on UI.
…leId" so that it can match with search query.
|
Pull request test invoked at http://test-1-17.liferay.com/job/test-portal-acceptance-pullrequest(master). |
The pull request was closed.The pull request was closed due to the following branch validation error: /opt/dev/projects/github/liferay-subrepo-tools/bin/git-lpush -b master -o 9e418395e030757fc80b564bf5726b3cd47f860a -s e0f778de1f4e7e62717868df1adab678d03a6d82 -r liferay-portal -v master: Verifying. error: Illegal change to a .gitrepo file. For more details, click here. |
Some tests FAILED.Build Time: 2 minutes 51 seconds 10 ms Base Branch:Branch Name: master 1 Failed Jobs:For more details click here.Failures unique to this pull:
For upstream results, click here. |
|
ci:reopen |
|
ci:retest |
|
Pull request test invoked at http://test-1-18.liferay.com/job/test-portal-acceptance-pullrequest(master). |
The pull request tester is still running.Please wait until you get the final report before running 'ci:retest'. See this link to check on the status of your test: However, the pull request was closed.The pull request was closed because the following critical batches had failed: For information as to why we automatically close out certain pull requests see this article. *This pull will no longer automatically close if this comment is available. If you believe this is a mistake please re-open this pull by entering the following command as a comment. ci:reopen Critical Failure Details:test-portal-acceptance-pullrequest-batch(master)/source-format-jdk8Job Results:0 Tests Passed.
|
|
ci:reopen |
|
ci:retest |
|
Pull request test invoked at http://test-1-16.liferay.com/job/test-portal-acceptance-pullrequest(master). |
Some tests FAILED.Build Time: 3 hours 26 minutes 15 seconds 627 ms Base Branch:Branch Name: master 8 Failed Jobs:
103 Successful Jobs:
For more details click here.Failures unique to this pull:
|
|
Hi @arboliveira Hope you are well. Since the issue came from EE customer, do you have a chance to review this? Regards, |
topolik
reviewed
Oct 16, 2017
| companyId, className, | ||
| ResourceConstants.SCOPE_GROUP_TEMPLATE, | ||
| String.valueOf(GroupConstants.DEFAULT_PARENT_GROUP_ID), | ||
| viewActionId)); |
There was a problem hiding this comment.
Hi guys,
I'm not sure this is correct, but I don't know search permission checking.
I think when we index the document, it's fine to add SCOPE_INDIVIDUAL roles. When we change permissions we can reindex just one doc.
But is it fine to index also ResourceConstants.SCOPE_GROUP_TEMPLATE roles? When we change assignment on that scope then we need to reindex everything?!
On the other hand, when you index these wide-in-scope permissions, why don't you add also SCOPE_GROUP and SCOPE_COMPANY roles and move whole permission checking inside search operation?
There was a problem hiding this comment.
Hi @topolik
Thanks for your check. Please refer to the below explanation:
1.Why index ResourceConstants.SCOPE_GROUP_TEMPLATE(or ) roles?
In search query, we use ({"terms" : {"groupRoleId" : [ "groupId-roleId"]}), and if the entry is only viewable for siteRole, own siteRole's user also can view the model.
2.Why don't I add also SCOPE_GROUP and SCOPE_COMPANY?
Please refer to https://github.com/yuhai/liferay-portal/blob/master/modules/apps/foundation/portal-search/portal-search/src/main/java/com/liferay/portal/search/internal/SearchPermissionCheckerImpl.java#L415-L427
When compose search query, if regular role owns view permission, the query({"terms" : {"groupRoleId" : [ "groupId-roleId"]}) won't add into query. However, we can't do siteRole (as regular role) in search query because siteRole needs to use with group together.
Please refer to the fix detailed explanation from hhuijser#3030 (comment)
Regards,
Hai
|
Just started reviewing :) |
Sent from
|
Reviewed, submitted as brianchandotcom#52535. Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
hey André,
the change seems okay from my perspective but you might need to take a look on it too. Also I think @topolik might be interested too.
cc: @yuhai @hhuijser