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

disable query_see_board when manage_boards is allowed fixes #5002 #5396

Closed
wants to merge 1 commit into
base: release-2.1
from

Conversation

Projects
None yet
6 participants
@albertlast
Copy link
Collaborator

albertlast commented Feb 8, 2019

issue: #5002

disable query_see_board when mange_boards is allowed
Signed-off-by: albertlast albertlast@hotmail.de
@jdarwood007

This comment has been minimized.

Copy link
Member

jdarwood007 commented Feb 9, 2019

Not sure I agree with that logic..

@albertlast

This comment has been minimized.

Copy link
Collaborator Author

albertlast commented Feb 9, 2019

Because?

@Sesquipedalian

This comment has been minimized.

Copy link
Member

Sesquipedalian commented Feb 9, 2019

Because it is too permissive. The goal is to allow members with the 'manage_boards' permission to see empty categories when they are on the Manage Boards page. This PR would allow anyone with the 'manage_boards' permission to see all the boards every time the board tree is retrieved.

@albertlast

This comment has been minimized.

Copy link
Collaborator Author

albertlast commented Feb 9, 2019

yes sound good to me,
alternative solution would be not to use the board tree to build to view of manage boards.

@Sesquipedalian

This comment has been minimized.

Copy link
Member

Sesquipedalian commented Feb 9, 2019

Perhaps we could pass a $show_empty_categories parameter to getBoardTree() when calling it from the ManageBoards.php functions, and handle the query differently when that parameter is true.

@albertlast

This comment has been minimized.

Copy link
Collaborator Author

albertlast commented Feb 19, 2019

but should he not able to managed all boards? the permission is called manage_boards
atm he is only able to mange his own boards
when we add add empty boards,
he lost function to managethem in this moment when the board is not empty any more...
sound a litte bit suprise to take the rights away in the middle of process.

@Sesquipedalian

This comment has been minimized.

Copy link
Member

Sesquipedalian commented Feb 19, 2019

Hm. In theory, the changes in 22aa767 and 68fa7bc were supposed to allow membergroups with the manage_boards permission to access all boards. In reality, we are being inconsistent about that.

Initially, granting the manage_boards permission to a membergroup changes nothing about which boards the membergroup is allowed to access. Only after the admin edits the membergroup will the group's board access permissions be changed.

In order to resolve this inconsistency, there are only two options that I can see:

  1. If we are going to say that groups with the the manage_boards permission can see all boards, then we should change build_query_board() so that $user_info['query_see_board'] is set to 1=1 not only if the user is an admin, but if the user has the manage_boards permission. Along with that change, we should remove this chunk of code:

    // If they can manage boards, the rules are a bit different. They can see everything.
    if ($context['can_manage_boards'])
    {
    $accesses = array();
    $request = $smcFunc['db_query']('', '
    SELECT id_board
    FROM {db_prefix}boards'
    );
    while ($row = $smcFunc['db_fetch_assoc']($request))
    $accesses[(int) $row['id_board']] = 'allow';
    $smcFunc['db_free_result']($request);
    }

  2. We revert 22aa767, 68fa7bc, and any other changes that tried to allow anyone with the manage_boards permission to access all boards.

I am inclined to think that the first option is better. It will implement consistently what @Arantor seems to have had in mind in 22aa767 and 68fa7bc and what everyone working with SMF 2.1 ever since then has come to expect. However, there may be aspects of all this that I am not aware of, so I'd appreciate input from others before any decisions are made.

@Arantor, is there anything I am missing and/or that you could shed light on regarding the changes you made to board access and the manage_boards permission back in 2014?

@jdarwood007, are there any security concerns that you can think of that I might have missed if we go with option 1 above?

@Sesquipedalian Sesquipedalian added this to the RC2 milestone Feb 19, 2019

@albertlast

This comment has been minimized.

Copy link
Collaborator Author

albertlast commented Feb 19, 2019

@Sesquipedalian i'm a little bit suprise that you want to release the rc2 in the near future and
you set this pr to rc2 with the knowledgt that the solution is unclear.

@Sesquipedalian

This comment has been minimized.

Copy link
Member

Sesquipedalian commented Feb 19, 2019

Well, both possible solutions are fairly straightforward to implement and test. If a general consensus emerges that one or the other is the best way to go, it won't be hard to get this done. But if the discussion reveals that the issue is more complicated than it appears, then we can always change the milestone to RC3.

Sent with GitHawk

@Arantor

This comment has been minimized.

Copy link
Contributor

Arantor commented Feb 20, 2019

The two commits were certainly intended to cover the UI issues, but I guess I forgot to change the logic around actually making sure those with manage boards (but not admin) could actually see all boards, on the theory that if they could edit all boards, giving themselves access is a triviality they could do at any time.

But the timing of the commits suggests it was in a time where anything other than minor UI tidy ups were... discouraged.

@albertlast

This comment has been minimized.

Copy link
Collaborator Author

albertlast commented Feb 20, 2019

@Sesquipedalian i don't get fully your ideas,
are you willing to create a pr for this? than i could close this.

@jdarwood007

This comment has been minimized.

Copy link
Member

jdarwood007 commented Feb 21, 2019

@Sesquipedalian Biggest concern is how this changes the logic that has been consistent since early days of SMF where manage boards did not grant full access to boards. Upgrades may cause users to gain access to boards they shouldn't if they had the manage boards permission.

@Sesquipedalian

This comment has been minimized.

Copy link
Member

Sesquipedalian commented Feb 21, 2019

Upgrades may cause users to gain access to boards they shouldn't if they had the manage boards permission.

...grumble grumble backwards compatibility with flawed logic grumble grumble...

@Sesquipedalian

This comment has been minimized.

Copy link
Member

Sesquipedalian commented Feb 21, 2019

See #5459

@Arantor

This comment has been minimized.

Copy link
Contributor

Arantor commented Feb 21, 2019

FWIW I disagree with this move. If someone can manage boards, they can just give themselves that access anyway, so what exactly are people defending against here?

Worse, if you assume that manage boards permission is no longer overriding board access, a world of pain becomes possible: boards that are visible inside boards that are hidden (this is a thing people do to hide from BoardIndex) that are now not manageable by such users.

Honestly, I’m not sure that this approach is the wrong one, as we observed with manage permissions being able to escalate to admin forum permissions, or manage groups getting a quasi workaround to prevent someone with non admin powers giving themselves admin powers.

Maybe it’s time to just fold all these into “admin forum” like other forum platforms do, because there’s a lot of potential for edge cases that really isn’t necessary.

@live627

This comment has been minimized.

Copy link
Contributor

live627 commented Feb 21, 2019

a quasi workaround to prevent someone with non admin powers giving themselves admin powers.

Is that why 2.0 RC4 introduced restricted groups?

@Sesquipedalian

This comment has been minimized.

Copy link
Member

Sesquipedalian commented Feb 21, 2019

I don't like it either. To my mind, it only makes sense for the manage_boards permission to entail access to all boards.

However, this doesn't appear to be true:

If someone can manage boards, they can just give themselves that access anyway

In my tests while writing #5459, I found that if a non-admin with the manage_boards permission tries to use URL manipulation to change the settings of a board they can't access, it doesn't work. The user is just kicked back to the main manage boards page. (Of course, there may be some other way to do it that I haven't thought of. I'm far from omniscient, after all.)

Nevertheless, the potential for weird edge cases to cause problems remains a concern, so I would rather have continued to have the manage_boards permission entail access to all boards.

The reason to give it up is only that @jdarwood007 is right that we can't have the upgrade process unexpectedly grant people access to boards they couldn't see before.

@Sesquipedalian

This comment has been minimized.

Copy link
Member

Sesquipedalian commented Feb 21, 2019

Maybe it’s time to just fold all these into “admin forum” like other forum platforms do, because there’s a lot of potential for edge cases that really isn’t necessary.

That's not a bad idea for 3.0, but it's too late to make such a change in 2.1.

...However, it does spark a new idea for a potentially better solution to this problem.

If we simply revoke the manage_boards permission from all non-admins during the upgrade process, then we can avoid the problem @jdarwood007 is rightly concerned about while still making the change to allow full board access to anyone with the manage_boards permission. Immediately after upgrading, only the admin would be able to manage boards, and the admin would have to proactively choose to grant that permission to other groups again afterwards. Provided we make sure that the UI and the help text communicate clearly that granting this permission means granting access to all boards, then the admin will be able to make an informed decision in light of the new behaviour.

@jdarwood007

This comment has been minimized.

Copy link
Member

jdarwood007 commented Feb 22, 2019

FYI, The manage_boards showed up in the ManagePermissions.php between 1.0 RC1 and 1.0 RC2. Didn't dig into which commit added it, but it has existed since that long. Just to give an idea of how long this permission has existed.

@Arantor

This comment has been minimized.

Copy link
Contributor

Arantor commented Feb 22, 2019

How often, realistically, do people hand out manage boards without being admins?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.