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

[com_content] - respect acces level #18417

Merged
merged 2 commits into from Jan 16, 2018

Conversation

Projects
None yet
10 participants
@alikon
Contributor

alikon commented Oct 25, 2017

Pull Request for Issue #18413

Summary of Changes

cheked the access level always

Testing Instructions

see #18413

Expected result

access level respected

Actual result

access level not respected

@@ -253,12 +253,11 @@ protected function getListQuery()
}
// Filter by access level.
if ($access = $this->getState('filter.access'))

This comment has been minimized.

@mbabker

mbabker Oct 25, 2017

Member

Seems like the fix for this should be to make the getState() call default return true instead of removing support for the filter.

if ($this->getState('filter.access', true))

This comment has been minimized.

@alikon

alikon Oct 25, 2017

Contributor

uhm $this->getState('filter.access', true) return false

This comment has been minimized.

@mbabker

mbabker Oct 25, 2017

Member

Then something is setting the state filter to false. Either way it definitely feels wrong to just up and remove support for a state filter.

This comment has been minimized.

@alikon

alikon Oct 25, 2017

Contributor

is not removed is performed always

This comment has been minimized.

@mbabker

mbabker Oct 25, 2017

Member

You are removing support for an outside caller to say "I do not want the result to have the access filter applied". Which is technically a B/C break and breaks a feature used in at least one core module.

// Access filter
$access = !JComponentHelper::getParams('com_content')->get('show_noauth');
$authorised = JAccess::getAuthorisedViewLevels(JFactory::getUser()->get('id'));
$model->setState('filter.access', $access);

This comment has been minimized.

@alikon

alikon Oct 25, 2017

Contributor

you are right as usual 😃

Then something is setting the state filter to false

please double check i'm doing right
https://github.com/joomla/joomla-cms/pull/18417/files#diff-e5b3effc5f60b387662db42517e31654R123
should cover now

@csthomas

This comment has been minimized.

Contributor

csthomas commented Oct 25, 2017

What about default value for that:

$access = $this->getState('filter.access');

@danpdanp

This comment has been minimized.

danpdanp commented Oct 26, 2017

I have not tested this item.

The test was successful


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18417.

@franz-wohlkoenig

This comment has been minimized.

franz-wohlkoenig commented Oct 26, 2017

@danpdanp you didn't test?

@ladyjer

This comment has been minimized.

Contributor

ladyjer commented Dec 4, 2017

I have tested this item successfully on 8a81c65


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18417.

1 similar comment
@Sieger66

This comment has been minimized.

Sieger66 commented Jan 7, 2018

I have tested this item successfully on 8a81c65


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18417.

@Quy

This comment has been minimized.

Contributor

Quy commented Jan 7, 2018

RTC

@joomla-cms-bot joomla-cms-bot added the RTC label Jan 7, 2018

@mbabker mbabker added this to the Joomla 3.8.4 milestone Jan 16, 2018

@mbabker mbabker merged commit a8ce731 into joomla:staging Jan 16, 2018

5 checks passed

JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!

@joomla-cms-bot joomla-cms-bot removed the RTC label Jan 16, 2018

@alikon alikon deleted the alikon:patch-72 branch Jan 17, 2018

@ggppdk

This comment has been minimized.

Contributor

ggppdk commented Feb 7, 2018

Please read this, i think this PR is not a proper fix
#19601 (comment)

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