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

#164069215 JWT Auth on successful login and registration. #10

Merged
merged 1 commit into from
Mar 5, 2019
Merged

#164069215 JWT Auth on successful login and registration. #10

merged 1 commit into from
Mar 5, 2019

Conversation

BagzieGracious
Copy link
Contributor

@BagzieGracious BagzieGracious commented Feb 28, 2019 •
edited
Loading

What does this PR do?

  • This PR enables users receive a JWT upon successful Registration and Login, and then implements the newly strategy for authentication.

Description of Task to be completed?

  • Decoded user object into a token.
  • Return token to successfully logged in user.
  • Implemented JWTAuthentications in backends.
  • Added token field in models.

How should this be manually tested?

  • Clone the repo and cd into Ah-backend-aquaman
  • git checkout ft-jwt-authentication-164069215
  • pip install -r requirements.txt
  • python manage.py makemigrations
  • python manage.py migrate
  • python manage.py runserver

The below endpoints will be used to log in with

  • Registration endpoint
    POST http://127.0.0.1:8000/api/users/
  • Login endpoint
    POST http://127.0.0.1:8000/api/users/login/

Any background context you want to provide?

  • N/A

What are the relevant pivotal tracker stories?

#164069215

screenshots

  • Registration screenshot

screenshot 2019-03-04 at 17 25 45

  • Login screenshot

screenshot 2019-02-28 at 20 40 35

joelethan
joelethan reviewed Feb 28, 2019
View reviewed changes
Copy link
Contributor

@joelethan joelethan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@BagzieGracious I think you should add an image to show how the user inputs their credentials when logging in and registering

@Peace-Apple
Copy link
Contributor

Good work, could you also add a screenshot showing some of these things in action, like when someone logs in or signs up

Peace-Apple
Peace-Apple previously approved these changes Feb 28, 2019
View reviewed changes
Copy link
Contributor

@Peace-Apple Peace-Apple left a comment

Choose a reason for hiding this comment

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

LGTM

@CryceTruly
Copy link
Contributor

LGTM

joelethan
joelethan previously approved these changes Feb 28, 2019
View reviewed changes
Copy link
Contributor

@joelethan joelethan left a comment

Choose a reason for hiding this comment

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

LGTM

wasibani-roy
wasibani-roy previously approved these changes Mar 1, 2019
View reviewed changes
Copy link
Contributor

@wasibani-roy wasibani-roy left a comment

Choose a reason for hiding this comment

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

LGTM cloned and run it successfully on my computer

@BagzieGracious BagzieGracious force-pushed the ft-jwt-authentication-164069215 branch 9 times, most recently from 22f1301 to b274358 Compare March 4, 2019 12:46
CryceTruly
CryceTruly previously approved these changes Mar 4, 2019
View reviewed changes
Copy link
Contributor

@CryceTruly CryceTruly left a comment

Choose a reason for hiding this comment

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

LGTM

Peace-Apple
Peace-Apple previously approved these changes Mar 4, 2019
View reviewed changes
Copy link
Contributor

@Peace-Apple Peace-Apple left a comment

Choose a reason for hiding this comment

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

LGTM

joelethan
joelethan previously approved these changes Mar 4, 2019
View reviewed changes
marthamareal
marthamareal reviewed Mar 4, 2019
View reviewed changes
Copy link
Contributor

@marthamareal marthamareal left a comment

Choose a reason for hiding this comment

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

Good work @BagzieGracious , You can also add tests for the different edge cases for example what happens when an invalid token is provided

@BagzieGracious BagzieGracious force-pushed the ft-jwt-authentication-164069215 branch from b274358 to e791cf8 Compare March 5, 2019 08:16
CryceTruly
CryceTruly suggested changes Mar 5, 2019
View reviewed changes
Copy link
Contributor

@CryceTruly CryceTruly left a comment

Choose a reason for hiding this comment

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

Thanks @BagzieGracious for the good work, you just have to make sure you test enough as your push decreased test coverage by 4%.

@BagzieGracious BagzieGracious force-pushed the ft-jwt-authentication-164069215 branch from e791cf8 to b4dfdb1 Compare March 5, 2019 09:58
joelethan
joelethan previously approved these changes Mar 5, 2019
View reviewed changes
Copy link
Contributor

@joelethan joelethan left a comment

Choose a reason for hiding this comment

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

@BagzieGracious take a look at the ReadMe.md it has some repetitive information

CryceTruly
CryceTruly previously approved these changes Mar 5, 2019
View reviewed changes
Copy link
Contributor

@CryceTruly CryceTruly left a comment

Choose a reason for hiding this comment

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

LGTM

@malep2007 @BagzieGracious
Chore(Jwt Authentication): Users should receive a JWT on successful R…
fda1d70 
…egistration and Login

 - Users receive a JWT upon successful Registration and Login, and then implements the newly
 strategy for authentication.

 [Finishes #164069215]
@BagzieGracious BagzieGracious dismissed stale reviews from CryceTruly and joelethan via fda1d70 March 5, 2019 11:57
@BagzieGracious BagzieGracious force-pushed the ft-jwt-authentication-164069215 branch from b4dfdb1 to fda1d70 Compare March 5, 2019 11:57
wasibani-roy
wasibani-roy previously requested changes Mar 5, 2019
View reviewed changes
Copy link
Contributor

@wasibani-roy wasibani-roy left a comment

Choose a reason for hiding this comment

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

Good work @BagzieGracious you where able to add tests however you could consider adding a minimum length to your password field under your login serializer just to ensure a reasonable password is entered

@wasibani-roy wasibani-roy dismissed their stale review March 5, 2019 12:16

Okay it would bring about maintainability issues

wasibani-roy
wasibani-roy approved these changes Mar 5, 2019
View reviewed changes
Copy link
Contributor

@wasibani-roy wasibani-roy left a comment

Choose a reason for hiding this comment

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

LGTM good job @BagzieGracious

CryceTruly
CryceTruly approved these changes Mar 5, 2019
View reviewed changes
Copy link
Contributor

@CryceTruly CryceTruly left a comment

Choose a reason for hiding this comment

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

LGTM

@marthamareal marthamareal merged commit 1dc63dd into develop Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marthamareal marthamareal marthamareal left review comments

@joelethan joelethan joelethan approved these changes

@CryceTruly CryceTruly CryceTruly approved these changes

@wasibani-roy wasibani-roy wasibani-roy approved these changes

@Peace-Apple Peace-Apple Peace-Apple left review comments

@oryono oryono Awaiting requested review from oryono

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

Successfully merging this pull request may close these issues.

None yet
7 participants
@BagzieGracious @Peace-Apple @CryceTruly @joelethan @marthamareal @wasibani-roy @malep2007