Regression: Fix edit check in backend articles manager, always denying edit after soft deny #11511

ggppdk · 2016-08-08T00:45:46Z

Pull Request for Issue #11463 and #11510

Summary of Changes

Fixed edit check in backend not allowing edit via edit.own after soft deny
Same code used for frontend

Testing Instructions

(check is the same for frontend / backend)

at backend articles manager,
at frontend views that show the edit button

make sure that people that should be allowed to edit can do and others can not

infograf768 · 2016-08-08T05:27:33Z

I have tested this item ✅ successfully on

Looks like working here.

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11511.}

alikon · 2016-08-08T05:55:33Z

i have experienced this issue on GsoC 2016 Multilingual project.

test conditions

Create a user "test" added to "Administrator" group
Go to com_content Options, Permissions tab and disable "Edit" for "Administrator" group
Create a new content item with the "test" user

before patch
You can't edit your own.

Apply patch
you can edit your own items.

p.s
tested on backend only

jeckodevelopment · 2016-08-08T12:48:06Z

I have tested this item ✅ successfully on

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11511.}

ggeorgg · 2016-08-08T18:20:23Z

I have tested this item ✅ successfully on
Thanks a lot!

zero-24 · 2016-08-08T19:06:31Z

RTC. Thanks

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11511.}

matrix630307 · 2016-08-09T11:06:22Z

I have tested this item successfully. Thank's.

brianteeman · 2016-08-09T23:50:25Z

administrator/components/com_content/controllers/article.php

 		$user = JFactory::getUser();

-		// For new record (id:0) return component permission
+		// Zero record (id:0) return component permission, e.g. show edit btn


That makes less sense than it did before. Now I domt have a clue what it means

It was like this before,

why any code is calling allowEdit() on zero id ?
only purpose would be to get component edit

for me it is meaningless, and from what i see in the code ,
we can probably return false and break nothing,

I have checked this,

there is no code in Joomla that uses allowEdit() to decide creating new records, or to get component permissions (by passing zero record id)

so we can change it to false !

(maybe some 3rd party extends the class though and use it to get component edit, but i think it is very unlikely)

Sorry, with like this before

i mean current "frontend" controller

and backend controller in J3.6.0

zero-24 · 2016-08-10T14:38:28Z

Put off RTC so we get a retest after the last changes Thanks.

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11511.}

ggppdk · 2016-08-10T14:49:52Z

yes i made 1 more change (besides comments),

always return false if article id is 0

I searched through all calls of the method, i did not find any usage that will need to return component permissions, in such a case,

so better return false (aka deny), in case someone in the future writes code to misuse this

mbabker · 2016-08-10T14:52:59Z

Except this is wrong. You've introduced a hard deny rule instead of continued use of the ACL system in this scenario. So there needs to be a pretty freakin' strong validation against this change because right now that screams massive B/C break to me.

mbabker · 2016-08-10T14:54:08Z

administrator/components/com_content/controllers/article.php


-		// If we get a deny at the component level, we cannot override here.
-		if (!parent::allowEdit($data, $key))
+		// Zero record (id:0) return false, if caller wants component permissions just get them directly


So a method that is supposed to check the ACL now has a hardcoded deny rule in place when it should be using the ACL system? Am I the only one who sees an issue with this?

ggppdk · 2016-08-10T15:03:02Z

Can you find a place that is needed today ?
And something that gets broken ?

example

    protected function allowSave($data, $key = 'id')
    {
        $recordId = isset($data[$key]) ? $data[$key] : '0';

        if ($recordId)
        {
            return $this->allowEdit($data, $key);
        }
        else
        {
            return $this->allowAdd($data);
        }
    }

so calling allowEdit() on zero record id as an indirect way to get return parent controler permissions ?
which return the component permission,

is weird usage

i will revert, i made this change worrying of someone misusing it in the future
it creates no problems

mbabker · 2016-08-10T15:08:57Z

I don't have a place off hand that will break with that hardcoded change but the point of it is that something that should be reading the ACL system should be doing that, not introducing an arbitrary rule based on some condition and not even implement it consistently.

ggppdk · 2016-08-10T15:19:22Z

I restored the code it is same as before, when all testing was done

but that deny would break nothing anyway, because it is not used anywhere

anyway this is PR makes the allowEdit how it should have been in the first place

mbabker · 2016-08-10T15:20:46Z

I agree it shouldn't be getting used, but in the off case someone's code is screwed up somewhere it should still respect the ACL system or throw an Exception because at that point it's dealing with an invalid record anyway.

ggppdk · 2016-08-10T15:21:12Z

The code is correct to be there
i am not saying something different

but after 1 or 2 years someone may write code to misuse it

that was my only reason for making that change

mbabker · 2016-08-10T15:23:16Z

It needs to be a separate proposal if you're going to push for it. We all have a very bad habit of throwing unrelated items into pull requests because we're already making a change or want to enforce some arbitrary standard. That crap needs to stop.

ggppdk · 2016-08-10T15:24:32Z

It needs to be a separate proposal if you're going to push for it. We all have a very bad habit of throwing unrelated items into pull requests because we're already making a change or want to enforce some arbitrary standard. That crap needs to stop.

yes correct, it is unrelated to this PR, but you know how it is with finding people to test PRs etc

mbabker · 2016-08-10T15:26:29Z

Oh I know, I know. I've got cache PRs stalled out because someone asked for an arbitrary extra change, one of which actually adds support for a PHP 5.3 feature to our callback system.

infograf768 · 2016-08-10T16:58:14Z

where do we stand here?

ggppdk · 2016-08-10T19:46:04Z

Hello

please click on each of the commit made August 10,
https://github.com/joomla/joomla-cms/pull/11511/commits

5 commits

1 commit changes comments
2 next commits changed in the 2 controllers (frontend / backend identical), for the case of record ID zero

        if (!$recordId)
        {
-           return parent::allowEdit($data, $key);
+           return false;
        }

this should be still be RTC

2 more commits changed it back to be the same as when all tests where made

        if (!$recordId)
        {
-           return false;
+           return parent::allowEdit($data, $key);
        }

zero-24 · 2016-08-10T19:50:46Z

Back to RTC. Thanks

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11511.}

Sieger66 · 2016-08-14T00:06:11Z

I have tested this item ✅ successfully on 84d0dad

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11511.}

* re-arrayhelper-min: (2467 commits) Minimize JArrayHelper methods `toInteger`, `pivot`, `arrayUnique` by using Joomla\Utilities\ArrayHelper internally. Leaving (reverting from joomla#7782) other four methods as is for b/c reasons as mentioned in joomla#8455. remove platform include (joomla#11615) [GitHub Templates] Make headings bigger (joomla#11607) [com_contact] Make ACL core.edit.own work (PR for 11466) (joomla#11503) Small review on docs & code structure in JModelLegacy library classes (joomla#11057) Obviously, this should be an array. (joomla#11610) Don't manually import JPlatform anymore (joomla#10841) Parse preprocess rules from component routers (joomla#8986) Add the correct exception after 11593 merge (was waiting for that merrge) (joomla#11606) Add missing clean line after joomla#9277 (joomla#11605) Deprecate the _PROFILER global var (joomla#10845) Spelling errors (joomla#11604) Moved travis javascript bash file to build/travis like joomla#11600 (joomla#11603) Regression: Fix edit check in backend articles manager, always denying edit after soft deny (joomla#11511) [com_plugins] User not allowed to core.manage? Use 403 php custom exception (instead of a 404 JError) (joomla#11593) [com_newsfeeds] Make ACL core.edit.own work (PR for 11466) (joomla#11502) $result-variable-undefined-given-default-value (joomla#9277) com_banners use exceptions. and not allowed is a 403 (joomla#11418) Frontend & plugins using the autoloader (joomla#10882) New version of PR 6788 (JText::_() Optimizations) (joomla#11235) ...

…g edit after soft deny (joomla#11511) * Fix allow after soft deny in backend * Improve edit check in frontend similar to backend * Better comments for the code * Force better usage of allowEdit * Force better usage of allowEdit * Restore behaviour on zero record id to return component edit permissions * Restore behaviour on zero record id to return component edit permissions

ggppdk added 2 commits August 8, 2016 03:40

Fix allow after soft deny in backend

a707d60

Improve edit check in frontend similar to backend

1ad1ce2

joomla-cms-bot added the PR-staging label Aug 8, 2016

This was referenced Aug 8, 2016

Allow Edit after Soft deny is no longer possible (always denied) for backend articles manager #11463

Closed

Error: Editing not allowed (ACL problem) #11510

Closed

ggppdk changed the title ~~Fix edit check in backend not allowing edit via edit.own after soft deny~~ Fix edit check in backend articles manager, always denying edit after soft deny Aug 8, 2016

joomla-cms-bot changed the title ~~Fix edit check in backend articles manager, always denying edit after soft deny~~ Regression: Fix edit check in backend articles manager, always denying edit after soft deny Aug 8, 2016

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 8, 2016

Better comments for the code

df0bff1

brianteeman reviewed Aug 9, 2016
View reviewed changes

ggppdk added 2 commits August 10, 2016 03:05

Force better usage of allowEdit

001e157

Force better usage of allowEdit

0c7e0a8

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 10, 2016

mbabker reviewed Aug 10, 2016
View reviewed changes

ggppdk added 2 commits August 10, 2016 18:06

Restore behaviour on zero record id to return component edit permissions

d5e378b

Restore behaviour on zero record id to return component edit permissions

84d0dad

ggppdk mentioned this pull request Aug 10, 2016

Refactor allowEdit of backend category controller #11547

Merged

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 10, 2016

rdeutz added this to the Joomla 3.6.3 milestone Aug 14, 2016

rdeutz merged commit cb4ffa5 into joomla:staging Aug 14, 2016

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 14, 2016

andrepereiradasilva mentioned this pull request Aug 15, 2016

[com_contact] Make ACL core.edit.own work (PR for 11466) #11503

Merged

ggppdk mentioned this pull request Aug 17, 2016

ACL article edit permissions not inherited by Category #11650

Closed

ggppdk mentioned this pull request Sep 6, 2016

Users cannot edit a joomla article on site backend (administrator) #11955

Closed

Uh oh!

Regression: Fix edit check in backend articles manager, always denying edit after soft deny #11511

Regression: Fix edit check in backend articles manager, always denying edit after soft deny #11511

Uh oh!

Conversation

ggppdk commented Aug 8, 2016 • edited by joomla-cms-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Testing Instructions

Uh oh!

infograf768 commented Aug 8, 2016

Uh oh!

alikon commented Aug 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeckodevelopment commented Aug 8, 2016

Uh oh!

ggeorgg commented Aug 8, 2016

Uh oh!

zero-24 commented Aug 8, 2016

Uh oh!

matrix630307 commented Aug 9, 2016

Uh oh!

brianteeman Aug 9, 2016

Choose a reason for hiding this comment

Uh oh!

ggppdk Aug 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggppdk Aug 10, 2016

Choose a reason for hiding this comment

Uh oh!

zero-24 commented Aug 10, 2016

Uh oh!

ggppdk commented Aug 10, 2016

Uh oh!

mbabker commented Aug 10, 2016

Uh oh!

mbabker Aug 10, 2016

Choose a reason for hiding this comment

Uh oh!

ggppdk commented Aug 10, 2016

Uh oh!

mbabker commented Aug 10, 2016

Uh oh!

ggppdk commented Aug 10, 2016

Uh oh!

mbabker commented Aug 10, 2016

Uh oh!

ggppdk commented Aug 10, 2016

Uh oh!

mbabker commented Aug 10, 2016

Uh oh!

ggppdk commented Aug 10, 2016

Uh oh!

mbabker commented Aug 10, 2016

Uh oh!

infograf768 commented Aug 10, 2016

Uh oh!

ggppdk commented Aug 10, 2016

Uh oh!

zero-24 commented Aug 10, 2016

Uh oh!

Sieger66 commented Aug 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

ggppdk commented Aug 8, 2016 •

edited by joomla-cms-bot

Loading

alikon commented Aug 8, 2016 •

edited

Loading

ggppdk Aug 9, 2016 •

edited

Loading