Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OAuth Login #95

Open
wants to merge 34 commits into
base: develop
Choose a base branch
from
Open

OAuth Login #95

wants to merge 34 commits into from

Conversation

vibhorgupta-gh
Copy link

@vibhorgupta-gh vibhorgupta-gh commented Oct 1, 2018

fixes #73

@vibhorgupta-gh vibhorgupta-gh mentioned this pull request Oct 1, 2018
@charithccmc charithccmc requested review from iammosespaulr and a team October 2, 2018 01:33
ivantha
ivantha previously requested changes Oct 2, 2018
Copy link
Collaborator

@ivantha ivantha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! 👍
However would this PR be better if it also included the frontend part so that the functionality can be tested completely before merging? Any thoughts @malithsen ?

@ivantha ivantha dismissed their stale review October 2, 2018 07:45

This should not be a request to change.

@ivantha
Copy link
Collaborator

ivantha commented Oct 2, 2018

Looks good to me! 👍
However would this PR be better if it also included the frontend part so that the functionality can be tested completely before merging? Any thoughts @malithsen ?

Copy link

@mpiplani mpiplani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work.

@vibhorgupta-gh
Copy link
Author

@OshanIvantha @mpiplani thanks a lot!
I'd definitely add the frontend as well, the only thing being that facebook and google have considerably changed their APIs and OAuth request permissions, which is making it even more tedious to set it up on the frontend. My original plan was to do precisely that, but then because of this, I figured I'd completely test the backend and get it merged, and then start working on the frontend. Thoughts?

@malithsen
Copy link
Contributor

@VibhorCodecianGupta This looks good, and thank you for the contribution 👍
Let's merge when it is ready to be usable by the end user. Integration with a single OAuth provider like Google will be good enough to get this merged

@vibhorgupta-gh
Copy link
Author

I'll put the frontend code too then, thanks!

@vibhorgupta-gh
Copy link
Author

@malithsen @OshanIvantha I was recently getting on this, then I realised that new users can only be created by a logged in admin. With this logic in place, social login is not of much use, reason being that no new user can login with social accounts until and unless the admin has created one. As for existing users, they can just connect their social accounts and probably login only after the admin has created a user.
Every time one will attempt to login via social account, the code will try to create a new user (if not already existing) and that gets blocked until and unless admin authorises

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Login with OAuth
10 participants