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.

Sign up for GitHub

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

Jump to bottom

Google Verification #7631

Merged
merged 18 commits into from
Nov 11, 2020
Merged

Google Verification #7631

merged 18 commits into from
Nov 11, 2020

Conversation

iRhonin
Copy link
Contributor

@iRhonin iRhonin commented Oct 6, 2020 •
edited

Description

Added google verification.

Refers/Fixes

gitcoinco/skunkworks#187

Testing

This feature uses Google as a third-party, so can not test easily, but if you insists, I will find a way.

Senario

screen-capture (1)

@iRhonin iRhonin changed the title Googleverification Google Verification Oct 6, 2020
@iRhonin iRhonin marked this pull request as ready for review October 7, 2020 10:24
@iRhonin
Copy link
Contributor Author

iRhonin commented Oct 7, 2020

you should research mate the best node. and the best issue regarding to this project. why dont you try to read first mate. and you can create a good result regarding to this plan. and project. i alrready answer this yesterday. its about the verification and access in working in github.

Hi @rodgz1622,
I did not find your comment, the link you provided is a link to this PR.

@iRhonin iRhonin mentioned this pull request Oct 7, 2020
Open
@thelostone-mc
Copy link
Member

cc @apbendi

@thelostone-mc thelostone-mc added this to Engineering-Review in PR Review Board Oct 9, 2020
@octavioamu octavioamu requested a review from owocki October 9, 2020 23:06
thelostone-mc
thelostone-mc requested changes Oct 12, 2020
View reviewed changes
Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

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

@iRhonin Could you share a video with the whole flow where a user is not Google verified and what the steps are to get themselves verified ?

thelostone-mc
thelostone-mc requested changes Oct 12, 2020
View reviewed changes
Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

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

@iRhonin Could you share a video with the whole flow where a user is not Google verified and what the steps are to get themselves verified?

app/app/urls.py
r'^api/v0.1/profile/verify_user_google',
dashboard.views.verify_user_google,
name='verify_user_google'
),
Copy link
Member

Choose a reason for hiding this comment

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

^ not very keen on having 2 new endpoints
Ideally I'd say we have one common endpoint

handle/verify/ as a POST operation and based on a body param -> we could decide which flow to send it into

cc @apbendi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems good, I will going for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized google callback URL can not be dynamic, so we can not include the handle parameter in the callback URL.
Another issue, google uses the GET method to call callback URL.
But existing twitter callback uses POST and getting a handle parameter in the URL. to merge them I need to change that.
@thelostone-mc what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine to have separate endpoints

@thelostone-mc thelostone-mc moved this from Engineering-Review to Waiting on contributor in PR Review Board Oct 12, 2020
@iRhonin
Copy link
Contributor Author

iRhonin commented Oct 12, 2020

@iRhonin Could you share a video with the whole flow where a user is not Google verified and what the steps are to get themselves verified ?

Added to the first comment.

apbendi
apbendi reviewed Oct 14, 2020
View reviewed changes
app/dashboard/views.py
from oauthlib.oauth2.rfc6749.errors import InvalidGrantError

try:
google = connect_google()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to ask Google if the account is at least 6 months old? We do this for Twitter, and I think we should here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@apbendi Unfortunately, I did not find any programmatic way to retrieve the account creation date.

@apbendi
Copy link
Contributor

apbendi commented Oct 14, 2020

Awesome work @iRhonin! I left some feedback, but overall it looks great. Thanks for your contribution :)

@thelostone-mc thelostone-mc moved this from Waiting on contributor to Engineering-Review in PR Review Board Oct 20, 2020
@thelostone-mc thelostone-mc mentioned this pull request Nov 5, 2020
Closed
@octavioamu octavioamu merged commit 862527a into gitcoinco:master Nov 11, 2020
PR Review Board automation moved this from Engineering-Review to Done Nov 11, 2020
@iRhonin iRhonin deleted the googleverification branch November 11, 2020 17:05
@apbendi
Copy link
Contributor

apbendi commented Dec 1, 2020

hey @iRhonin, I'm regression testing Google Verification as part of the preparation for Grants Round 8. Can you give me some instruction on how to properly set up a Google OAuth app as a part of my dev environment. I know I need these two env vars:

GOOGLE_CLIENT_ID=
GOOGLE_CLIENT_SECRET=

...but I don't know what to do on the Google side to get these running from my own account for dev purposes. Could you provide some guidance?

@apbendi apbendi mentioned this pull request Dec 1, 2020
Closed
21 tasks
@iRhonin
Copy link
Contributor Author

iRhonin commented Dec 1, 2020

Hello @apbendi, please check this documention

@owocki
Copy link
Contributor

owocki commented Mar 16, 2021

@iRhonin
the google account id of the user stored anywhere on the DB? during our testing we realized that multiple users could connect to the same google account id , which is def a problem for anti-sybil design.

would you be able to PR a fix for that? happy to pay in ETH :)

@iRhonin
Copy link
Contributor Author

iRhonin commented Mar 16, 2021

@iRhonin
the google account id of the user stored anywhere on the DB? during our testing we realized that multiple users could connect to the same google account id , which is def a problem for anti-sybil design.

would you be able to PR a fix for that? happy to pay in ETH :)

Sure, that is wired, I will fix that.

@iRhonin iRhonin mentioned this pull request Mar 17, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apbendi apbendi apbendi left review comments

@danlipert danlipert danlipert approved these changes

@octavioamu octavioamu octavioamu approved these changes

@thelostone-mc thelostone-mc thelostone-mc approved these changes

@chibie chibie chibie approved these changes

@frankchen07 frankchen07 Awaiting requested review from frankchen07

@PixelantDesign PixelantDesign Awaiting requested review from PixelantDesign

@owocki owocki Awaiting requested review from owocki

@amustapha amustapha Awaiting requested review from amustapha

Assignees
No one assigned
Labels
None yet
Projects
PR Review Board
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@iRhonin @thelostone-mc @apbendi @owocki @danlipert @octavioamu @chibie