Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUGFIX: site list in "content" of left drawer is policy checked #3979

Closed
wants to merge 4 commits into from
Closed

BUGFIX: site list in "content" of left drawer is policy checked #3979

wants to merge 4 commits into from

Conversation

tdausner
Copy link
Contributor

@tdausner tdausner commented Dec 11, 2022

Using policies for NodeTreePrivilege (in Policy.yaml) did not reflect in the list of sites in left drawer "content" site list. The bugfix restricts user to only sees sites with NodeTreePrivilege access GRANTED.

Function buildSiteList is enriched by $context and $privilegeManager. For each site the node of path /sites/{root-node-name-of-site} is checked by $privilegeManager->isGranted() and put into site list on GRANTED.

Use case / Review instructions

$ ./flow security:listroles
+-----------------------------------------------+-----------------------+
| Id                                            | Label                 |
+-----------------------------------------------+-----------------------+
| Neos.Neos:LivePublisher                       | Live publisher        |
| Neos.Neos:Administrator                       | Neos Administrator    |
| Neos.Neos:RestrictedEditor                    | Restricted Editor     |
| Neos.Neos:Editor                              | Editor                |
| Neos.Neos:UserManager                         | Neos User Manager     |
| Neos.RedirectHandler.Ui:RedirectAdministrator | RedirectAdministrator |
| Neos.Setup:SetupUser                          | SetupUser             |
| First.Site:FirstEditor                        | First site Editor     |
| Second.Site:SecondEditor                      | Second site Editor    |
| Third.Site:ThirdEditor                        | Third site Editor     |
+-----------------------------------------------+-----------------------+

NodeTreePrivilege matcher in Policy.yaml is 'isDescendantNodeOf("/sites/first-site")' for First.Site and so on. Retrieve the node names by command:

$ ./flow site:list

 Name                 Node name                 Resources package               Status
----------------------------------------------------------------------------------------
 First                first-site                First.Site                      online
 Second               second-site               Second.Site                     online
 Third                third-site                Third.Site                      online

  • Add test user:
    $ ./flow user:create --roles Second.Site:SecondEditor TestUser testuser-password Test User

  • Log into NEOS as administrator
    image

  • Log into NEOS as TestUser
    image

Checked releases 5.0 and 7.0 source code. Also in theese releases regards to policies are missing. Looks like the bug is inherited.

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

… in the list of sites in left drawer "content" site list. Now a restricted user only sees sites with NodeTreePrivilege access GRANTED.

Function buildSiteList is enriched by $context and $privilegeManager. On each site the node of path `/sites/{root-node-name-of-site}` is checked by `$privilegeManager->isGranted()` and put into site list on GRANTED.
@tdausner
Copy link
Contributor Author

tdausner commented Dec 14, 2022

@markusguenther lost my repo so the original pr got closed.

Copy link
Contributor

@grebaldi grebaldi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tdausner,

thanks to your excellent review instructions I was able to reproduce the problem and to verify manually that your change solves it :)

Code looks good to me 👍

Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, by reading.
Just two tiny issues/questions

Neos.Neos/Classes/Controller/Backend/MenuHelper.php Outdated Show resolved Hide resolved
Neos.Neos/Classes/Controller/Backend/MenuHelper.php Outdated Show resolved Hide resolved
…ce; added / missing property type declarations added / PhpDoc @throws updated
@tdausner
Copy link
Contributor Author

  • '/sites' replaced by SiteService::SITES_ROOT_PATH
  • use ...\SiteService; added
  • missing property type declarations added / PhpDoc @throws updated

@kdambekalns
Copy link
Member

Thanks! I was wondering… should this be fixed in 7.3, as it's a bugfix? I think yes…

If so, the type declarations won't work with injection. 😬

@tdausner
Copy link
Contributor Author

Oooops... I'm so happy with Neos 8... didn't think about backwards compatibility.
Removed type declarations on @flow\Inject.
Tested on

  • Neos 7.3.10
  • Neos 8.2 development collection

Copy link
Member

@kdambekalns kdambekalns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine (by reading!)

@kdambekalns
Copy link
Member

@tdausner Could you open this against 7.3 then?

@tdausner
Copy link
Contributor Author

Obsolete by #4025

skurfuerst added a commit that referenced this pull request Mar 3, 2023
We need to re-activate this, see #3979
neos-bot pushed a commit to neos/neos that referenced this pull request Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants