Move BasicAuth check to isLoggedIn #11158
Conversation
Ensures that Basic Auth works properly for APIs and removes the need for some even uglier lines of code.
|
A new inspection was created. |
|
Expected behaviour: If valid Basic Authentication header is set:
If invalid Basic Authentication header is set:
|
|
Hmm. In case of API requests no login page should be returned but only the proper http response code. Or am I missing something? |
|
Yes. That is also the expected behaviour ;-) |
|
We also only want to backport critical security problem. What is the reason here? |
Regression with #11130 that lead to the case that only a valid cookie is returned for API requests and the resource is not served. This was not the wanted behaviour and potentially breaks clients that are not using the cookie. |
|
It's worse than that: bypassing the login hook through this bug disables encryption for files. So files get uploaded unencrypted... |
|
Ok sorry, mixed up with the other basic auth fix PR... |
|
Yeah - that is "only" a bug fix for the other PR which is merged ;-) |
|
Because you can't touch things in base.php without breaking other things - I now moved the Basic Auth login check into "isLoggedIn()" where it makes more sense... |
| @@ -780,15 +780,6 @@ public static function handleRequest() { | |||
| if (isset($_COOKIE['oc_token'])) { | |||
| OC_Preferences::deleteKey(OC_User::getUser(), 'login_token', $_COOKIE['oc_token']); | |||
| } | |||
| if (isset($_SERVER['PHP_AUTH_USER'])) { | |||
There was a problem hiding this comment.
@LukasReschke please double check the git history here - this was added on purpose to enable some scenarios.
Looks like I'm loosing track of all the reasons why stuff was changed ......
There was a problem hiding this comment.
This was added with 4ddf5d9 and is not necessary anymore. Login and logout works also now with invalid basic auth credentials.
Logout is not something that we can control do due to Basic Auth though, if somebody decides to access ownCloud via Basic Auth the credentials are sent by the browser - nothing we can control.
|
O.K. Thanks for the explanation. 👍 |
|
💣 Test Failed. 💣 |
LukasReschke
changed the title
|
Let's try again .... great software... |
|
@owncloud-bot Retest this please |
|
🚀 Test Passed. 🚀 |
|
👍 fixes login issues with the news app on android |
Move BasicAuth check to isLoggedIn
|
Stable7: 37632e4 |
|
@danimo maybe this also fixes your auth issue |
Ensures that Basic Auth works properly for APIs and removes the need for some even uglier lines of code.
Related to #11155 (comment) and needs a backport to stable7.
@PVince81 @DeepDiver1975 @karlitschek Testing welcome. Especially the APIs (e.g.
curl -D - http://admin:admin@localhost/core/index.php/apps/files/api/v1/thumbnail/100/100/welcome.txt) and #11129