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

Add token collation using server-obtained token #186

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Add token collation using server-obtained token #186

wants to merge 13 commits into from

Conversation

hkmatsumoto
Copy link
Contributor

@hkmatsumoto hkmatsumoto commented Dec 7, 2019 •
edited
Loading

This pull request adds an auth token validation that collates with an input token with a token obtained from the server.

This pull request is made to finish the Google Code-in task (https://codein.withgoogle.com/dashboard/task-instances/5898939732066304/)

@hkmatsumoto
Copy link
Contributor Author

I changed the way to validate the given token.
Now set_token() sends a post request to /api/auth/user with the token in header, and if server replies back without errors set_token() assumes the given token is a valid token.

@vkartik97
Could I have your review? Thanks.

Ram81
Ram81 requested changes Dec 8, 2019
View reviewed changes
Copy link
Member

@Ram81 Ram81 left a comment

Choose a reason for hiding this comment

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

@takitsuse can you add the test for the new method that you added. Code coverage is failing because there is no test for the new method. Please add those then we will review the complete PR.

@hkmatsumoto
Copy link
Contributor Author

Code coverage recovered.

@yashdusing
Copy link

LGTM 👍 !

Ram81
Ram81 requested changes Dec 8, 2019
View reviewed changes
tests/test_auth.py Outdated Show resolved Hide resolved
tests/test_auth.py Outdated Show resolved Hide resolved
tests/test_auth.py Outdated Show resolved Hide resolved
krtkvrm
krtkvrm reviewed Dec 8, 2019
View reviewed changes
evalai/utils/auth.py Outdated Show resolved Hide resolved
@hkmatsumoto
Copy link
Contributor Author

Thanks for your reviews! I am not in home now, so I will make those changes in 1-2 hours.

@hkmatsumoto
Copy link
Contributor Author

hkmatsumoto commented Dec 8, 2019 •
edited
Loading

@vkartik97 @yashdusing @Ram81
Do my changes look good?

Ram81
Ram81 requested changes Dec 8, 2019
View reviewed changes
evalai/utils/auth.py Outdated Show resolved Hide resolved
tests/test_auth.py Outdated Show resolved Hide resolved
@hkmatsumoto
Copy link
Contributor Author

I am going to bed in a minute, so I create a new commit assuming I should put generate_random_string in evalai/utils/common.py.

@hkmatsumoto hkmatsumoto requested a review from Ram81 December 8, 2019 16:02
krtkvrm
krtkvrm reviewed Dec 9, 2019
View reviewed changes
evalai/utils/auth.py Outdated Show resolved Hide resolved
@hkmatsumoto hkmatsumoto requested a review from krtkvrm December 9, 2019 06:46
@pushkalkatara
Copy link

Looks good to me. @vkartik97

krtkvrm
krtkvrm approved these changes Dec 9, 2019
View reviewed changes
Copy link
Member

@krtkvrm krtkvrm left a comment

Choose a reason for hiding this comment

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

@takitsuse
Thanks for prompt changes on reviews!
LGTM :)

RishabhJain2018
RishabhJain2018 requested changes Dec 15, 2019
View reviewed changes
evalai/utils/auth.py Outdated Show resolved Hide resolved
evalai/add_token.py Outdated Show resolved Hide resolved
evalai/utils/urls.py Outdated Show resolved Hide resolved
RishabhJain2018
RishabhJain2018 requested changes Dec 15, 2019
View reviewed changes
evalai/add_token.py Show resolved Hide resolved
evalai/utils/auth.py Show resolved Hide resolved
@hkmatsumoto
Copy link
Contributor Author

I made changes accordingly to your reviews. @RishabhJain2018 Please take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krtkvrm krtkvrm krtkvrm approved these changes

@Ram81 Ram81 Awaiting requested review from Ram81

@RishabhJain2018 RishabhJain2018 Awaiting requested review from RishabhJain2018

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@hkmatsumoto @yashdusing @pushkalkatara @krtkvrm @RishabhJain2018 @Ram81