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

Fixes #3233 Cascading usergroup moderator permissions broken #3421

Open
wants to merge 1 commit into
base: feature
Choose a base branch
from
Open

Fixes #3233 Cascading usergroup moderator permissions broken #3421

wants to merge 1 commit into from

Conversation

Starpaul20
Copy link
Member

Fixes #3233

@euantorano euantorano added the b:1.8 Branch: 1.8.x label Aug 31, 2018
@dvz
Copy link
Contributor

dvz commented Sep 6, 2018

$perms values can be inadvertently downgraded when a group with lower permissions is checked after a group with higher permissions in the same forum.

Some variables, like $perms[$action], can be undefined when used with max() (same with existing code, but we can start fixing those given the code will be carried over to future branches).

@Eldenroot
Copy link
Contributor

@PaulBender - are you willing to complete this?

@Starpaul20
Copy link
Member Author

I can't really, I just changed computers and I'm still settling it in.

@euantorano euantorano self-requested a review November 28, 2018 23:03
@Ben-MyBB Ben-MyBB added the s:feedback Status: Feedback. Further information/input needed label May 29, 2019
@Ben-MyBB
Copy link
Member

Ben-MyBB commented Aug 4, 2019

@PaulBender Ping, if you get time.

@Eldenroot
Copy link
Contributor

@PaulBender - please can you finish this one? To review and merge :) thank you!

@Starpaul20
Copy link
Member Author

Don't know if I can, I had to delete and recreate my repository awhile back.

@Eldenroot
Copy link
Contributor

This PR is a small one... maybe recreate it? :) thx

@Starpaul20
Copy link
Member Author

Yeah, I'll try to fix this and recreate it this week if I can.

@Starpaul20
Copy link
Member Author

After playing around with this issue last night, I've come to the conclusion that the original issue probably isn't worth fixing, at least in 1.8.

@yuliu
Copy link
Member

yuliu commented Jan 6, 2020

euantorano has said in Discord on a relating issue #3776:

Yes, permissions can be confusing sometimes especially for users with lots of groups
That’s why 2.0 was going to use a yes/no/never system like XenForo’s
I’d also like a way to visualise the exact permissions for any given user

@Ben-MyBB
Copy link
Member

@euantorano thoughts on this one?

@euantorano
Copy link
Member

Will have to review and reacquaint myself, but based on comments it sounds like we may be looking at a different approach in the future.

@yuliu
Copy link
Member

yuliu commented May 16, 2020

I have some idea about resolving #3996 (including #3233 that this PR tries to resolve), however the PR resolving #3996 and this PR will conflict with each over.

Anyway, the way getting mod permissions from multiple user groups of this PR seems fine to me, though primitive but does some work and should have no side effect.

@lairdshaw
Copy link
Contributor

With the intention of reviewing unmerged 1.8 PRs and issues to try to get them merged where possible, I've just revisited this PR, and I think the concerns raised by @dvz above, and @yuliu in #3996 mean that merging this PR would create as many problems as it solves - e.g., it would mean that the comment // User settings override usergroup settings no longer applies unconditionally, given that a $value of zero in the usergroup code block would override an earlier non-zero permission set in the preceding user code block.

Recommendation: close this PR without merging it, and revisit permissions in 1.9, taking careful account of @yuliu's and @dvz's comments/analyses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
b:1.8 Branch: 1.8.x s:feedback Status: Feedback. Further information/input needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cascading usergroup moderator permissions broken
7 participants