flarum / framework

Simple forum software for building great communities.
http://flarum.org/
6.39k stars 834 forks source link

Restricted sub-tags having enabled "Start Discussion" button #2149

Closed PeopleInside closed 3 years ago

PeopleInside commented 4 years ago

Bug Report

Current Behavior and steps to reproduce. If a restric tag has a sub tag where user can't still posting the start discussion is showed, user are able to write title, message and when press to publish the red error indicate user has no permission.

I tested this on Beta 13 on my community. The main restrict tag is Annunci https://community.peopleinside.it/t/annunci here all works, user cannot start a discussion.

The sub tag is Aggiornamenti https://community.peopleinside.it/t/aggiornamenti here user can start a topic without be able to publish. also there the start button should be disabled.

Expected Behavior The start discussion should be disable also in sub tag where user are not allowed to post.

Environment

Output of "php flarum info", run this in terminal in your Flarum directory.

Possible Solution

Additional Context Add any other context about the problem here.

PeopleInside commented 4 years ago

This issue is about the recent pull request https://github.com/flarum/tags/pull/74

PeopleInside commented 4 years ago

The issue "can be solved" by adding also the sub tag in the permission page and repeat permission from the main tag.

I suggest:

if the sub tag is under a main tag that doesn't allow the user to post, also on the sub tag, the start discussion button should be disabled.

askvortsov1 commented 4 years ago

Good catch! Something else that popped up during internal discussion is that if a user isn't logged in, the button should prompt them to log in (open the modal) As such, the current proposal is as follows:

How does this sound?

PeopleInside commented 4 years ago

Sound good @askvortsov1. At this time for fix the issue on my community i added also "Aggiornamenti" to the permission page but this can be avoided in future I hope :)

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum. In any case, thanks for taking an interest in this software and contributing by opening the issue in the first place!

askvortsov1 commented 4 years ago

Still want this

jammerxd commented 4 years ago

I disagree with making it so the button should be disabled.

Right now the button is enabled but when a user tries to "Post Discussion" the api/discussion returns a 403 and a message of "You do not have permission to do that." is displayed. I think this is incorrect behavior as the user or group has been assigned permission to start the discussion so the user should be allowed to start the discussion.

The sub-category is more specific than the primary category so the sub-category group permissions should override the parent category.

The scenario I need is to have a Parent Category "Products" then sub-categories of Products. But I don't want users posting in "Products", I want them to post in the appropriate sub-category. So I made it so only admins can post in "Products" and thus, the error message above when a user tries to post in the sub-category. But because the group verified members is explicitly allowed to post in the sub-category, they should be allowed to post in the sub-category.

Screenshots for reference: https://gyazo.com/370ff50ec5dd805984380db987145fec https://gyazo.com/9ce0912aaa6afadbd9104fd1e0bd6e01

Flarum Discussion Thread: https://discuss.flarum.org/d/24894-permissions-of-sub-primary-tags-and-primary-tags

PeopleInside commented 4 years ago

I disagree with making it so the button should be disabled.

Right now the button is enabled but when a user tries to "Post Discussion" the api/discussion returns a 403 and a message of "You do not have permission to do that." is displayed. I think this is incorrect behavior as the user or group has been assigned permission to start the discussion so the user should be allowed to start the discussion.

The sub-category is more specific than the primary category so the sub-category group permissions should override the parent category.

The scenario I need is to have a Parent Category "Products" then sub-categories of Products. But I don't want users posting in "Products", I want them to post in the appropriate sub-category. So I made it so only admins can post in "Products" and thus, the error message above when a user tries to post in the sub-category. But because the group verified members is explicitly allowed to post in the sub-category, they should be allowed to post in the sub-category.

Screenshots for reference: https://gyazo.com/370ff50ec5dd805984380db987145fec https://gyazo.com/9ce0912aaa6afadbd9104fd1e0bd6e01

Flarum Discussion Thread: https://discuss.flarum.org/d/24894-permissions-of-sub-primary-tags-and-primary-tags

Umh... if sub-category doesn't take permission from main category then what happen if you have a private tag that is allowed only to administrator and sub-tags should be also private?

Currently I have a private tag and all sub-tags are also private, this is good. If the logic you are talking is applied than also all sub-category should be threaded as the main category so every one need a single permission.

Maybe there exist a way in the permission system to decide if sub category should take permission from the main category or are treated as the single main tag where you can customize permission.

I don't know if I'm clear.

clarkwinkelmann commented 4 years ago

This issue is about the frontend not reflecting how the backend works. It's not about changing how the backend works.

Another issue should be created to discuss an alternate tag-scoped-permission strategy. It's important to note that the current approach is unaware of the tag hierarchy. All tags are equal from the permission perspective, and the most restrictive always prevails. That's how it has worked for a long time in Flarum. We need to discuss this in detail if we ever want to change it, including the impact on existing forums and how to migrate them to the new system. It would be no small task.

PeopleInside commented 4 years ago

This issue is about the frontend not reflecting how the backend works. It's not about changing how the backend works.

Another issue should be created to discuss an alternate tag-scoped-permission strategy. It's important to note that the current approach is unaware of the tag hierarchy. All tags are equal from the permission perspective, and the most restrictive always prevails. That's how it has worked for a long time in Flarum. We need to discuss this in detail if we ever want to change it, including the impact on existing forums and how to migrate them to the new system. It would be no small task.

I agree how this is working so the expected situation here is the start discussion is disabled. Howevr I can understand the situation suggested by @jammerxd, maybe an extension can help for more complex functions.

clarkwinkelmann commented 4 years ago

maybe an extension can help for more complex functions

Yes definitely.

That's the whole reason Tags is an extension after all. You can fork it, and make it work differently. Or another extension can modify some of its behavior without replacing it completely. Flarum does not force users to use the provided tag system. The bundled Tags extension is designed to be a (relatively) simple extension that meets most forum's needs.

jammerxd commented 4 years ago

@clarkwinkelmann can a tags extension be made to change the behavior to do the scenario I described above? Is the permission denied coming from the tags extension or the discussion extension and where should I look in the tags to modify to make the behavior I need?

clarkwinkelmann commented 4 years ago

This is the code responsible for the tag-scoped permissions https://github.com/flarum/tags/blob/v0.1.0-beta.13/src/Access/DiscussionPolicy.php#L46-L70

Changing the strategy can be done by un-registering that policy somehow and registering a different policy.

jammerxd commented 4 years ago

I don't think that's the code blocking the creation. The problem is actually in the tags extension in the SaveTagsToDatabase method.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum. In any case, thanks for taking an interest in this software and contributing by opening the issue in the first place!

askvortsov1 commented 3 years ago

This should be an easy fix: https://github.com/flarum/tags/blob/master/src/Access/TagPolicy.php#L25-L27 needs to be updated to also take the parent tag into account.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum. In any case, thanks for taking an interest in this software and contributing by opening the issue in the first place!

luceos commented 3 years ago

I can no longer reproduce this on demo.flarum.site. To me, the button is disabled when you are on a tag page for an unrestricted sub tag of a restricted parent tag.