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

States don't seem to work properly anymore #77

Closed
arkascha opened this issue Dec 28, 2014 · 37 comments
Closed

States don't seem to work properly anymore #77

arkascha opened this issue Dec 28, 2014 · 37 comments
Labels
Milestone

Comments

@arkascha
Copy link
Contributor

@fredl99 reported this issue in over there: #76
I took the liberty to separated the issue here into a separate entry.

The selected states don't seem to work properly anymore. Any of "closed", "private" or "shared" results in a "forbidden" message when calling the short URL, regardless if or which user is logged in. No login-page appears either. The only working status at the moment is "public".

@arkascha
Copy link
Contributor Author

Sorry, I can not reproduce the issue you report.
For me the behavior is as expected: when accessing a shared Shorty from outside an owncloud session having access I am prompted for authentication.

Indeed the authentication strategy has been reimplemented shortly ago. So far I only had positive results on that modification. So I take your report seriously. @fredl99 : Can you provide any more details / info? Especially which browser you are using?
The "Forbidden" message you receive is the result in a failed authentication attempt. So to me this smells like although requested to authenticate your browser does not ask you for authentication...

@fredl99
Copy link

fredl99 commented Dec 28, 2014

Thanks for the reply, I'm currently trying with different browsers and will report asap.

@fredl99
Copy link

fredl99 commented Dec 31, 2014

I have tried Google-Chrome, Iceweasel, Qupzilla and the Gnome-Browser (which afair is based on Epiphany). No matter what, it's the same behavior with all of these.
You're right, none of them asks for authentication. But after an already successful login to my site this shouldn't be necessary when I request a short URL within the same browser window, right?
Don't know where to start looking, could it depend on a .htaccess file? I cannot imagine that, because it only contains a rewrite-rule, and everything else works flawless, also with public Shortys. ❔

When has the authentication strategy changed, may I ask? (commit?)

@arkascha
Copy link
Contributor Author

This is the commit that reimplemented the authentication strategy:

commit d023c6d
Author: arkascha [email protected]
Date: Sun Oct 19 20:38:46 2014 +0200
Shorty: Fix relay access authorization.

The different is that the authentication strategy has been changed when an authentication is required. It uses basic http authentication now. This results in a separate modal authentication dialog displayed by the browser, not on the owncloud login dialog as before. This prevents some routing issues and is logically cleaner in my eyes. In the end such an access has nothing to do with using the shorty UI itself.

I have no idea what the problem is in your case.
From your description I understand that you are already logged in, so no authentication is required. I have no idea why the access fails in that case. (Expect if the target denies access, which would be relayed)
Please try to access a Shorty using a browser where you are NOT logged in to owncloud. best for this is to use an anonymous session, most browser offer that feature these days. Does that access work as expected at least?

@fredl99
Copy link

fredl99 commented Jan 1, 2015

It's always the same, regardless which browser or if I'm already authenticated or not. The "Forbidden" message comes immediately, except when the Shorty is "public".

I've reverted to the commit just before the introduction of HTTP auth, and it's the same. So it doesn't look like shorty's fault, but in my server config.
My OC is one level below httpd's root-dir an the shortys are created exactly there. Also I'm strictly using https. (That's why I don't like the "unsecure elements" warning in other places, BTW.)
Main: https://SERVER_NAME/oc/
Shortys: https://SERVER_NAME/oc/<shorty-id>

Funny that it works with public shortys. Apart from that it would look like trying to get a directory listing, which is of course "Forbidden". That would mean it's using a wrong path, but not with public shortys. Hmm..

@arkascha
Copy link
Contributor Author

arkascha commented Jan 1, 2015

https insecure element warning is tracked separately in issue #87 now.

@arkascha
Copy link
Contributor Author

arkascha commented Jan 1, 2015

This sounds like you are using the static backend and configured it such that it points to the folder your owncloud installation is located in. That indeed will not work.
Note that this would also lead to collisions! By chance a Shorty might get an id that is (coincidally) identical to the name of a folder or file inside the owncloud installation! The base URL for your Shortys should be separate of your installation!

@arkascha
Copy link
Contributor Author

arkascha commented Jan 1, 2015

Hm... maybe such configuration should be prevented in an explicit manner.
It never occurred to me one could try such a configuration. Classical case of "creator blindness" :-)

@fredl99
Copy link

fredl99 commented Jan 1, 2015

This sounds like you are using the static backend and configured it such that it points to the folder your owncloud installation is located in.

Correct.

By chance a Shorty might get an id that is (coincidally) identical to the name of a folder or file inside the owncloud installation!

Well, I was heavily relying on the randomness of shorty-ids and the correct behavoiur of OC-files and folders just in case. But basically you're right, I will re-configure with this in mind. Thanks for the hint!

It never occurred to me one could try such a configuration.

"Make it fool-proof and a better fool will come along." 😃

@arkascha
Copy link
Contributor Author

arkascha commented Jan 1, 2015

I separated the issue of the backend configuration in to issue #89.

Feel free to reopen this issue if reconfiguring the setup does not solve your problem :-)

@arkascha arkascha closed this as completed Jan 1, 2015
@arkascha
Copy link
Contributor Author

arkascha commented Jan 1, 2015

BTW: the Shorty IDs are not random at all! and that is important!
They are guaranteed to be unique over time throughout an installation (note: installation, not user). This is important, since ownCloud is a multi user environment. And collisions here would be a privacy issue.

@fredl99
Copy link

fredl99 commented Jan 1, 2015

Feel free to reopen this issue
Seems like I can't do that: arkascha opened this issue 4 days ago...

I'm afraid my problem continues to exist. Tried several variations, but the result is always a "Forbidden" message, when it should work logically.

While Shortys directly within the OC dir might have been a questionable idea, that alone doesn't seem to be the cause. I've tried shortys directly under doc-root as well as with non-existent sub-dirs, always with according rewrite-rules. No luck.

BTW: I took precautions against collisions with existing files/folders. One of them was a sort of "salt" by preceding the <shorty-id> with some characters, like this:
https://domain.tld/e3<shorty id> -> there is no file/dir named e3* in doc-root. Same is true with https://domain.tld/oc/e3<shorty id> (existing folder with no e3* files/folders) or https://domain.tld/e3/<shorty id> (subfolder e3/ doesn't exist).

Common facts:

  • When a Shorty is declared "public" it just works. In each of the above configurations.
  • Every other case leads to "Forbidden".
  • This is true for the relay-URL too, which is strange, because https://domain.tld/oc/public.php?service=shorty_relay&id=Tz2UA02T4h should do. The target is not restricted in any way, because when the shorty is declared as "public" it opens immediately.
  • There is no auth request, regardless of being authenticated before or not.

@arkascha
Copy link
Contributor Author

arkascha commented Jan 1, 2015

OK, that brings us forth a step!

  • you are right, your precautions work, that is a great solution! One thing to be considered as an option. So I will update issue Configuration of static backend should deny setups known to create issues #89.
  • Your "common facts" reveal this:
    The resolution or relay requests is working. This gets clear by you testing the relay URL. You get the same result, so most likely the issue is located at the same step which is after resolving the source URL. Two steps remain:
  1. access to the public service of owncloud does not work (whyever)
  2. the app is unable to resolve the request (I doubt that)

Whatever. In both cases best would be to take a look at the log files. Not sure if you have access.

  • error log files of the http server, though I doubt this will reveal anything
  • ownclouds own log file inside the data folder (https://.../oc/data/owncloud.log)
    ownClouds logging engine offers different loggin levels. But an error should always get logged.

Any entries there during a request?

@fredl99
Copy link

fredl99 commented Jan 1, 2015

Since "public" Shortys work instantly, the service must be accessible and the request is resolved. The only difference is when the state is any other than public.
So I would suspect the obstacle somewhere where a decision is to be made if the caller is authorized to access the shorty. In other words, if an authentication is valid or should be renewed. It looks like this step is missing and the answer is always "no access".

The logs don't show any useful entries. Only errors when resolution fails due to improper rewrite-rules. Interestingly enough there's no logging of succesful access, either. Also no 40x-errors, which would make sense if http-auth takes place.

BTW: Can you catch this page here with the shortlet? The URL field is flashing, until I remove or escape the #... ;-)

@arkascha
Copy link
Contributor Author

arkascha commented Jan 1, 2015

Well, there must be a log entry in the access log file about the request. Otherwise you really have an issue with your system :-)

Most likely there are no 40x errors in the log files because the error is not created by the http server, but by the app.

There most likely is an easy solution for this, but I don't see it right now with the limited information I have. This might very well be a bug, so I take this serious. But I haven't got a clue. Could you grant me access to your system somehow? Temporarily? I know.... that is always a problem. But we have to dig deeper here...

I re-checked the code again. The strategy is this: a 403 is thrown for all states expect public. This matches this issue. The purpose of the 403 is that that the browser retries the request after having asked for authentication credentials. This, because it received according http headers asking for authentication.
It would be interesting to see if such headers are sent to your browser. That is why I ask for access. You can also check yourself though: if you have a network sniffer at hand block the specific connection and dump the conversation. but since you are using https you have to decrypt. Not easy. You could use something like 'Charles' for this, but you'd have to install the server certificate inside Charles to be able to decrypt...

Sorry, too tired right now, tomorrow is another day.

Oh: happy new year :-)


BTW: I have no problems at all with the link you mention. I can use it, create a Shorty for it, using the Shortlet or pasting it directly. There is no flashing going on and the target is called when accessing the Shorty.

@fredl99
Copy link

fredl99 commented Jan 2, 2015

Happy New Year to you and family, too!

there must be a log entry in the access log

I was wondering all the time, why tail -f is so silent. Then I read this. Now it's clear. No useful debugging possible that way. I have requested error-logging now. This should help.

It's not a question of trust, but I guess at the moment you wouldn't see more than I on the server.

I can find a lot of 40x codes in the recent log, but it's hours old. I have to tidy up my list of shortys to regain some overview. My list is full of differently created id's from the last experiments. Will have to start over then.

The owncloud.log is running continous, but there are only creations logged and script errors due to invalid id's.

Thank you for the explanations, I will look at this on the weekend.

@arkascha
Copy link
Contributor Author

arkascha commented Jan 2, 2015

Hm... "script errors due to invalid id's"... might be interesting to take a closer look here. Are those Shorty IDs or something else?
The 40x codes might also be of interest, since Shorty creates valid http response codes. Firbidden is 403 for example. Oh, an afterthought: how does that "Forbidden" message look like? Is it a typical browser generated error page (plain black text on a white background), or is it an owncloud view that displays the error?

But in the end more interesting would be to step through the relay code and see what's happening. Either using a debugger or at least using debug output steps.

@fredl99
Copy link

fredl99 commented Jan 2, 2015

The invalid IDs were caused by different configuration variants. When I changed the path, the formerly created IDs became invalid. Hence the errors.

The "Forbidden" message is in a typical owncloud screen.

Here's a log entry from a "shared" shorty:

[02/Jan/2015:01:25:55 +0100] "GET /oc/public.php?service=shorty_relay&id=Tz2UA02T4h HTTP/1.1" 403 7889 "-" "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/39.0.2171.95 Safari/537.36"

That's interesting: There's not even one 401-code created by shorty. But from remote.php/carddav, so OC itself can produce 401s. Afaik 401 should go out if auth is required.

BTW: Wikipedia says about 403: "Die Anfrage wurde mangels Berechtigung des Clients nicht durchgeführt. Diese Entscheidung wurde – anders als im Fall des Statuscodes 401 – unabhängig von Authentifizierungsinformationen getroffen, auch etwa wenn eine als HTTPS konfigurierte URL nur mit HTTP aufgerufen wurde."
Hmm... Could it be...?

@arkascha
Copy link
Contributor Author

arkascha commented Jan 2, 2015

OK, that makes some sense. If requests to Shortys defined in older configurations are not rewritten correctly any more, then it is not Shortys relay handler that is called with the correct Shorty ID, but something else, typically something undefined, which might well result in a forbiddden error in ownCloud. In taht case the effect is not caused by Shorty.
The fact that you do not see any 401 results, nor get an authentication prompt on the browser side hints into the same direction.

So in my eyes this has nothing to do with Shorty, nor with http/https which works fine for me.

What I'd like to know now: what with freshly generated Shortys using a correctly configured static backend. Same behaviour? Then my first guess would be that the configuration is not correct. Two ways to tell:

  1. the configuration of the static backup offers the option to test the current configuration. Does that give a success?
    2.) create a bad syntax error in the file /relay.php. Do you get an error (in the logfiles) when accessing a Shorty URL now? If not: the relay is _not_used, so again: should be a configuration issue.

@fredl99
Copy link

fredl99 commented Jan 2, 2015

Does that mean short URLS should be re-created when the base-url changes? There should't be any shorties with, say e3<id> in the table after changing to e3/<id>? If so, then something went really wrong here.

Neither old nor new shortys work. The old ones have obsolete IDs, so they cannot work. New ones only work when they're "public". All, old and new relay-URLs work, but only if they're public. And that's what confusing me here. Why would these work when your theory is right?

The testing tool does not succeed. I haven't noticed that until now, because it always took a while until it finished. My guess is, all these problems came in with the update from 7.0.3 -> 7.0.4. It definately worked with 7.0.3, but reverting back did not help.

So what would be a death-proof setup to start all over in your opinion?

@fredl99
Copy link

fredl99 commented Jan 4, 2015

Ok, did some further investigations 🚶

What we have:

  • public shortys work everytime -> relay.php is called -> rewrite-rules work
  • blocked shortys end in a "Forbidden"-screen by OC -> correct behaviour
  • private or shared shortys end in a "Forbidden"-screen by OC, regardless if the user was authenticated before by OC/Shorty or not -> wrong:
    looking at the source, I see
    • depending on the shorty's status there should be two checks:
    • is the user logged in?
    • is the logged-in user allowed to get the shorty?
  • if either one is false then there should be a http-auth, which never comes. As a result it ends in the default action "Forbidden".

To me there are two things obvious now:

  • the response code of 403 is not appropriate, because even if a new http-auth header would be prepared and (maybe) sent to the client, access is denied in the first place regardless of the credentials provided by the client. (See quotation of Wikipedia some comments ago)
    • For a quick&dirty hack I changed the status codes to 401 at the five according positions. Now there's a http-auth when a shorty is requested for the first time, followed by the correct action. (Access granted or not, depending on the authenticating user and the state of the shorty).
      Subsequent request are denied or allowed depending on the shorty's state alone until the browser is closed. That's normal, because the http-auth is valid for the entire session, so it won't be repeated.
      But also the user is authenticated for OC from the first http-auth on. Drawback: It's impossible to log out of OC, because the http-auth will log-in over and over.
  • checks against the logged-in user either don't happen or fail for some reason, because otherwise an already logged-in user should get access to a shorty that either belongs to him or is shared with him, without http-auth. This is still not working, but I won't try further here as long as I'm not familiar with the different variables and functions provided by owncloud/core.
    As far as I can see, OC_User::isLoggedIn() and OC_User::getUser play a role here.

@fredl99
Copy link

fredl99 commented Jan 5, 2015

@arkascha could you please re-open this issue? I don't have that option.

@arkascha
Copy link
Contributor Author

arkascha commented Jan 5, 2015

You are obviously right, that some of the 403s are completely wrong.
No idea why I coded that, guess it was a c&p issue whilst cleaning code...

Anyways, I made some changes that look better to me, but unfortunately I currently do not have time to test that as required. Do you want to give it a try?
The problem: without testing I do not want to integrate that into the main owncloud/shorty repo. That repo only serves for finalizing and publishing for me. The development is done in a cloned repo in my own account instead, as typical on github. So you'd have to clone that repository instead of the "official" owncloud/shorty repository. That repository in my account is also where the changes mentioned here are to be found. From here I merge tested and validated changes into the main repository:
https://github.com/arkascha/owncloud-shorty

This would also have another advantage: you could give it a try and make such changes yourself, if you want to. I'd be happy to grant you write access! :-)

@arkascha arkascha reopened this Jan 5, 2015
@fredl99
Copy link

fredl99 commented Jan 6, 2015

Ok, @arkascha
I've cloned your personal repo now. I guess it's better to discuss specific issues there?
Is it allowed to open tickets in German language?

@arkascha
Copy link
Contributor Author

arkascha commented Jan 6, 2015

To keep things transparent always post and track issues at the "offical" repository. Since owncloud is an international project using English as communication language does make sense. On the other hand a German feedback or ticket is better that none :-)
To discuss things in detail you are welcome to contact me directly via email too, the address is enclosed in all files. But keep in mind that transparency is a vital aspect for open source projects.

It is a very typical thing to use private repositories to prepare and evaluate spikes and bug fixes before merging them into the official repository. That allows to keep failed attempts out of the project history.

@fredl99
Copy link

fredl99 commented Jan 6, 2015

I just thought to discuss things where they happen. But when it's common practice then naturally I will accept the rules.


Back to topic:
As I can see, you implemented my suggestion about code 403 vs. 401. As already mentioned, the http-auth is now taking place. But after that there's no way to log out of OC without closing the browser. I consider this a serious issue. While it's only annoying at a personal workstation it's unacceptable at a public computer, where the user may not be able to do so.
That leads me to the question: Why was the http-auth implemented some time ago? Please excuse that question, but I'm not so familiar with the internals of OC and Shorty.

Of course I see that somehow it has to be decided if a client's request for a particular shorty is valid. But as a bottom line there are only three options:

  1. the shorty is public -> go ahead.
  2. the shorty is blocked -> send out 403 (by OC or the server, which I personally would prefer for different reasons)
  3. the shorty is shared or private -> check, if the user is already authenticated by OC
    1. no -> call OC's auth routine and proceed next
    2. yes -> get the OC username.
    3. a shared shorty can be delivered now (as long as there's no function to share only with some users)
    4. if it's a private shorty, the OC username must match with the shorty's owner to be delivered. Otherwise send out 403 (this time by OC as it makes sense).

In my opinion the HTTP-auth interferes with OC's auth, or am I missing something?

@arkascha
Copy link
Contributor Author

arkascha commented Jan 7, 2015

First of all apologies for not replying and coding faster - another project requires my attention.

About the authentication strategy:
I ran into a number of problems when relying on the OC authentication mechanism. I do not remember the details right now, but there were issues with query parameters not getting handed over during the OC authentication. At least at the time I made the switch...
Actually I still think that keeping OC sessions and access to Shortys (so using the relaying service) are two completely separate things. Thus the sessions should be separate.

At the time of testing http authentication to an OC session was not possible. Or so I thought. There actually is an app that implements exactly that, because it is (was) missing in the OC core. But now indeed my short test indeed confirms your observation. This indeed is a problem...
When implementing the http based authentication I took care NOT to create a valid OC session. So I did not implement a login using the provided credentials, but merely checked the password in a raw manner. Nevertheless I now see that you are right.

sigh

@arkascha arkascha added the bug label Jan 10, 2015
@arkascha arkascha added this to the shorty-0.5 milestone Jan 18, 2015
@arkascha
Copy link
Contributor Author

I just pushed a reimplementation of the relaying strategy, authorization and forwarding aspects.
This appear to be working, but I had to trick the OC core with the authorization. There appears to be a strange regression with session verification in the OC core. I will address the core developers tomorrow, got a few questions anyway.

@fredl99 Wanna give it another try?
I followed your advice to switch back to form based authentication...
Also I implemented an additional forwarding page informing the user what happens. This is new. I know this means an additional view and an additional click. Still I think it makes sense. I received a few complaints in the past about privacy and security threads caused by url shorteners which are not totally invalid in my eyes. Transparency is important and shorteners typically do not offer that but take you somewhere without giving you a way to stay in control. This additional page offers such control.
The layout is not final. I am currently unsure what information should be shown there. Feedback welcome as always.

@fredl99
Copy link

fredl99 commented Feb 23, 2015

I just tried.
When I enter a short URL into the address bar, I get a completely blank page.
The log says:

{"app":"PHP","message":"Class 'ChromePhp' not found at /path/to/shorty/relay.php#181","level":3,"time":"2015-02-23T12:26:18+00:00"}

There is a login screen or not before, depending on the auth state. But the result is the same.

@arkascha
Copy link
Contributor Author

ooops, sorry for that. Debug output statements that should not have been pushed. As said: work in progress.
I fixed that, just pull again.

@fredl99
Copy link

fredl99 commented Feb 23, 2015

😄 OK!

tested:

  • the interception page looks great 👍
    (except that the old issue with special characters in the title field is inherited from the settings)
  • blocked shortys are "forbidden" right away ✅
  • public shortys work without auth ✅
  • shared shortys work with required auth ✅
  • if an auth was required for access then logging out of OC is possible and working afterwards ✅
  • private shortys require an auth and work, but: it doesn't matter who logs in 🔴
    In other words "private" is handled as "shared"

Aside from that: As always, well done! ➕

@arkascha
Copy link
Contributor Author

Fixed the issue about private Shortys. That indeed was a logical twist I implemented :-(

@arkascha
Copy link
Contributor Author

@fredl99 OK, three issues left here:

  • your umlaut problem - could you please separate that into a new Shorty if that is an issue? I do not really find that you mentioned that before, sorry. I only remember a single URL you made a Shorty for which had indeed an invalid encoding. But that was nothing Shorty can fix. Is there some other Umlaut/encoding issue?
  • when getting denied after a successful login (private Shorty) one receives a 403 now (denied). This makes sense, but it does not allow to retry using a different login right away. Not really sure if that actually is an issue: it would only be relevant for a person having more than a single account inside the same owncloud installation. Which is not very likely for a normal user.
  • there is a strange redirection issue I still do not understand. Actually it could be an issue with my local setup. When I fail to login one or more times for a private Shorty and then succeed I am redirected to a non-existing url: the owncloud base directory is repeated inside the url and the quoted separators look funny: http://localhost/owncloud-8.0.0/%25252Fowncloud-8.0.0%25252Fpublic.php%25253Fservice%25253Dshorty_relay%252526id%25253DNkPaabhQaq
    Looks more like a core issue to me, but I did not yet find the cause.

@fredl99
Copy link

fredl99 commented Feb 24, 2015

Regarding the Umlauts, I only mentioned it here because it appeared on the new warning page. Of course it belongs to the other thread.

Code 403 for a private Shorty makes sense, I agree.

A also had the strange encoded address line withe the doubled directory name. First I thought about a wrong rewrite, but this couldn't alter the encoding. As it only happens during the login sequence, I think you're right. Pasting the correct link into the field again does work anyway, when the auth was successful before.

@arkascha
Copy link
Contributor Author

OK, just talked to Lukas and he confirmed the issue with the scrambled redirection URL.
He says it is currently getting fixed in OC-core master. So there is nothing I can do.

@fredl99 I guess this means this one can be closed?

@fredl99
Copy link

fredl99 commented Feb 24, 2015

Basically the initial issue seems resolved. So this long-term topic could be closed, yes. (I cannot do that, because you opened it.)

I was just thinking if there could be a way out of the one-way street when a user logs in for a Shorty which isn't available to him. Because the "forbidden"-screen without any way out is not so cool. :)
A button to take him to his OC-home (since he is already authenticated) or anything else. It would only have to be shown to registered users, not to the unknown.

Of course that'll be a nice extra, but not a requirement.

@arkascha
Copy link
Contributor Author

Closing for good...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants