Skip to content

Conversation

@andrepereiradasilva
Copy link
Contributor

@andrepereiradasilva andrepereiradasilva commented Aug 3, 2016

Pull Request for Improvement.

Summary of Changes

Replace com_banners exisiting 404 JError for a 403 php exception when the user does not have access to "Access Administration Interface" (core.manage).

Before

image

After

image

Testing Instructions

  1. Use latest staging
  2. Create a user and add it to "Administrator" group
  3. Go to com_banners and set "Access Administration Interface" (core.manage) to "Denied" for "Administrator" group
  4. Login with the Administrator user in a private window and go to /administrator/index.php?option=com_banners
  5. See the red message (Before)
  6. Apply patch
  7. Repeat step 4, you'll see now a 403 error (After).

If this change is ok i can do it for the other components that uses JError here.

@andrepereiradasilva andrepereiradasilva changed the title [com_banners] User not allowed to core.manage? Use 403 php exception (instead of jerror 404) [com_banners] User not allowed to core.manage? Use 403 php exception (instead of a 404 JError) Aug 3, 2016
@mbabker
Copy link
Contributor

mbabker commented Aug 3, 2016

I'd prefer a more specific Exception class but +1 on the idea.

@andrepereiradasilva
Copy link
Contributor Author

yes i would prefer to, but can you hint on how to add a specific exception like NotAllowedException?

@mbabker
Copy link
Contributor

mbabker commented Aug 3, 2016

Well, if there isn't something in the standard SPL exceptions, just add a custom class like we have in the database API.

@andrepereiradasilva
Copy link
Contributor Author

hum, doesn't seem to be any http://php.net/manual/en/spl.exceptions.php

@tomartailored
Copy link

I have tested this item ✅ successfully on 0cded66


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

2 similar comments
@1apweb
Copy link

1apweb commented Aug 4, 2016

I have tested this item ✅ successfully on 0cded66


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

@truptikagathara
Copy link

I have tested this item ✅ successfully on 0cded66


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

@jeckodevelopment
Copy link
Member

RTC please


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 13, 2016
@rdeutz rdeutz added this to the Joomla 3.6.3 milestone Aug 14, 2016
@rdeutz rdeutz merged commit 858c97f into joomla:staging Aug 14, 2016
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 14, 2016
@rdeutz
Copy link
Contributor

rdeutz commented Aug 14, 2016

I would like to see a nicer error message, this one looks very technical. I also think to have some custom exceptions would be great. Merging it anyway because this is the way to go

@andrepereiradasilva andrepereiradasilva deleted the patch-9 branch August 14, 2016 16:03
@andrepereiradasilva
Copy link
Contributor Author

@rdeutz i will change to a custom exception when this #11593 gets merged

@jeckodevelopment
Copy link
Member

what about:

You are not allowed to access this resource. Please contact your website administrator in order to get access to this resource.

or

You are not allowed to access this resource. Your website administrator can give you access to this resource just changing permissions.

@andrepereiradasilva
Copy link
Contributor Author

andrepereiradasilva commented Aug 14, 2016

people that is a language change nothing to do with this PR which porpose is to change the error to an exception.
Please make a PR for changing JERROR_ALERTNOAUTHOR if you feel the need.

izharaazmi added a commit to izharaazmi/joomla-cms that referenced this pull request Aug 15, 2016
* 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)
  ...
ggppdk pushed a commit to ggppdk/joomla-cms that referenced this pull request Aug 19, 2016
I would like to see a nicer error message, this one looks very technical. I also think to have some custom exceptions would be great. Merging it anyway because this is the way to go
roland-d pushed a commit to roland-d/joomla-cms that referenced this pull request Sep 11, 2016
I would like to see a nicer error message, this one looks very technical. I also think to have some custom exceptions would be great. Merging it anyway because this is the way to go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants