Implemented refresh tokens #165

iulianav · 2018-12-29T20:52:16Z

Description

Access tokens expire and require the user to login again in order to receive a new one. With the use of refresh tokens, the user can request from the auth server new access tokens without having to logout and login again. Moreover, the life of a refresh token is much longer, but for this reason they need to be stored very securely.

Fixes #160

Type of Change:

Code

Code/Quality Assurance Only

This change requires a documentation update (software upgrade on readme file)
New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

I have not tested it properly yet.

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 Postman API at /docs folder

Code/Quality Assurance Only

My changes generate no new warnings
My PR currently breaks something (fix or feature that would cause existing functionality to not work as expected)
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

isabelcosta · 2018-12-29T21:26:19Z

@iulianav thank you so much for sending this PR 🎉

I might have to study a little bit to understand how it works and review it properly, but from what I've seen in your PR your work seems to make a lot of sense 😄

Regarding testing do you need help testing the API itself (1)? Or just writing tests for it (2)?

(1) Do you know how to access Swagger UI? here's 2 wiki pages I wrote that kind of show how to use Swagger for login and register new user. When you run the app, you can access the Swagger UI through localhost:5000 (if I'm not mistaken). Do you need help with this?
(2) I have no clear idea of how to do this. But you could look into this file tests/users/test_api_login.py and get some ideas?

Let me know if you have any question.

iulianav · 2018-12-29T22:01:24Z

@isabelcosta Thank you! I have already written some tests (2), but not sure about the location! At first, I wanted to add them to test_api_login.py, but then I added them in the end to test_api_authentication.py because the class was called TestProtectedAPI? Maybe I should create a new test file, but I think it fits with the authentication.

I also played with (1) and it seemed fine. I will test it more tho!

isabelcosta · 2018-12-29T22:32:44Z

@iulianav Regarding written tests: I think it is better to create a separate file dedicated to your new API /refresh tests

There are some tests that may not be well separated, because when I did them I was still learning how to implement tests and make the project "modular".

isabelcosta · 2018-12-29T22:35:27Z

@iulianav I will edit te PR description to have Fixes _#ISSUE_ instead of Addresses _#ISSUE_, because the first option closes the related issue when the PR is merged. Thats how the template is done ;)

iulianav · 2018-12-29T23:55:41Z

@isabelcosta great! I amended the commit following some API testing (1) and your suggestions. I believe everything will work as expected hopefully! :)

isabelcosta · 2019-02-10T11:59:51Z

@ramitsawhney27 , @m-murad can you take a look at this? I understand a bit of this feature, but I need more eyes on this just to be sure it does what is supposed to.

isabelcosta · 2019-02-10T12:02:04Z

app/api/models/user.py

@@ -154,7 +155,14 @@ def add_models_to_namespace(api_namespace):

 login_response_body_model = Model('Login response data model', {
    'access_token': fields.String(required=True, description='User\'s access token'),
-    'expiry': fields.Float(required=True, description='Access token expiry UNIX timestamp')


I think this could remain still on this PR, until the android app is refactored to not use expiry, I'm not sure now, I think expiry is not used for anything on systers/mentorship-android if this is true than this can be removed :)

@isabelcosta i will leave it for now and add a TODO comment

isabelcosta

Great work @iulianav with this PR!
I just need you to fix merge conflicts and then squash commits

You definitely followed the community guidelines here 🎉
I just need at least one more review from maintainers here.

iulianav · 2019-02-10T12:31:41Z

@isabelcosta thank you alot!

Addresses anitab-org#160 Access tokens expire and require the user to login again in order to receive a new one. With the use of refresh tokens, the user can request from the auth server new access tokens without having to logout and login again. Moreover, the life of a refresh token is much longer, but for this reason they need to be stored very securely. Signed-off-by: Iuliana Voinea <iulianavoinea96@gmail.com>

iulianav · 2019-02-10T12:49:17Z

app/api/models/user.py

 login_response_body_model = Model('Login response data model', {
    'access_token': fields.String(required=True, description='User\'s access token'),
-    'expiry': fields.Float(required=True, description='Access token expiry UNIX timestamp')
+    'expiry': fields.Float(required=True, description='Access token expiry UNIX timestamp'),


@isabelcosta i kept it.

Cool ;) it can be removed later @iulianav

isabelcosta added the Review Requested label Dec 29, 2018

iulianav force-pushed the implement-refresh-tokens-feature branch from 4ffc68e to 88d1445 Compare December 29, 2018 21:46

iulianav force-pushed the implement-refresh-tokens-feature branch from 88d1445 to d1e2e36 Compare December 29, 2018 23:53

iulianav force-pushed the implement-refresh-tokens-feature branch from d1e2e36 to db5b9f3 Compare December 30, 2018 00:03

isabelcosta changed the title ~~feat: Implement refresh tokens~~ Implemented refresh tokens Dec 30, 2018

isabelcosta reviewed Feb 10, 2019

View reviewed changes

isabelcosta previously approved these changes Feb 10, 2019

View reviewed changes

iulianav dismissed isabelcosta’s stale review via 8e58691 February 10, 2019 12:48

iulianav force-pushed the implement-refresh-tokens-feature branch from db5b9f3 to 8e58691 Compare February 10, 2019 12:48

iulianav commented Feb 10, 2019

View reviewed changes

isabelcosta approved these changes Feb 10, 2019

View reviewed changes

isabelcosta merged commit 226b573 into anitab-org:develop Feb 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented refresh tokens #165

Implemented refresh tokens #165

iulianav commented Dec 29, 2018 •

edited by isabelcosta

Loading

isabelcosta commented Dec 29, 2018

iulianav commented Dec 29, 2018 •

edited

Loading

isabelcosta commented Dec 29, 2018

isabelcosta commented Dec 29, 2018

iulianav commented Dec 29, 2018 •

edited

Loading

isabelcosta commented Feb 10, 2019

isabelcosta Feb 10, 2019

iulianav Feb 10, 2019

isabelcosta left a comment

iulianav commented Feb 10, 2019

iulianav Feb 10, 2019

isabelcosta Feb 10, 2019

Implemented refresh tokens #165

Implemented refresh tokens #165

Conversation

iulianav commented Dec 29, 2018 • edited by isabelcosta Loading

Description

Type of Change:

How Has This Been Tested?

Checklist:

isabelcosta commented Dec 29, 2018

iulianav commented Dec 29, 2018 • edited Loading

isabelcosta commented Dec 29, 2018

isabelcosta commented Dec 29, 2018

iulianav commented Dec 29, 2018 • edited Loading

isabelcosta commented Feb 10, 2019

isabelcosta Feb 10, 2019

Choose a reason for hiding this comment

iulianav Feb 10, 2019

Choose a reason for hiding this comment

isabelcosta left a comment

Choose a reason for hiding this comment

iulianav commented Feb 10, 2019

iulianav Feb 10, 2019

Choose a reason for hiding this comment

isabelcosta Feb 10, 2019

Choose a reason for hiding this comment

iulianav commented Dec 29, 2018 •

edited by isabelcosta

Loading

iulianav commented Dec 29, 2018 •

edited

Loading

iulianav commented Dec 29, 2018 •

edited

Loading