Re: [Kimchi-devel] [PATCH V5 0/5] Switch to a traditional login flow

on 2014/06/13 17:57, Yu Xin Huo wrote: > On 6/13/2014 5:45 PM, Zhou Zheng Sheng wrote: >> So your point is the proposed alternatives is uglier than the current >> one, and the current one can be improved. >> >> Firstly from the JS accessing browser history link you gave me, I don't >> see anywhere it says it's unreliable. The solution doesn't need to >> inspect the history, it just back needs to go back. >> >> Secondly you think "refresh.html" is uglier than cookie. It's actually >> not. A "refresh.html" only affects the login process, but the current >> cookie solution affects all requests and responses that is not related >> to login. The current solution works, but it is far from works well, and >> if you want to improve it by suppress sending the previous page cookie >> in AJAX requests and response, more hacks would be added in both >> front-end and back-end. This is not improving step by step. To improve >> step by step, the original direction should be correct and effective, >> but this is not. > Again, there will always cookie back and forth between browser and web > container. It is handled automatically by browser and web container. > What make you uncomfortable is that piece of code at server side to get > previousPage. Yes, but the previousPage cookie is unnecessary regardless if it's handled automatically or not. The alternative solutions we discussed can avoid this unnecessary handling, and restrict the login handling logic only to a small region of code and runtime process.

> Both client javascript and web container can set cookie. > the previsousPage can also be set with javascript in client side, > each time user go to a new tab, there is a callback, in the callback, > previousPage can be set into cookie. But the previousPage cookie is still carried forth and back in normal requests, which is absolute redundant information, not to mention the troubles it causes when the user opens two firefox tabs accessing different page of Kimchi.

>> on 2014/06/13 17:24, Yu Xin Huo wrote: >>> First of all, what you are complaining about is a pretty, pretty small >>> fraction of the overall login solution. >>> For that fraction, the design is to back to the original url after user >>> login from a session timeout. >>> Shaohe's current implementation works well. what zhengsheng means is >>> about how to improve it. >>> >>> On 6/13/2014 3:12 PM, Zhou Zheng Sheng wrote: >>>> If I remembered correctly, Shao He, Yu Xin and me had several long >>>> talks >>>> on the login design. This solution is not the solution we agreed, and >>>> all of us thought that this solution is ugly and obviously should be >>>> improved. >>> We have ever discussed several options, but we have not converged to a >>> certain one. >>> Let me continue to read to see what the 'ugly' you mean. >>> >>> This solution works very well, it can always be improved. >>>> The problem is on how it redirects the user back to the previous page >>>> after login. In this patch, the back-end has to intercepts each access >>>> to any of the ".html" page, and sets >>>> cookie['lastPage'] = current page URI, >>>> and return it to front-end, then the front-end sends this cookie to >>>> back-end in every query, including AJAX query. When the session >>>> expires, >>>> the back-end redirects the front-end to a login page, after login >>>> successfully, the back-end gets cookie['lastPage'], at last, redirect >>>> the user to the last page. >>>> >>>> Just to implement returning to the previous page afte login, the >>>> back-end has to intercept each '.html' access and sets cookie, and the >>>> front-end has to send the cookie in each request including AJAX ones. >>> Handling cookie back and forth is done by browser and web container. >>> There will be always cookie, this is not an additional overhead. >>> What shaohe does is only to get the current tab url. That is a quite >>> small amount of code. >>>> So in the last talk we agreed that a simpler and more effective >>>> solution >>>> should be used. We at least have two alternative solutions. >>>> >>>> 1. When session is expired, we redirect the user to login.html. After >>>> login successfully, the JS script in the front-end asks the browser to >>>> go back to the previous page. Since the browser keeps a stack of page >>>> histories, it should be easy to do this. >>> This is server side redirect after form is submitted, at that time, >>> client side has no code to run. >>> Server side redirect browser directly to last page in login response, >>> quite straight-forward. >>> >>> I do not think brower history will be a reliable solution. >>> http://www.w3schools.com/js/js_window_history.asp >>>> 2. When the back-end detects the session is expired or the user hasn't >>>> login yet, it uses internal redirect to present the "login.html". From >>>> the front-end point of view, an unauthenticated access to "GET >>>> #tabs/vms.html" returns "login.html". After the user input his/her >>>> password in the "login.html" and click "login", the back-end receives >>>> the request, if the password is correct, it returns a >>>> "refresh.html". In >>>> "refresh.html" there is actually a small JS code to ask the browser to >>>> refresh the page. Since we are using internal redirect all the time, >>>> the >>>> page URI in the browser remains "#tabs/vms.html", so after the login, >>>> just refreshing the page would lead user to the real "vms.html.tmpl". >>>> >>>> In the above two solutions, no ugly cookie is needed for each request >>>> and response, and the back-end doesn't have to intercept each ".html" >>>> access, but just has to intercept each unauthenticated access. >>> internal redirect will make browser url mismatch with the content. >>> the current design will always keep url address its intended content, >>> this is a virtue over internal redirect we should pursue. >>> >>> The ugly cookie is removed, but introduced *a much uglier* >>> "refresh.html" and code in that html. >>> To me, it is much more complicated. >>>> I don't know why Yu Xin and Shao He sent the to-be-abandoned solution >>>> again to the mailing list. Patches were sent on 20:00 Chinese local >>>> time, the patches got merged in 05:00 Chinese local time in the next >>>> day. There is no other developer gets CCed. There is no reviewed-by. >>> This is not to-be-abandoned solution. Adam, Aline, Shao He and I have >>> discussed this in a team meeting. >>> It is sent to mail list. all people subscribed to mail list should >>> get it. >>> It is already V5, it is reviewed enough. >>>> After talked to Shao He this morning, he told me that we determined to >>>> defer this feature/task to seek a better solution. Shao He told me that >>>> they sent the patch as RFC, not aim to be a final solution. However it >>>> is a big misleading to other developers because there is no RFC in the >>>> patch title. There is even no reviewed-by. Is there any reason to merge >>>> it so hurry? >>> This is already V5, how can it be an RFC? >>>> If there was any time and task pressure, I think as an open source >>>> project, the progress should have some flexibility. We should not write >>>> code for a known broken solution, while there is obvious alternative >>>> solutions. This is very different from incremental development. In >>>> incremental development, the direction and the solution is correct, we >>>> just completes the missing pieces step by step. In this case, the >>>> solution and the framework itself is not so effective. Once it's >>>> merged, >>>> we started to rely on this, and changing and improving it would be much >>>> harder. >>> Speed will always be most key to succeed for any organization. >>> There is nothing broken, it works well. >>> Improvement will never stop. >>> >>> Again, the overall solution is discussed across whole team in one of the >>> Wed's team meeting. >>> Cookie is the best way to store previousPage to reserve user context. If >>> anyone see a better way than cookie for this issue, welcome to discuss >>> with me. >>> >>> As zhengsheng point out, we need to try to improve that small bit of >>> code to store previousPage into cookie. but again, it works well >>> currently. >>> But obviously, that is far from a justification to stop this patch from >>> being merged. >>>> on 2014/06/13 05:50, Aline Manera wrote: >>>>> Applied. Thanks. >>>>> >>>>> Regards, >>>>> >>>>> Aline Manera >>>>> >>>>> _______________________________________________ >>>>> Kimchi-devel mailing list >>>>> Kimchi-devel(a)ovirt.org >>>>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel >>>>> >