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.

Sign up for GitHub

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

Jump to bottom

[Bugfix] CheckPermissions.php #213

Merged
merged 3 commits into from
Sep 20, 2023
Merged

[Bugfix] CheckPermissions.php #213

merged 3 commits into from
Sep 20, 2023

Conversation

haraldwitt
Copy link
Contributor 

 * Get FeGroups that are allowed to view a file/folder (checks NOT full rootline)
 * Check from the given folder up to root, i. e. the reverse! rootline. 
 * First restriction matches.
 * 
 * This Bugfix should be sure because it's only called from:
 *   - Aspects/SolrFalAspect.php
 *   - Hooks/KeSearchFilesHook.php
 *
 * This Bugfix resolvees issues:
 *   - https://github.com/beechit/fal_securedownload/issues/161
 *   - https://github.com/beechit/fal_securedownload/issues/166

@haraldwitt
Update CheckPermissions.php
737c78c 
     * Get FeGroups that are allowed to view a file/folder (checks NOT full rootline)
     * Check from the given folder up to root, i. e. the reverse! rootline. 
     * First restriction matches.
     * 
     * This Bugfix should be sure because it's only called from:
     *   - Aspects/SolrFalAspect.php
     *   - Hooks/KeSearchFilesHook.php
     *
     * This Bugfix resolvees issues:
     *   - #161
     *   - #166
@FamousWolf FamousWolf self-assigned this Jul 17, 2023
FamousWolf
FamousWolf reviewed Jul 17, 2023
View reviewed changes
Classes/Security/CheckPermissions.php Outdated
* Check from the given folder up to root, i. e. the reverse! rootline.
* First restriction matches.
*
* This Bugfix should be sure because it's only called from:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the comments about the bug fix here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll do it

Classes/Security/CheckPermissions.php
*/
public function getFolderRootLine(FolderInterface $folder)
{
return array_reverse($this->getReverseFolderRootLine($folder));
Copy link
Contributor

Choose a reason for hiding this comment

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

This would mean a double array_reverse. It's better to do it the other way around. That way you'll only need one array_reverse. So move the code from getReverseFolderRootline to getFolderRootline, remove the array_reverse on the return and change getReverseFolderRootline to return array_reverse($this->getFolderRootline($folder));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, there's only this one array_reverse() in all the code. The method $this->getReverseFolderRootLine() builts the reverse array native.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's an array_reverse on line 258 at the end of getReverseFolderRootLine

Copy link
Contributor Author

@haraldwitt haraldwitt Jul 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Oops, in my patched version it was removed. Forgot it here. Sorry for that.
Thank you for your attention :-)
And should we rename the array $rootLine inside getReverseFolderRootline() to $reverseRootLine for clearness?

haraldwitt
haraldwitt commented Jul 18, 2023
View reviewed changes
Copy link
Contributor Author

@haraldwitt haraldwitt left a comment

Choose a reason for hiding this comment

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

Comments removed

@haraldwitt
Update CheckPermissions.php
e262535 
array_reverse() removed from return value of getReverseFolderRootline().
@haraldwitt
Copy link
Contributor Author

haraldwitt commented Sep 19, 2023 •
edited
Loading

What about merging this into 4.0.3 and 5.0.1?
Should I prepate a pull request for these tags?

@joey-bolts
Copy link
Collaborator

@FamousWolf

If this is okay by you can merge and release this.

@haraldwitt
I might just release it for ^5, since ^4 is only v11, and ^5 both supports v11 & v12.
This will encourage only using the last version.

@haraldwitt
Copy link
Contributor Author

I think That's ok for everyone.
I'm about to migrate some sites from v10 to v11. So I'll use my old Composer-Patch for fal_securedownload until migration is done. Then I'll update to fal_securedownload ^5.

@FamousWolf FamousWolf merged commit 24826cc into beechit:master Sep 20, 2023
@haraldwitt
Copy link
Contributor Author

Great. Many thanks.

@haraldwitt
Copy link
Contributor Author

Oh no.
I was so happy to have this merged into master. But before publishing V 5.0.2 the interface between fal_securedownload and sorfal was changed from signalSlotDispatcher to Events.
But the Event ApacheSolrForTypo3\Solrfal\Event\Indexing\AfterFileMetaDataHasBeenRetrievedEvent does only exists in solrfal 12.0. So if one need solrfal AND fal_securedownload he also needs Typo3 12.

So I have do downgrade to fal_securedownload 5.0.1 :-(

Or is it possible to re-activate the Class SolrFalAspect for compatibility reasons?

Thanks
Harald

@joey-bolts
Copy link
Collaborator

@haraldwitt

I might be able to look into this on friday, but not sure yet.
If you could provide a pull request which injects signalSlot for backward compatibility i will be willing to merge this asap.

@haraldwitt
Copy link
Contributor Author

haraldwitt commented Oct 11, 2023 •
edited
Loading

@joey-bolts
Thx for the quick answer. But meanwhile I think that it doesn't make sense anymore.
In Typo3 12 the signalSlotDispatcher was removed. So I'd would have to deal with the VersionUtility ... :-(
The signalSlotDispatcher was already deprecated in Typo3 10. So it's surprising, that solrfal works it (and ONLY with it) in Typo3 11.
So, as long as I use Typo3 11, I'll downgrade to fal_securedownload 5.0.1 an make my own patches in composer.json.

Thanks a lot and I'll be back, as soon as I'm using Typo3 12 :-)
Harald

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FamousWolf FamousWolf FamousWolf left review comments

Assignees

@FamousWolf FamousWolf

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@haraldwitt @joey-bolts @FamousWolf