-
Notifications
You must be signed in to change notification settings - Fork 97
Improved ways to link accounts #17
base: master
Are you sure you want to change the base?
Improved ways to link accounts #17
Conversation
Updated versions to match the latest development environment. Signed-off-by: Hara Kang <[email protected]>
Since the first implementation, token verification functionality was added to Kakao API. This API prevents spoof attack by allowing apps to check if the token is indeed issued for themselves. Signed-off-by: Hara Kang <[email protected]>
Exhibit better ways to link custom oauth providers for users already signed with built-in providers. The current best way to do so is organized in the github issue FirebaseExtended#10 below: FirebaseExtended#10 Admin.auth.Auth.setCustomUserClaims(uid, customUserClaims) was added for setting custom data for control access. TODO: As mentioned in the above issue, clients should ensure that the same user is previously signed in with built-in providers (facebook or google), but this is omitted in the sample. It is each developer's responsibility to ensure this for security. Signed-off-by: Hara Kang <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for starting this :) To the best-practice linking flow we'll need to do a few extra things (including some work on the Client).
params['photoURL'] = photoURL; | ||
} | ||
console.log(`creating a firebase user with email ${email}`); | ||
return firebaseAdmin.auth().createUser(params); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you are creating a user with Kakao you should mark it as being a Kakao user using setCustomUserClaim({lineUID: lineMid})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you are right. :) Added the logic.
kakao/KakaoLoginServer/app.js
Outdated
if (email) { | ||
updateParams['email'] = email; | ||
console.log(`fetching a firebase user by email ${email}`); | ||
return firebaseAdmin.auth().getUserByEmail(email) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there some cases where the email is empty? In this case I think we'd need to check that first. IF the email is empty you can look for the user using getUser('kakao:${userId}')
he should already be a Kakao user for sure.
kakao/KakaoLoginServer/app.js
Outdated
console.log(`linking user with kakao provider with app user id ${userId}...`); | ||
return firebaseAdmin.auth() | ||
.setCustomUserClaims(userRecord.uid, | ||
{kakaoUID: userId}).then((Void) => Promise.resolve(userRecord)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last line, the same is:
{kakaoUID: userId}).then(() => userRecord);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it :)
kakao/KakaoLoginServer/app.js
Outdated
updateParams['email'] = email; | ||
console.log(`fetching a firebase user by email ${email}`); | ||
return firebaseAdmin.auth().getUserByEmail(email) | ||
.then((userRecord) => linkUserWithKakao(userId, userRecord)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you should check that the user is a Kakao user by check ing that he has an attribute lineUID
equals to the Kakao UID.
Then if the user is already a Kakao user, no need to link him again. Just return the user.
If the user is existing but he is NOT a Kakao user you need to "link" the user. Normally you should ask the user to sign-in using the other provider first (e.g. Google or Facebook) and only once he signed-in you can mark him as "linked".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about utilizing email_verified field that most oauth providers provide? This can simplify login process since re-authenticating with facebook or google is not necessary.
Here is the modified logic.
-
First check with uid kakao:${userId}
- This should be done first. Users signed up with kakao should be able to login again whether he or she has email field or not.
-
What if email is empty?
- Email could be empty due to lack of user consent. There is no way to map this user with any existing user if email is not given. Custom provider UIDs cannot be queried from custom user claims. There are two choices.
- Disallow creating a user
- Users will have to re-authenticate with providers with valid email and link again by passing firebase ID token (which will be used to retrieve firebase uid on the server-side) and Kakao access token.
- Just create a new user and deal with mapping later on user requests
- Disallow creating a user
- In this sample, just go with the second choice, cuz it's just sample. :)
- Email could be empty due to lack of user consent. There is no way to map this user with any existing user if email is not given. Custom provider UIDs cannot be queried from custom user claims. There are two choices.
- If email is present and there is a user with the email, give custom token to user if email is verified, make users re-authenticate with other providers if not verified.
If the email from Kakao provider is not verified yet, it is dangerous, in user security perspective, to give custom token since it might be different person with unverified email account. Therefore, only give custom token when email is verified. Throw an error otherwise, so that users can re-authenticate with other oauth providers first and link Kakao account later. This commit also contains other minor fixes mentioned in FirebaseExtended#17. Signed-off-by: Hara Kang <[email protected]>
Always thanks for your prompt review :) I added a new commit that contains several fixes you mentioned. |
Any update on this PR? :) |
Hey @CoderSpinoza sorry about that :) I'll get to it soon, I'm just done with a quite busy bunch of weeks. |
@nicolasgarnier Haha, guess you are busy these days! Just get to it whenever you are free :) |
"kakao": { | ||
"appId": -1 | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing line break at end of file :)
return firebaseAdmin.auth().getUserByEmail(email) | ||
.then((userRecord) => { | ||
if (!emailVerified) { | ||
throw new Error('This user should authenticate first ' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I would return a special error with a specific code so that we can act a special way when this happens.
Currently a new User gets created. Maybe do:
const error = new Error('This user should authenticate first ' +
'with other providers');
error.code = 'auth/existing-account-need-auth';
error.lineAccessToken = kakaoAccessToken; // Pass the access Token up to here, we'll need it for later.
throw error;
@@ -126,7 +200,7 @@ app.post('/verifyToken', (req, res) => { | |||
createFirebaseToken(token).then((firebaseToken) => { | |||
console.log(`Returning firebase token to user: ${firebaseToken}`); | |||
res.send({firebase_token: firebaseToken}); | |||
}); | |||
}).catch((error) => res.status(401).send({message: error})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe I would change message
to error
since it's really an error more than a message :)
Important point: in the Android Client application we need to display a special message when error.code === 'auth/existing-account-need-auth';
and in that case we need to link the account if the user signs-in with the existing account. To do that we need to add a new endpoints to just "link accounts" where you would pass the kakaoAccessToken
and the existing firebase user's ID Token.
Sorry for the delay ^^ I'll try to be faster next time :P |
Any updates? :) |
Any updates? This is very important. |
Waiting for updates too. |
Custom auth is not practical without the ability to merge accounts. +1 |
#10 demonstrates developers' concerns with correctly linking custom oauth providers with users already signed in with privileged providers. @nicolasgarnier wrote a wonderful comment on how this can be achieved using Auth#setCustomUserClaims(uid, customClaims).
Although there is a limitation in this approach, compared to FirebaseUser#linkWithCredential() method, I assume this could be a fair guide to third-party developers for structuring complete auth system with multiple oauth providers including custom ones.
Could you review this pull request? @nicolasgarnier @samtstern
p.s. Token verification functionality was added to Kakao API so I also included it. :)