cozy / cozy-proxy

This repository was part of CozyV2 which has been deprecated - Cozy authentication and routing layer
https://blog.cozycloud.cc/post/2016/11/21/On-the-road-to-Cozy-version-3
GNU Affero General Public License v3.0
26 stars 31 forks source link

Add accounts step in onboarding #334

Closed gregorylegarec closed 7 years ago

gregorylegarec commented 7 years ago

This PR :

Important

To send credentials to server, our fetch call need to have the credentials option set to true. Otherwise the result of the call is a 401 Unauthorized. It has been pretty long to understand this part and Passport mechanisms.

To do

There is still a case to deal with : when user stops registration in accounts step and come back a later. If he is not authenticated anymore, the onboarding will go in a infinite redirection. We'll just have to be able to redirect to login page if user has completed password step, and then redirect to last steps of onboarding once user has logged in.

CPatchane commented 7 years ago

What do you mean by "authenticate user as soon as his password is recorded"? Did you expect to have the login page just after the password saving in order to authenticate the user? If yes, I don't have this behaviour on my own so maybe I did something wrong...

If not, actually the password step currently redirect to the next step (Infos/Accounts) without asking user authentication, but I suggest that should redirect to login?next=/register after password saving. That will automatically require authentication before continue to the next onboarding steps.

To handle this way, we need some changes but here are my suggestions that I tested and which seem to work in my environment: These changes is maybe out of the scope here but will also complete your todo point.

# Returns true if the user data contains enough informations fotr cozy authentication
User.isAuthenticatable = (userData) ->
    if not userData?.onboardedSteps or userData.onboardedSteps.length < 3
        return false
    hasCompletedWelcome = userData.onboardedSteps[0] is 'welcome'
    hasCompletedAgreement = userData.onboardedSteps[1] is 'agreement'
    hasCompletedPassword = userData.onboardedSteps[2] is 'password'

    hasCompletedFirstThreeSteps = hasCompletedWelcome and \
        hasCompletedAgreement and hasCompletedPassword

    hasPassword = userData and userData.password and userData.salt

    return hasCompletedFirstThreeSteps and hasPassword
# If authenticatable but not registered yet
# -> need authentication to continue next registration steps
if not req.isAuthenticated() and \
    User.isAuthenticatable(userData) and \
    not User.isRegistered(userData)
        res.redirect '/login?next=/register'
# if already registered -> login to cozy home
else if User.isRegistered userData
     res.redirect '/login'
# access onboarding, here the user is not register and:
# - either he is authenticatable and will be authenticated
# - either he is not authenticatable (first three steps)
else
    if userData
        ...
if not User.isRegistered(userData)
    if not User.isAuthenticatable(userData)
        return res.redirect '/register'
    # avoid looping between register and login redirection
    else if req.url isnt '/login?next=/register'
        return res.redirect '/login?next=/register'

It's just an idea here, maybe there could be a better solution. What do you think about?

gregorylegarec commented 7 years ago

Thanks @CPatchane for your great feedback, I updated the PR with your additionnal code dedicated to login screen when session has expired. Please notice that I updated the code in User.isAuthenticatable to deal with only one "last unauthenticated step" to avoid dealing with too much step information in the method. What do you think about this refactoring ?

CPatchane commented 7 years ago

@gregorylegarec That's great, I totally agree with this way to keep the code as maintainable as possible 👍

CPatchane commented 7 years ago

Thanks @gregorylegarec !