custom-pages icon indicating copy to clipboard operation
custom-pages copied to clipboard

Images are public in private page in private space

Open Gilbertdelyon opened this issue 1 year ago • 6 comments

In a private space: Add an image in a post. This image is private (get image url, delete cache, if you enter the url in your browser you will get an error message). It works!

Now add a private custom page in this space. This custom page is "template" type. It embeds an image container (multiple images) These images are public. Disapointing !?

Is it the normal behaviour? Did I miss some setting?

Gilbertdelyon avatar Nov 11 '23 11:11 Gilbertdelyon

@luke- Yes, we have this issue because the checking is executed here https://github.com/humhub/humhub/blob/master/protected/humhub/modules/file/models/File.php#L291-L299:

public function canRead($userId = "")
{
    $object = $this->getPolymorphicRelation();
    if ($object instanceof ContentActiveRecord || $object instanceof ContentAddonActiveRecord) {
        return $object->content->canView($userId);
    }

    return true;
}

Image from custom template page is linked to humhub\modules\custom_pages\modules\template\models\ImageContent which is ActiveRecord and not ContentActiveRecord.

We could extend the core method File::canRead() with adding new code there like this:

if ($object instanceof ActiveRecord && method_exists($object, 'canView')) {
    return $object->canView();
}

and then implement the method canView for ActiveRecord models of the module "Custom Pages" where it is needed and possible. Do you agree this?

Maybe instead of the checking $object instanceof ActiveRecord && method_exists($object, 'canView') we should use some new interface with name like Viewable or you can suggest another name.

yurabakhtin avatar Nov 13 '23 11:11 yurabakhtin

@yurabakhtin With v1.16´ we introduced such interfaces. Can you please provide PRs [v1.16] ... for the custom pages Module? Can you please also create a PR for the mail module with the same issue here: https://github.com/humhub/mail/issues/320

luke- avatar Nov 20 '23 20:11 luke-

@luke- Core PR https://github.com/humhub/humhub/pull/6668 - main change there is the method humhub\modules\file\models\File::canRead():

if ($object instanceof ViewableInterface) {
    return $object->canView($user);
}

I see you already merged the PR, as you can see I have done there a small cleanup in the interfaces. Also I am a bit confised why we need to keep two interfaces ReadableInterface and ViewableInterface, I think one of them can be deleted, in most cases we have method canView, so the interface ReadableInterface can be deleted, do you agree? I find the ReadableInterface is used only for ContentAddonActiveRecord, but we can modify it to use ViewableInterface and canView.


  • Custom Pages - https://github.com/humhub/custom-pages/pull/311
  • Mail - https://github.com/humhub/mail/pull/363

yurabakhtin avatar Nov 21 '23 15:11 yurabakhtin

I see you already merged the PR, as you can see I have done there a small cleanup in the interfaces. Also I am a bit confised why we need to keep two interfaces ReadableInterface and ViewableInterface, I think one of them can be deleted, in most cases we have method canView, so the interface ReadableInterface can be deleted, do you agree? I find the ReadableInterface is used only for ContentAddonActiveRecord, but we can modify it to use ViewableInterface and canView.

Could not agree more. See

  • https://github.com/humhub/humhub/issues/6669#issuecomment-1821205874

martin-rueegg avatar Nov 21 '23 16:11 martin-rueegg

I find the ReadableInterface is used only for ContentAddonActiveRecord, but we can modify it to use ViewableInterface and canView.

Sounds good. Can you please create a PR to align this?

luke- avatar Nov 21 '23 19:11 luke-

I find the ReadableInterface is used only for ContentAddonActiveRecord, but we can modify it to use ViewableInterface and canView.

Sounds good. Can you please create a PR to align this?

@luke- PR https://github.com/humhub/humhub/pull/6671.

yurabakhtin avatar Nov 22 '23 07:11 yurabakhtin