[ticket/10537] Remove unnecessary permissions #505
Conversation
| @@ -5,7 +5,7 @@ | |||
| // | |||
| // FILENAME : add_permissions.php | |||
| // STARTED : Sat Nov 06, 2004 | |||
| // COPYRIGHT : © 2004 phpBB Group | |||
| // COPYRIGHT : � 2004 phpBB Group | |||
There was a problem hiding this comment.
For the record, I did not make this change. Probably something screwy with my editor. Can we just switch it to (c) instead of the copyright symbol like it is in other files?
There was a problem hiding this comment.
Maybe its because of UTF8?
|
I still don't understand the way the logic should be in viewforum.php (see the comment I made there). Can someone else take a look? |
| @@ -1078,8 +1073,8 @@ function compose_pm($id, $mode, $action, $user_folders = array()) | |||
| 'S_SIGNATURE_CHECKED' => ($sig_checked) ? ' checked="checked"' : '', | |||
| 'S_LINKS_ALLOWED' => $url_status, | |||
| 'S_MAGIC_URL_CHECKED' => ($urls_checked) ? ' checked="checked"' : '', | |||
| 'S_SAVE_ALLOWED' => ($auth->acl_get('u_savedrafts') && $action != 'edit') ? true : false, | |||
| 'S_HAS_DRAFTS' => ($auth->acl_get('u_savedrafts') && $drafts), | |||
| 'S_SAVE_ALLOWED' => (($auth->acl_get('u_sendpm') || $auth->acl_get('u_pm_reply')) && $action != 'edit') ? true : false, | |||
There was a problem hiding this comment.
Do we still have a setting to turn off the draft stuff?
There was a problem hiding this comment.
I don't see a setting to enable/disable drafts, neither in the ACP or in the code itself.
The draft mode within the UCP does/did not implement the permission that is being removed, so ultimately, a user without permission to use drafts can still access that area of the UCP. In fact, a user that has a saved draft and has Never set for the savedraft permission, can still access it but when he tries to load a draft, it just opens an empty posting page.
So really, the only part of the draft system that can be disabled even with the permission is saving drafts. There is not a global on/off permission or a global on/off setting.
IMO, we should remove this permission and implement a board setting to allow admin to global disable drafts if they really need to.
There was a problem hiding this comment.
IMO, we should remove this permission and implement a board setting to allow admin to global disable drafts if they really need to.
Which is exactly what I wanted to point out ;) It should be possible to disable them.
There was a problem hiding this comment.
However, that would be a separate RFC, I think. This RFC is only for removing permissions, not adding settings.
There was a problem hiding this comment.
But this RFC removes the "control" setting of this feature :P
There was a problem hiding this comment.
Don't see a point for that setting. Let's just remove the permission.
There was a problem hiding this comment.
I would prefer a separate PR for enabling/disabling drafts as a whole if we decide it is necessary (I do not think it is myself).
|
You should add an article to the wiki with a table what is removed or replaced by what... |
|
I agree with nickvergessen, that this needs an article on the wiki - will merge when that is created. |
|
To note, here is a wiki page listing the removed permissions. It is also linked from the 3.1 overview page. |
| @@ -2666,6 +2666,32 @@ function change_database_data(&$no_updates, $version) | |||
| $db_tools->sql_column_remove(USERS_TABLE, 'user_dst'); | |||
| } | |||
|
|
|||
| // Remove unnecessary permissions | |||
There was a problem hiding this comment.
The auth class or auth management class should have a method for dropping permissions.
This part here should then look like
foreach ('u_pm_delete' ... 'a_jabber' as $perm) { $auth->drop_permission($perm); }
There was a problem hiding this comment.
Neither class has one. This is how it is done in UMIL.
There was a problem hiding this comment.
Should take a list of permissions for efficiency reasons.
|
Patch looks incomplete. Check recent file.php and functions_download.php changes. |
| $auth_option_id = (int) $row['auth_option_id']; | ||
| foreach (array(ACL_GROUPS_TABLE, ACL_ROLES_DATA_TABLE, ACL_USERS_TABLE, ACL_OPTIONS_TABLE) as $table) | ||
| { | ||
| $db->sql_query("DELETE FROM $table WHERE auth_option_id = $auth_option_id"); |
There was a problem hiding this comment.
Use sql_in_set and a single delete per table.
|
I think I support this now. Please merge/rebase on develop and
|
|
I did not see any further use of u_pm_download in file.php, and the one occurrence in includes/functions_download.php has been changed to fall back on u_download. All that is left to do is rebase onto develop. |
|
@p This has been rebased onto develop |
|
|
||
| foreach (array(ACL_GROUPS_TABLE, ACL_ROLES_DATA_TABLE, ACL_USERS_TABLE, ACL_OPTIONS_TABLE) as $table) | ||
| { | ||
| $db->sql_query("DELETE FROM $table WHERE " . $db->sql_in_set('auth_option_id', $option_ids)); |
There was a problem hiding this comment.
Query formatting + use the _sql() function.
|
Extract a function to delete permissions like #985? |
| * @param phpbb_auth $auth Auth object | ||
| * @return null | ||
| */ | ||
| function _remove_permissions(array $permissions, &$errored, &$error_ary, dbal $db, phpbb_cache_service $cache, phpbb_auth $auth) |
There was a problem hiding this comment.
The order of arguments should be consistent with _add_permissions as much as possible.
PHPBB3-10537
PHPBB3-10537
PHPBB3-10537
PHPBB3-10537
…_pm_reply PHPBB3-10537
PHPBB3-10537
PHPBB3-10537
PHPBB3-10537
PHPBB3-10537
PHPBB3-10537
|
@p Done |
|
Database updated a new QI board. Gave admin never for u_sendpm. Going to manage pm drafts (http://localqi//boards/x34/ucp.php?i=ucp_pm&mode=drafts) - shows: Here you can view, edit and delete your saved drafts. No drafts saved. (normal ui). I would expect this to be prohibited. It seems that pre-patch behavior is the same. |
|
|
Were the changes made in this PR tested? |
|
I tested that the permissions were removed. I have not yet tested that all functionality still works (I will need to go through to each thing that was changed and ensure that it still works as intended, which may take a while). |
|
That's kind of important? Are you saying none of the changes outside of db updater/schema have been tested? |
This is supposed to be discovered during testing, and if you do not intend to fix it in the PR being submitted a ticket should be created and linked to from the PR. |
Not none; I tested some of it a while back (though I cannot remember specifically which parts). I just haven't had a chance to test it all recently. With exams done I should have plenty of time. |
| @@ -313,7 +313,6 @@ | |||
| 'NO_AUTHOR' => 'No author defined for this message', | |||
| 'NO_AVATAR_CATEGORY' => 'None', | |||
|
|
|||
| 'NO_AUTH_DELETE_MESSAGE' => 'You are not authorised to delete private messages.', | |||
| 'NO_AUTH_EDIT_MESSAGE' => 'You are not authorised to edit private messages.', | |||
| 'NO_AUTH_FORWARD_MESSAGE' => 'You are not authorised to forward private messages.', | |||
There was a problem hiding this comment.
Is this error still used with u_pm_forward removed?
|
Code looks good to me, I will test this now. |
|
The permissions for the email friend button on viewtopic is not the same as the actual page. As a guest I can see the button, but clicking it gives me: "You are not permitted to send email to this user." Where is the u_pm_reply permission? This permission does not exist (at least not on the version of develop this is based on). Was this added in a a different PR? Would you mind rebasing this on develop? Merging causes conflicts. |
|
@imkingdavid bump |
|
Because the codebase has changed significantly since i started, i'm going to close this and start over. |
http://tracker.phpbb.com/browse/PHPBB3-10537
See RFC for permissions suggested for removal.
(BTW, I closed the old pull request because I screwed up the commits and such trying to squash them into one.)