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

[Zhou Zheng Sheng]

on 2014/06/16 16:34, Yu Xin Huo wrote:
> 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.

If you didn't try, how do you know which is better? If I remembered
correctly, you agreed to try it and agreed that the current solution was
too dirty.

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

No, my point is not that we should update so many version for each
patch, instead, the point is for big mechanism change, a large version
count is not rare. Just v5 does not gives it the justification to be
mature, only the test discussion and code review on the code and
solution does.

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

I did not said at all you should 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.

No. I don't see there is any trouble to split this piece out. You can
always redirect the user back to the /host tab as a temp solution, then
improve the redirection solution.
> 
> 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 fact is that I can only code the back-end, and my proposed solution
involves front-end coding. If I didn't get help from the front-end, how
can I submit patches? You didn't give a try on the alternative solution.
If you did, I'm sure I can help with the back-end things.

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

I don't see any trouble of my proposed redirection solution to leave
with the current one.

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

Sorry for the misleading. By saying "code quality" I meant to say the
design and code. I think my argument still takes hold if you consider
the design.

>>
>>>> on 2014/06/13 05:50, Aline Manera wrote:
>>>>> Applied. Thanks.
>>>>>
>>>>> Regards,
>>>>>
>>>>> Aline Manera
>>>>>
>>>>> _______________________________________________
>>>>> Kimchi-devel mailing list
>>>>> Kimchi-devel at ovirt.org
>>>>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>>>>>
>>
> 
> 


-- 
Zhou Zheng Sheng / 周征晟
E-mail: zhshzhou at linux.vnet.ibm.com
Telephone: 86-10-82454397