remoteuser - Patch for default setting and improved checking in hasAccess() #774
Conversation
|
That's the point. Setting it to empty will allow any user to access the remote API. Actions within the API are still subject to the ACLs of course. |
|
Oh, I see it breaks a bunch of tests. Not fine then ;-) Can you check what's going on there? |
|
@MartijnRas can you please check the failing test cases? |
|
The inspection completed: No new issues |
|
@splitbrain can you please review the updated test as i might have missed some of the intricacies of how the combinations of settings (remote, remoteuser, acl) are supposed to work? |
|
@dom-mel could you have a look at this? |
|
Is this security related and useful for coming release? or is it fine, but just an improvement? |
| @@ -175,6 +175,9 @@ public function hasAccess() { | |||
| if (!$conf['remote']) { | |||
| return false; | |||
| } | |||
| if(trim($conf['remoteuser']) == '!!not set!!') { | |||
There was a problem hiding this comment.
There is a dependency here that at least warrants a comment.
|
looks good to me so 👍 |
remoteuser - Patch for default setting and improved checking in hasAccess()
I noticed a strange default setting of remoteuser.
I also noticed hasAccess() did not check this default setting.
Furthermore I think the following code in hasAccess() should at least return false, even better would be to remove it completely, as this feels like circumventing all security mechanisms: