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

[ticket/10537] Remove unnecessary permissions #505

Closed
wants to merge 23 commits into
base: develop
from

Conversation

Projects
None yet
7 participants
@imkingdavid
Contributor

imkingdavid commented Dec 18, 2011

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

@imkingdavid

View changes

Show outdated Hide outdated phpBB/develop/add_permissions.php
@imkingdavid

View changes

Show outdated Hide outdated phpBB/viewforum.php
@imkingdavid

View changes

Show outdated Hide outdated phpBB/develop/add_permissions.php
@imkingdavid

This comment has been minimized.

Show comment
Hide comment
@imkingdavid

imkingdavid Jan 18, 2012

Contributor

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?

Contributor

imkingdavid commented Jan 18, 2012

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,

This comment has been minimized.

@nickvergessen

nickvergessen Feb 20, 2012

Contributor

Do we still have a setting to turn off the draft stuff?

@nickvergessen

nickvergessen Feb 20, 2012

Contributor

Do we still have a setting to turn off the draft stuff?

This comment has been minimized.

@imkingdavid

imkingdavid Feb 20, 2012

Contributor

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.

@imkingdavid

imkingdavid Feb 20, 2012

Contributor

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.

This comment has been minimized.

@nickvergessen

nickvergessen Feb 21, 2012

Contributor

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.

@nickvergessen

nickvergessen Feb 21, 2012

Contributor

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.

This comment has been minimized.

@imkingdavid

imkingdavid Feb 21, 2012

Contributor

However, that would be a separate RFC, I think. This RFC is only for removing permissions, not adding settings.

@imkingdavid

imkingdavid Feb 21, 2012

Contributor

However, that would be a separate RFC, I think. This RFC is only for removing permissions, not adding settings.

This comment has been minimized.

@nickvergessen

nickvergessen Feb 21, 2012

Contributor

But this RFC removes the "control" setting of this feature :P

@nickvergessen

nickvergessen Feb 21, 2012

Contributor

But this RFC removes the "control" setting of this feature :P

This comment has been minimized.

@naderman

naderman Jun 12, 2012

Member

Don't see a point for that setting. Let's just remove the permission.

@naderman

naderman Jun 12, 2012

Member

Don't see a point for that setting. Let's just remove the permission.

This comment has been minimized.

@EXreaction

EXreaction Jan 11, 2013

Contributor

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

@EXreaction

EXreaction Jan 11, 2013

Contributor

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

@nickvergessen

This comment has been minimized.

Show comment
Hide comment
@nickvergessen

nickvergessen Feb 20, 2012

Contributor

You should add an article to the wiki with a table what is removed or replaced by what...

Contributor

nickvergessen commented Feb 20, 2012

You should add an article to the wiki with a table what is removed or replaced by what...

@naderman

This comment has been minimized.

Show comment
Hide comment
@naderman

naderman Jun 12, 2012

Member

I agree with nickvergessen, that this needs an article on the wiki - will merge when that is created.

Member

naderman commented Jun 12, 2012

I agree with nickvergessen, that this needs an article on the wiki - will merge when that is created.

@imkingdavid

This comment has been minimized.

Show comment
Hide comment
@imkingdavid

imkingdavid Jun 12, 2012

Contributor

To note, here is a wiki page listing the removed permissions. It is also linked from the 3.1 overview page.
http://wiki.phpbb.com/Permissions_Removed_in_3.1

Contributor

imkingdavid commented Jun 12, 2012

To note, here is a wiki page listing the removed permissions. It is also linked from the 3.1 overview page.
http://wiki.phpbb.com/Permissions_Removed_in_3.1

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jun 12, 2012

This pull request fails (merged abe3001b into e045a80).

travisbot commented Jun 12, 2012

This pull request fails (merged abe3001b into e045a80).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jun 12, 2012

This pull request fails (merged 6917296f into e045a80).

travisbot commented Jun 12, 2012

This pull request fails (merged 6917296f into e045a80).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jun 12, 2012

This pull request fails (merged b12e0d83 into e045a80).

travisbot commented Jun 12, 2012

This pull request fails (merged b12e0d83 into e045a80).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jun 12, 2012

This pull request passes (merged 0b32e33e into e045a80).

travisbot commented Jun 12, 2012

This pull request passes (merged 0b32e33e into e045a80).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Sep 1, 2012

This pull request passes (merged c1be6aa4 into d866624).

travisbot commented Sep 1, 2012

This pull request passes (merged c1be6aa4 into d866624).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Sep 1, 2012

This pull request passes (merged 2466d4f0 into d866624).

travisbot commented Sep 1, 2012

This pull request passes (merged 2466d4f0 into d866624).

@bantu

View changes

Show outdated Hide outdated phpBB/install/database_update.php
@bantu

This comment has been minimized.

Show comment
Hide comment
@bantu

bantu Sep 2, 2012

Member

Patch looks incomplete. Check recent file.php and functions_download.php changes.

Member

bantu commented Sep 2, 2012

Patch looks incomplete. Check recent file.php and functions_download.php changes.

@p

p reviewed Dec 7, 2012
View changes

Show outdated Hide outdated phpBB/install/database_update.php
@p

This comment has been minimized.

Show comment
Hide comment
@p

p Dec 7, 2012

Contributor

I think I support this now. Please merge/rebase on develop and

Patch looks incomplete. Check recent file.php and functions_download.php changes.

Contributor

p commented Dec 7, 2012

I think I support this now. Please merge/rebase on develop and

Patch looks incomplete. Check recent file.php and functions_download.php changes.

@imkingdavid

This comment has been minimized.

Show comment
Hide comment
@imkingdavid

imkingdavid Dec 12, 2012

Contributor

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.

Contributor

imkingdavid commented Dec 12, 2012

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.

@imkingdavid

This comment has been minimized.

Show comment
Hide comment
@imkingdavid

imkingdavid Dec 12, 2012

Contributor

@p This has been rebased onto develop

Contributor

imkingdavid commented Dec 12, 2012

@p This has been rebased onto develop

@bantu

View changes

Show outdated Hide outdated phpBB/install/database_update.php
@p

This comment has been minimized.

Show comment
Hide comment
@p

p Dec 13, 2012

Contributor

Extract a function to delete permissions like #985?

Contributor

p commented Dec 13, 2012

Extract a function to delete permissions like #985?

@p

p reviewed Dec 13, 2012
View changes

Show outdated Hide outdated phpBB/install/database_update.php
@p

This comment has been minimized.

Show comment
Hide comment
@p

p Dec 14, 2012

Contributor

Due to #908 merge please rebase and apply the equivalent of p@1a1ae1b.

Contributor

p commented Dec 14, 2012

Due to #908 merge please rebase and apply the equivalent of p@1a1ae1b.

@imkingdavid

This comment has been minimized.

Show comment
Hide comment
@imkingdavid

imkingdavid Dec 14, 2012

Contributor

@p Done

Contributor

imkingdavid commented Dec 14, 2012

@p Done

@p

This comment has been minimized.

Show comment
Hide comment
@p

p Dec 15, 2012

Contributor

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.

Contributor

p commented Dec 15, 2012

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.

@imkingdavid

This comment has been minimized.

Show comment
Hide comment
@imkingdavid

imkingdavid Dec 16, 2012

Contributor

It seems that pre-patch behavior is the same.
Then that should go in a separate ticket/PR.

Contributor

imkingdavid commented Dec 16, 2012

It seems that pre-patch behavior is the same.
Then that should go in a separate ticket/PR.

@p

This comment has been minimized.

Show comment
Hide comment
@p

p Dec 16, 2012

Contributor

Were the changes made in this PR tested?

Contributor

p commented Dec 16, 2012

Were the changes made in this PR tested?

@imkingdavid

This comment has been minimized.

Show comment
Hide comment
@imkingdavid

imkingdavid Dec 16, 2012

Contributor

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

Contributor

imkingdavid commented Dec 16, 2012

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

@p

This comment has been minimized.

Show comment
Hide comment
@p

p Dec 16, 2012

Contributor

That's kind of important?

Are you saying none of the changes outside of db updater/schema have been tested?

Contributor

p commented Dec 16, 2012

That's kind of important?

Are you saying none of the changes outside of db updater/schema have been tested?

@p

This comment has been minimized.

Show comment
Hide comment
@p

p Dec 16, 2012

Contributor

It seems that pre-patch behavior is the same.

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.

Contributor

p commented Dec 16, 2012

It seems that pre-patch behavior is the same.

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.

@imkingdavid

This comment has been minimized.

Show comment
Hide comment
@imkingdavid

imkingdavid Dec 17, 2012

Contributor

Are you saying none of the changes outside of db updater/schema have been tested?

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.

Contributor

imkingdavid commented Dec 17, 2012

Are you saying none of the changes outside of db updater/schema have been tested?

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.',

This comment has been minimized.

@EXreaction

EXreaction Jan 11, 2013

Contributor

Is this error still used with u_pm_forward removed?

@EXreaction

EXreaction Jan 11, 2013

Contributor

Is this error still used with u_pm_forward removed?

@EXreaction

This comment has been minimized.

Show comment
Hide comment
@EXreaction

EXreaction Jan 11, 2013

Contributor

Code looks good to me, I will test this now.

Contributor

EXreaction commented Jan 11, 2013

Code looks good to me, I will test this now.

@EXreaction

This comment has been minimized.

Show comment
Hide comment
@EXreaction

EXreaction Jan 11, 2013

Contributor

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.

Contributor

EXreaction commented Jan 11, 2013

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.

@EXreaction

This comment has been minimized.

Show comment
Hide comment
@EXreaction
Contributor

EXreaction commented Jul 12, 2013

@imkingdavid

This comment has been minimized.

Show comment
Hide comment
@imkingdavid

imkingdavid Sep 13, 2013

Contributor

Because the codebase has changed significantly since i started, i'm going to close this and start over.

Contributor

imkingdavid commented Sep 13, 2013

Because the codebase has changed significantly since i started, i'm going to close this and start over.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment