Skip to content
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

Fix API access protection check #5053

Closed
wants to merge 3 commits into from
Closed

Conversation

nka11
Copy link
Contributor

@nka11 nka11 commented Apr 20, 2016

Fix API Access Control bogus implementation

@hregis
Copy link
Contributor

hregis commented Apr 20, 2016

why ? :-)

@nka11
Copy link
Contributor Author

nka11 commented Apr 20, 2016

I was stalled in my devs while trying to use API features with external users.
After tweaking the code, I've read the following in api classes :

 * @access protected
 * @class  DolibarrApiAccess {@requires user,external}

and after debugging deeply, it appears that the require's override in annotation sets a string in the variable, not an array as expect by the initial code.

For the environmen I use :
PHP 7.0.5 (cli) (built: Apr 2 2016 23:10:23) ( NTS )
Linux 4.4.5-1-ARCH #1 SMP PREEMPT
Dolibarr updated with Restler RC6 (ref #4912)

I could propose an alternative fix with a split of the static::$requires var before return in_array but I'm not convinced about performance and i'm pretty sure it will increase memory footprint.

@eldy
Copy link
Member

eldy commented Apr 20, 2016

Don't you think this change (having $requires that is a string instead of array may be a result of the RC6 version ?
In dev branch, we are 3.0.0rc5

If yes, may be we can fix by adding a

$requirefortest = static::$requires
if (! is_array($requirefortest)) $requirefortest=explode(',',$requirefortest);
return in_array(static::$role, (array) static::$requirefortest) || static::$role == 'admin';

Can you test this on your case so we have something that works in all cases.

@nka11
Copy link
Contributor Author

nka11 commented Apr 21, 2016

@eldy I think having $requires that is a string instead of array is a standard comportement when it's overrided in annotations.
As I'm not able to test with Restler RC5, I can't tell if the actual implementation works for an external user. If someone can confirm it works, then the alternate implementations would fit. If not, the original implementation is bogus and never worked at all, then I recommand my first proposal as it's more efficient on the resources point of view.

For the PHP7 compliance, I've PR'ed Restler RC6 in #5057.

Regards

eldy added a commit that referenced this pull request Apr 22, 2016
@eldy
Copy link
Member

eldy commented Apr 22, 2016

To reduce risk of introducing other problem i pushed i fix like i suggested.
It should works for you with RC6. Can you confirm ?

@eldy eldy added the Discussion Some questions or discussions are opened and wait answers of author or other people to be processed label Apr 22, 2016
nka11 added a commit to nka11/dolibarr that referenced this pull request Apr 24, 2016
@nka11 nka11 closed this Apr 24, 2016
@nka11 nka11 deleted the api_fix branch April 24, 2016 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Some questions or discussions are opened and wait answers of author or other people to be processed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants