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

Images are public in private page in private space #308

Open
Gilbertdelyon opened this issue Nov 11, 2023 · 6 comments · May be fixed by #311
Open

Images are public in private page in private space #308

Gilbertdelyon opened this issue Nov 11, 2023 · 6 comments · May be fixed by #311
Assignees

Comments

@Gilbertdelyon
Copy link

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?

@yurabakhtin
Copy link
Contributor

@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.

@luke-
Copy link
Contributor

luke- commented Nov 20, 2023

@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: humhub/mail#320

@yurabakhtin
Copy link
Contributor

@luke- Core PR humhub/humhub#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.


@martin-rueegg
Copy link

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

@luke-
Copy link
Contributor

luke- commented Nov 21, 2023

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?

@yurabakhtin
Copy link
Contributor

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 humhub/humhub#6671.

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 a pull request may close this issue.

4 participants