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

On 6/16/2014 12:12 PM, Zhou Zheng Sheng wrote: > on 2014/06/15 00:18, Aline Manera wrote: >> On 06/13/2014 04:12 AM, 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. >> I am not aware about ANY discuss you have just mentioned. >> ALL discussion related to patches/features **MUST BE DONE IN THE MAILING >> LIST** to the >> whole team be aware about it! >> >> And in a V5 patch, you have more than enough time to do it. >> > Yes. The problem is that after a local discussion we reached an > agreement and I trusted Shao He and Yu Xin who were actively work on > this should have reflected the discussion result either in the patch or > in the mail. Sorry for not sending the discussion to the mail list. I'd > send my opinion to mail list every from now on. The agreement is that it worth a try, not as a final design.

> >>> 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. >>> >>> 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. >>> >>> 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. >>> >>> 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. >> If I applied the patches I reviewed it! >> > You are the boss ;-). However even if you thought the solution is > acceptable, there was some new code in the v5 patch and it's worth > getting reviewed by other developers. > >>> 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? >> An RFC named as V5? >> A V5 patch set is to "be so hurry" for you?? >> > Yes. It's so hurry. The fact is the solution of redirecting user back to > previous page is not mature and I was strongly against it. As I said, Yu > Xin and Shao He also thought it's bad. The v5 patch does not improve > this at all. In fact I'd be curious on how it didn't get improved > through 5 series of patches.There was also no investigation on how other > sites does this. In my opinion, if it's not mature, we can split out the > redirection part, and apply the rest part, so that it can be improved. I am not thinking it is bad, till now, I am still thinking cookie is the only way to support all the scenarios we want to support. > > By the way, I think the version count does not directly related to the > completeness of a patch series. If it does not reach a certain point of > maturity, we should improve it in new version. In other projects it's > usual to see a patch gets updated to v8 v10 or even more. For example: > > [Qemu-devel] [PATCH v8 00/20] virtio endian-ambivalent target > [Qemu-devel] [PATCH v10 1/6] block: round up file size to nearest sector > [libvirt] [PATCH v8 0/4] Handling of undefine and r e define snapshots > with VirtualBox 4.2 or higher > http://gerrit.ovirt.org/#/c/28088/ v12 Final separation of IOProcess > and RFH If you would like to reference engineering statistics from other projects, please get their average patch version or the whole open source field's average patch versions. You intentionally to reference their several big version patches, it will mislead the whole team.

> > For this particular patch series, I think the redirecting part is very > ugly, the rest part is good. At least we can split out and merge the > rest, and improve the redirection mechanism. Consider this patch series > contains lots of mechanism changes, it's normal to update more than 5 or > 6 times to get it ready. I did not get any justification at all to pursue to try to get a patch version as big as possible.

> >>> 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. >> If you think you solution is better, send the patch! >> > It's always cheap to change things in the design phase, then development > phase. Once it's merged, new code starts to rely on it, and things would > get harder to changes. I already expressed my opinion to the developers, > and the redirection does not get improved even the patch was updated > through 5 series. I'd always happy to help, but I'm lack of some > front-end knowledge to do the UI part. However, don't knowing how to > make a film does not mean that the audience can not give comments on the > quality of the film. The same applies to patches. If you always say "If > you think you solution is better, send the patch!", that's destroying > the basis of code review. If your logic takes hold, there should be no > code review at all, and to do one thing right, we just submit patches by > patches and go in circles. Welcome your comment, please focus on the overall solution. Parts of the solution are inter-connected, pieces can not be separated. we always discuss pieces with the overall solution in mind, please also. Current design is as below: 1. user first time access a protected resource, redirect login page. 2. when session timeout, redirect user to login page with a message "session timeout, please re-login". 3. if anytime a wrong user/pass is provided, redirect back to login page with a message "username/password wrong." 4. if user login successfully, always redirect to the url that he intend to go originally.

if you are truly interested to be involved, please think about the overall solution totally when you are challenging a piece. You do not need to code everything out so client side skill is not an excuse.

> > The problem is that the authors of this patch series neither did any > investigation on how other sites do login redirection nor tried any of > the proposed alternatives. If there's something we all know it's > important, but the current solution is evil, and the same thing has been > solved by other WEB sites already, why we persisted on this solution and > merged it so quickly? Before layout your own overall solution, please do not speak with assumption.

> > If we consider an open source project as a product, the code quality is > an key point of this product, since its internal opens to everyone. > Merging a patch is a declaration of admitting that the code quality > reaches the common quality of this project. Yes, we can always improve, > but, shouldn't we have some bottom line? This is a design discussion, it has nothing to do with code quality, the current code support the design well.

> >>> 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 >>>> >