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

On 6/16/2014 11:33 AM, Zhou Zheng Sheng wrote: > on 2014/06/13 18:47, Yu Xin Huo wrote: >> On 6/13/2014 6:07 PM, Zhou Zheng Sheng wrote: >>> 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. >> there are 2 alternative solutions you proposed: >> >> 1. brower history >> -- unreliable as I replied. > 1. You are correct > >> 2. internal redirect >> -- 1. make browser url mismatch its content, it is >> desirable to make browser url truly address it intended content. >> 2. after login, server response back a refresh.html >> with javascript to reload browser, this refresh.html need to be pasted >> to browser to get that javascript to run. >> user will definitely perceive this page switch, >> make it a bad experience. >> 3. Compare with a single cookie attribute, there >> are >> must more side effects. > 2. There are plenty of ways to improve internal redirect method and > eliminate the need for "refresh.html". > > Option 2.1. Upon unauthenticated access, back-end internal redirects to > "login.html". The POST action of the form send the current page URI to > the back-end as well, then after the /login URI handler authenticates > the user, it fetches the "Referer" header of the request, and this > header contains the original page URL, then back-ends uses internal > redirect or HTTP 3xx redirect to present the original page content. "Referer" does not work. Please google search or try it yourself.

> > Option 2.2. Upon unauthenticated access, back-end internal redirects to > "login.html". The form in the "login.html" contains a hidden input > indicating the current URI. Then the JS in "login.html" fills this > hidden input upon successfully loading the page. So the POST action of > the form send the current page URI to the back-end as well, then after > the /login URI handler authenticates the user, it uses internal or HTTP > 3xx redirect to lead the processing flow back to the original URI. If the first time I try to access kimchi with "https://xxx.xxx.xxx.xxx:8001/login.html", what will happen?

> > Option 2.3. Upon unauthenticated access, back-end internal redirects to > "login.html". "login.html" does not use traditional HTTP POST form, > instead, it uses /login API call to login using AJAX invocation, upon > successful login, "login.html" refreshes the page. how to differentiate first time login and session timeout?

> > Actually "softlayer.com" is using some means of internal redirection to > guide user to the previous page. You can try to open the following links. > https://control.softlayer.com/account/invoices > https://control.softlayer.com/account/orders > You'll notice it returns a plain login page, after login, it triggers a > refresh that the user doesn't notice at all.

> > The page switch only happens in login process. Though you state cookie > approach as "single cookie attribute", but it gets transferred in every > request and response even the AJAX ones. It's obvious which is more > evil. There is also an inevitable bug in cookie approach. Say the user > opens two firefox tabs, one is for VMs, the other is for StoragePool. > Once the session is expired, and the user login in VMs, it may lead the > user to StoragePools if StoragePools happens to be opened after VMs. Quite easy to be solved, I believe there will be an event notification when switching tab.

> > Moreover, I didn't see any investigation on how other WEB sites does > this. Though this is not my task, I just investigated softlayer to prove > my point. I suggest anyone who works on this firstly do some research on > the existing WEB sites and avoid re-inventing the wheel. Kimchi has its own problem and solution, others can be referenced, but not defined by them.

> >>>> 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. >> Cookie will always be sent back and forth regardless of our previousPage >> url. >> What trouble you mean when multiple browser tabs, please clarify. >> >>>>> 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 >>>>>>>> >> >