feat: Social Logins #719

yugantarjain · 2020-08-10T14:43:08Z

Description

Currently a work in progress, this PR implements social Login options (Google and Apple). This has been done by creating callback APIs for both Google auth and Apple auth.

Flow:

The client uses the social sign in service on the app and receive id_token, name, and email.
The POST APIs created in this PR take that information (as payload) from the app.
The email of the user is used to find an existing user in the database.
If a user is not found for the email, a new user is created using the data. If a user with the unique id_token already exists on the system, an error message is returned. Else, tokens are generated and returned, succesfully signing in the user.
If a user is found, the social sign in details are verified for the user id and the exact provider (apple/google). If social sign in record is not found, an error is returned. Else, tokens are generated an returned, succesfully signing in the user.

Fixes #730

Type of Change:

Code
Quality Assurance
Outreach
Documentation

Code/Quality Assurance Only

New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

Tested on local host. Google sign in done using iOS app to get id token. Then id token used in backend api and tested.

Checklist:

My PR follows the style guidelines of this project
I have performed a self-review of my own code or materials
I have commented my code or provided relevant documentation, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
Any dependent changes have been merged
Update Swagger documentation and the exported file at /docs folder
Update requirements.txt

Code/Quality Assurance Only

My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been published in downstream modules

codecov · 2020-08-11T09:41:24Z

Codecov Report

Merging #719 into develop will increase coverage by 0.08%.
The diff coverage is 98.23%.

@@             Coverage Diff             @@
##           develop     #719      +/-   ##
===========================================
+ Coverage    95.84%   95.93%   +0.08%     
===========================================
  Files           95       98       +3     
  Lines         5200     5361     +161     
===========================================
+ Hits          4984     5143     +159     
- Misses         216      218       +2

Impacted Files	Coverage Δ
config.py	`89.23% <ø> (ø)`
tests/users/test_dao_social_sign_in.py	`94.73% <94.73%> (ø)`
app/api/resources/user.py	`90.40% <97.91%> (+1.45%)`	⬆️
tests/users/test_api_social_sign_in.py	`97.91% <97.91%> (ø)`
app/api/dao/user.py	`86.61% <100.00%> (+1.53%)`	⬆️
app/api/models/user.py	`100.00% <100.00%> (ø)`
app/database/models/social_sign_in.py	`100.00% <100.00%> (ø)`
app/database/models/user.py	`98.61% <100.00%> (+0.01%)`	⬆️
app/messages.py	`100.00% <100.00%> (ø)`
... and 3 more

… for email id)

isabelcosta

@yugantarjain good work with this PR :) This is the first time that I see code related with social login so this is a bit new to me. Here are my suggestions:

I suggested some improvements to the code to follow some conventions of this app.
Unit tests, where you may have to use mock functionalities, I am not sure
Can you explain in the PR description, and perhaps also in a markdown file under /docs folder, how the social login works, how to use this here (for me and others to learn)

isabelcosta · 2020-08-12T22:43:19Z

app/api/resources/user.py

+        # create tokens and expiry timestamps
+        access_token = create_access_token(identity=user.id)
+        refresh_token = create_refresh_token(identity=user.id)
+
+        from run import application
+        access_expiry = datetime.utcnow() + application.config.get(
+            "JWT_ACCESS_TOKEN_EXPIRES"
+        )
+        refresh_expiry = datetime.utcnow() + application.config.get(
+            "JWT_REFRESH_TOKEN_EXPIRES"
+        )
+
+        # return data
+        return (
+            {
+                "access_token": access_token,
+                "access_expiry": access_expiry.timestamp(),
+                "refresh_token": refresh_token,
+                "refresh_expiry": refresh_expiry.timestamp(),
+            },
+            HTTPStatus.OK,
+        )


This can go to a private method in this file or somewhere else, as long as you reuse this logic since you use it in at least 2 different places.

isabelcosta · 2020-08-12T22:44:53Z

app/api/resources/user.py

+
+        token = request.json.get("id_token")
+        email = request.json.get("email")
+        client_id = "992237180107-lsibe891591qcubpbd8qom4fts74i5in.apps.googleusercontent.com"


is this a secret variable? can you please put this in a constant, either here or probably better in Config.py

Okay. I checked the config.py file and am not sure how exactly to add this constant there... Should I just do it the way it is done in messages.py?

@yugantarjain Read the 4th Point here in Setup and Run part of Docs https://github.com/anitab-org/mentorship-backend. Config.py reads the os.env variables for config. As part of setup of this app, someone sets the env variables. So you just need to add the variable and read from there.
I am not sure how you can get the exact value of the config into production instance. @isabelcosta you can help clarify this?

Done. Made a constant in config.py.

isabelcosta · 2020-08-12T22:48:36Z

app/api/resources/user.py

+            user = DAO.get_user_for_social_login(email)
+
+            if not user:
+                # create a new user
+                data = request.json
+                user = DAO.create_user_using_social_login(data)
+


will the logic here work fine if this is not a gmail email? I never did anything like this so I am really curious

Yes, it will work fine. In-fact, when I tested with Sign in with Apple, I hid my email (the service then returns a private relay email) and it worked perfectly.

isabelcosta · 2020-08-12T22:51:54Z

app/database/models/user.py

@@ -29,6 +29,7 @@ class UserModel(db.Model):

    # security
    password_hash = db.Column(db.String(100))
+    apple_auth_id = db.Column(db.String(100))


for this to work I think you need to create a migration script to add this field to dev database. Can you do this?

Yes, I did create a migration script and ran it locally. Do I need to perform any additional steps?

isabelcosta · 2020-08-12T22:58:40Z

app/api/models/user.py

+        "username": fields.String(required=False, description="User username"),
+        "password": fields.String(required=False, description="User password"),


Does this need to change? as far as I see you did not change anything to the POST /register API, you created new endpoints that follow a different request model.

Yes. Actually, when a user signs up using social login, we first try to find them in our database. If a pre-existing user is not found, we create a new one. To facilitate the creation of a new user, these fields were made optional. I did some testing and it works fine (because we have validation checks for them in any case) and all the unit tests passed.

If there is a better solution to this, please let me know. We also have an ongoing discussion about this on zulip mentorship ios channel.

Sounds good @yugantarjain thank you for your explanation!

vatsalkul · 2020-08-17T13:16:36Z

This PR is aimed to solve #730 Issue

ApheleiaS · 2020-08-17T19:49:17Z

app/api/resources/user.py

+        user = DAO.create_user_using_social_login(data, provider)
+        # If any error occured, return error
+        if not isinstance(user, UserModel):
+            return user


You returning a user instance here? Is this expected? in case of error, shouldn't you format it or does the user variable have the error response in which case you should also return error type (and also can you make that clear using comments?)

If user is not an instance of UserModel, then that means it contains the error message and http code.
Comment added to clarify the code.

Should we unwrap the error and http code from user variable here to make the code more readable without comment?

ApheleiaS · 2020-08-17T19:52:55Z

app/api/resources/user.py

+
+        token = request.json.get("id_token")
+        email = request.json.get("email")
+        client_id = "992237180107-lsibe891591qcubpbd8qom4fts74i5in.apps.googleusercontent.com"


@yugantarjain Read the 4th Point here in Setup and Run part of Docs https://github.com/anitab-org/mentorship-backend. Config.py reads the os.env variables for config. As part of setup of this app, someone sets the env variables. So you just need to add the variable and read from there.
I am not sure how you can get the exact value of the config into production instance. @isabelcosta you can help clarify this?

…e constant in config.py

yugantarjain · 2020-08-19T07:06:48Z

@vatsalkul just realized, the id token for apple is always the same, but google gives a new one. However, the logic remains unaffected.

SanketDG · 2020-08-19T15:15:47Z

config.py

@@ -1,6 +1,7 @@
 import os
 from datetime import timedelta

+GOOGLE_AUTH_CLIENT_ID = "992237180107-lsibe891591qcubpbd8qom4fts74i5in.apps.googleusercontent.com"


You are not supposed to commit this, instead keep it configurable using an environment variable.

Please also make sure you revoke the oAuth application associated with this, if you don't want misuse.

Fixed. Thanks!

You are not supposed to commit this, instead keep it configurable using an environment variable.

I actually thought about that, but then ... not sure why I forgot to review it here. Thank you so much @SanketDG for your review!

SanketDG · 2020-08-19T15:20:25Z

app/database/models/user.py


        # default values
        self.is_admin = True if self.is_empty() else False  # first user is admin
-        self.is_email_verified = False
+        if social_login:
+            self.is_email_verified = True


Why not verify here? Any special usecase?

Yes. For normal login, the email is not verified and the user specifically has to do that from a link they receive.

Got you, I was thinking something different, not applicable here though

I have updated this to - self.is_email_verified = social_login
That removes unnecessary if statements.

yugantarjain · 2020-08-21T06:27:02Z

@isabelcosta I guess all the requested changes have been addressed.

docs/user_authentication.md

tests/users/test_dao_social_sign_in.py

isabelcosta · 2020-08-23T17:29:03Z

app/database/models/social_sign_in.py

+    associated_email = db.Column(db.String(50))
+    full_name = db.Column(db.String(50))
+
+    def __init__(self, user_id, sign_in_type, id_token, email, name):


Since you are using type hints in the other functions of this file, why not use here :)

I'm not clear about this point.

app/database/models/social_sign_in.py

isabelcosta · 2020-08-23T17:30:56Z

app/api/resources/user.py

+        "JWT_REFRESH_TOKEN_EXPIRES"
+    )
+
+    # return data


Suggested change

# return data

Is this line for debugging purposes?

No, this is just a comment, shall I remove it?

Co-authored-by: Isabel Costa <[email protected]>

paritoshsinghrahar · 2020-08-26T12:17:12Z

@isabelcosta @SanketDG
As the team leads for Mentorship-Backend I have a question. Since the present PR only deals with finding social login options using Google and Apple.

Should I raise new Issue under Outreach/Research for OSH:
Design a solution for integrating third-party apps authentication.

Such that someone might propose a probable uniform solution integrating more social logins like Slack, Facebook, Twitter, etc.
The proposal is only for suggestions/probable framework. Not involving coding.

If, yes. Should I break it into two or more parts? And how many hours and people should be assigned?

SanketDG

Looks good to merge!

SanketDG · 2020-08-26T18:36:24Z

Sounds good to me @paritoshsinghrahar

Such that someone might propose a probable uniform solution integrating more social logins like Slack, Facebook, Twitter, etc.
The proposal is only for suggestions/probable framework. Not involving coding.

Great!

isabelcosta · 2020-09-03T21:03:34Z

Sounds good to me @paritoshsinghrahar

Such that someone might propose a probable uniform solution integrating more social logins like Slack, Facebook, Twitter, etc.
The proposal is only for suggestions/probable framework. Not involving coding.

Great!

@paritoshsinghrahar I +1 this :)

techno-disaster · 2020-09-15T09:47:08Z

@anitab-org/mentorship-backend-maintainers any update on this?

vj-codes · 2021-03-17T19:47:42Z

@yugantarjain please resolve the merge conflicts

isabelcosta · 2021-03-20T18:49:22Z

@vj-codes i think we can close this for now, since its inactive for a whillllle. Also the main issue should have its scope reduced, to split issues. So social login for one service (e.g.: google) and another for another service (e.g.: github). Can you help refine the issue please?

google login done

a774cc8

yugantarjain requested a review from isabelcosta August 10, 2020 14:43

yugantarjain changed the title ~~wip: Social Login~~ wip: Social Logins Aug 10, 2020

requirements updated

fb6408f

yugantarjain added 2 commits August 11, 2020 16:44

sign up using google added (new user is created if old user not found…

9b8cedf

… for email id)

apple sign in done

4cea797

yugantarjain requested review from annabauza, ApheleiaS, sunjunkie and vatsalkul August 12, 2020 21:44

isabelcosta requested changes Aug 12, 2020

View reviewed changes

isabelcosta requested a review from SanketDG August 12, 2020 23:00

creating and returning access tokens code refactored

6340c57

yugantarjain requested a review from isabelcosta August 13, 2020 05:43

yugantarjain added 4 commits August 14, 2020 02:16

social sign in table created and used

dd9216a

social sign in implementation finalized

ba8528d

code documentation and logic improved

c6b1aea

messages cosntants fixed. API code refactored and re-used.

470d572

yugantarjain mentioned this pull request Aug 14, 2020

feat: Social logins anitab-org/mentorship-ios#112

Merged

9 tasks

tests added and logic improved

f78e925

yugantarjain changed the title ~~wip: Social Logins~~ feat: Social Logins Aug 14, 2020

social sign in documentation added in user_authentication.md

29708e3

ApheleiaS reviewed Aug 17, 2020

View reviewed changes

yugantarjain added 2 commits August 18, 2020 14:21

code documentation improved

a68ee39

error unwrapping done to make code clearer. google auth client id mad…

bb8c558

…e constant in config.py

yugantarjain requested a review from ApheleiaS August 19, 2020 07:01

SanketDG suggested changes Aug 19, 2020

View reviewed changes