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.

Sign up for GitHub

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

Jump to bottom

Fixes login / logout when HTTP Basic Headers are avilable. #7852

Merged
merged 6 commits into from
Apr 28, 2014
Merged

Fixes login / logout when HTTP Basic Headers are avilable. #7852

merged 6 commits into from
Apr 28, 2014

Conversation

josh4trunks
Copy link
Contributor

This patch..

  1. Ignores HTTP Basic Auth Headers when they are incorrect.
  2. Adds a cookie 'oc_ignore_php_auth_user' when a user with successful 'BasicAuthLogin' wants to logout.
    • Fixes impossible logout with matching HTTP Basic User/Pass bug
  3. Decreases the timelimit of the 'oc_ignore_php_auth_user' cookie to 5 minutes when a user who needed the cookie logs out.
    • Reallows HTTP Basic Auth for that browsing session
  4. HTTP Basic Username must match the value of the 'oc_ignore_php_auth_user' cookie for it to take affect.

Fixes #6922 #4556
In my opinion this is much cleaner and HTTP compliant than using http://whatever@domain.com as it has been depreciated in Chrome and IE.

@josh4trunks
Copy link
Contributor Author

@PVince81 @DamnDam

@Kondou-ger - Can you test if this works in your environment described here? da19109#commitcomment-5770399. I believe it should without needing the extra configurable 'basic_auth'.

@josh4trunks josh4trunks mentioned this pull request Mar 24, 2014
Closed
@ghost
Copy link

ghost commented Mar 24, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/3844/

@ghost
Copy link

ghost commented Mar 24, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/3845/

@josh4trunks
Copy link
Contributor Author

Logout still seems very spotty when you have an _incorrect_ HTTP Basic Auth header. All other cases seem to work as expected.

EDIT
This isn't caused by my patch. I tested without patch and just removing the block containing..
"Session loginname ($sessionUser) doesn't match SERVER[PHP_AUTH_USER] ($serverUser).",
and it is still an issue. I'll continue investigating.

@ghost
Copy link

ghost commented Mar 24, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/3846/

@josh4trunks
Copy link
Contributor Author

OK, all fixed. Every combination should now work =]

  • Correct Headers + Login
  • Correct Headers + Logout
  • Incorrect Headers + Login
  • Incorrect Headers + Logout

@ghost
Copy link

ghost commented Mar 24, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/3847/

@PVince81
Copy link
Contributor

@bantu @LukasReschke can you help reviewing this ?

@ghost
Copy link

ghost commented Mar 25, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/3861/

@josh4trunks josh4trunks mentioned this pull request Mar 28, 2014
Closed
@DamnDam
Copy link

DamnDam commented Mar 28, 2014

My setup changed, but I'll try and test it.
It feels a lot less hacky ^^

@DamnDam
Copy link

DamnDam commented Mar 28, 2014

What is the expected behaviour when cookies are disabled ?
I suppose Basic Authentication is especially usefull for clients with disabled cookies, does it break support for them ?

@josh4trunks
Copy link
Contributor Author

OC is usable without cookies disabled in a browser? I don't see how that would work because it never asks for basic login. I don't think this should affect clients without cookies but we should test it.

all this commit does is..

  1. not overwrite regular login with http headers
  2. ignore http headers when asking to logout

@PVince81 PVince81 mentioned this pull request Apr 3, 2014
Closed
@Niduroki Niduroki mentioned this pull request Apr 3, 2014
Closed
Niduroki
Niduroki reviewed Apr 3, 2014
View reviewed changes
lib/base.php
// Ignore HTTP Authentication for 5 more mintues.
setcookie('oc_ignore_php_auth_user', $_SERVER['PHP_AUTH_USER'], time() + 300, OC::$WEBROOT.(empty(OC::$WEBROOT) ? '/' : ''));
} elseif ($_SERVER['PHP_AUTH_USER'] === self::$session->get('loginname')) {
// Ignore HTTP Aunthentication to allow a different user to log in.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a typo here.

@Niduroki
Copy link
Member

Niduroki commented Apr 3, 2014

Tested it with my setup and works fine. 👍
This needs a rebase/merge though.

@ghost
Copy link

ghost commented Apr 3, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4035/

@josh4trunks
Copy link
Contributor Author

@Kondou-ger done.

@scrutinizer-notifier
Copy link

The inspection completed: 3 new issues, 2 updated code elements

@ghost
Copy link

ghost commented Apr 4, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4058/

@DeepDiver1975
Copy link
Member

I will perform some tests regarding shibboleth, ocs api and clients. Please don't merge until my explicit approval. Thanks a lot

@Niduroki
Copy link
Member

@DeepDiver1975 test and merge pls. kthxbye.

This screws my testing instance big time, having an untracked change in my base.php is a pain when git pulling

@DeepDiver1975
Copy link
Member

Tested:

  • Shibboleth
  • ocs api
  • web dav

@DeepDiver1975
Copy link
Member

👍

DeepDiver1975 added a commit that referenced this pull request Apr 28, 2014
@DeepDiver1975
Merge pull request #7852 from josh4trunks/basic_auth_fix
7c0340c 
Fixes login / logout when HTTP Basic Headers are avilable.
@DeepDiver1975 DeepDiver1975 merged commit 7c0340c into owncloud:master Apr 28, 2014
@Niduroki Niduroki mentioned this pull request May 6, 2014
Closed
@ColinFinck
Copy link

Same problem occurs on 6.0.4 with basic auth enabled and can be fixed by applying this patch.
So please backport to stable6 branch and release a new minor version.

@ColinFinck ColinFinck mentioned this pull request Jul 11, 2014
Closed
@t0mcat1337 t0mcat1337 mentioned this pull request Aug 18, 2014
Closed
@t0mcat1337 t0mcat1337 mentioned this pull request Feb 24, 2015
Closed
@lock lock bot locked as resolved and limited conversation to collaborators Aug 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
3 - To Review enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@josh4trunks @PVince81 @DamnDam @Niduroki @scrutinizer-notifier @DeepDiver1975 @ColinFinck @jancborchardt