cross-solution / YAWIK

YAWIK is a web application. It can be used as an ATS applicant tracking system or as a jobboard.
https://yawik.org
MIT License
125 stars 67 forks source link

404 after login #200

Closed fedys closed 8 years ago

fedys commented 8 years ago

user who has not value set in users.settings.localization.language get HTTP 404 right after successful login to YAWIK. User ends on http://yawik.local/cs for czech locale for example.

TiSiE commented 8 years ago

The 404 only occurs if the user does provide a non-supported language in his Accept-Language header.

fedys commented 8 years ago

You are right. In such cases, an user might be redirected to the application default language.

TiSiE commented 8 years ago

Well, the problem is, the Login process takes only the first accepted language and issue an redirect without checking wether it is supported. The LanguageRouteListener catches the request, but DOES NOT redirect to the appropriate language. It detects the appropriate language and displays the 404 page accordingly though.

Imho the LanguageRouteListener should do a redirect in that case, so we do not need to duplicate the detection logic.

In the login process, we simply redirect to a invalid language. That leads to 2 redirects in one request, but will work and is relatively simple to implement.

fedys commented 8 years ago

@TiSiE Your idea seems to be fine. Feel free to assign me this task.

TiSiE commented 8 years ago

@fedys Sorry, I've already fixed...

fedys commented 8 years ago

@TiSiE No problem :). Unfortunately, I realized the solution you suggested and implemented is not the ideal one. Let me explain why. Now, if you enter an arbitrary URL with non-supported language, then you get an HTTP response 302 with location containing a supported language. But this is not a proper behavior. You should get an HTTP 404 instead.

Examples: https://yawik.org/demo/xx => https://yawik.org/demo/en (it should be 404) https://yawik.org/demo/xx/non-existent => https://yawik.org/demo/en/non-existent (it should be 404)

Your fix solves a consequence not a cause actually. The proper fix should avoid redirecting a user to non-supported language.

TiSiE commented 8 years ago

First example: Why should it be 404? The requested content is available, just in another language. HTTP/1.1 spec:

302 Found The requested resource resides temporarily under a different URI.

Maybe some time in the future, the language will be supported.


Second example: The first redirect will be a 302, but accessing the URI https://yawik.org/demo/en/non-existent will indeed issue a 404.

It is a bit cumbersome maybe, but didn't require a big refactoring of the - admittedly awkward - implementation of the language detection stuff.

fedys commented 8 years ago

IMHO it should be 404 in both cases ('xx' is not even a valid language ISO code). However, I would left the implementation as is now.

TiSiE commented 8 years ago

Maybe we should meet and sort this out like real men do ....

... with "Rock, Paper, Scissors" :smirk:

cbleek commented 8 years ago

Am 22.07.2016 um 11:28 schrieb Miroslav Fedeleš:

IMHO it should be 404 1+

fedys commented 8 years ago

@TiSiE this evening in Frankfurt? :laughing:

TiSiE commented 8 years ago

@fedys I'm waiting! ;)

But I changed my mind. Creating a LocaleDetector service and throw away the whole redirect stuff makes the LanguageRouteListener much cleaner.

So .. yay - 404 for the world! :smile:

fedys commented 8 years ago

@TiSiE It sounds good. Do you want to implement it yourself or want to delegate it to me?

TiSiE commented 8 years ago

Maybe we should use https://github.com/juriansluiman/SlmLocale?

fedys commented 8 years ago

I would keep it simple for now. Just refactoring to the mentioned LocaleDetector. I would defer adding the SlmLocale dependency to a new issue.

cbleek commented 8 years ago

@fedys : We've shortly discussed it. SlmLocale ist not a good idea. So please implement it the way you suggested.

fedys commented 8 years ago

@cbleek Okay, please do not forget to assign this issue to me ;)