Implement /auth in REST API #33

indocomsoft · 2018-05-19T05:38:22Z

Based on #32, once that PR is merged, the no of files changed in this PR will be reduced significantly

coveralls · 2018-05-19T05:39:56Z

Pull Request Test Coverage Report for Build 236

37 of 40 (92.5%) changed or added relevant lines in 4 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+7.9%) to 94.186%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/cadet/auth/error_handler.ex	0	1	0.0%
lib/cadet/auth/guardian.ex	3	5	60.0%

Files with Coverage Reduction	New Missed Lines	%
lib/cadet_web/router.ex	1	75.0%

Totals
Change from base Build 216:	7.9%
Covered Lines:	162
Relevant Lines:	172

💛 - Coveralls

indocomsoft · 2018-05-20T05:35:13Z

@evansb I have just cleaned this PR up after #32 was merged. Currently the missing coverage is from the incomplete Assessment context (#26) and removal of authenticated pages (so the part of code that ensures authentication is not covered yet.)

evansb · 2018-05-26T04:49:15Z

lib/cadet_web/controllers/auth_controller.ex

+      end
+    else
+      conn
+      |> send_resp(404, "Missing parameters")


Why 404 should be Bad Request

evansb · 2018-05-26T04:50:25Z

lib/cadet_web/controllers/auth_controller.ex

+
+  def create(conn, _params) do
+    conn
+    |> send_resp(404, "Missing parameters")


I think for one function call we shouldn't use pipe syntax, what do you think?

Sure. Maybe we can base our code styling on this guideline: https://github.com/christopheradams/elixir_style_guide? @sreycodes

evansb · 2018-05-26T04:51:50Z

lib/cadet_web/controllers/auth_controller.ex

+    |> send_resp(404, "Missing parameter(s)")
+  end
+
+  def logout(conn, %{"refresh_token" => refresh_token}) do


Hmm why does logout need to pass the refresh token? After authenticating user you should simply delete the user entry in GuardianDB

Right, about that: we still haven't integrated guardiandb. Will integrate soon.

evansb · 2018-05-26T04:54:48Z

lib/cadet_web/controllers/auth_controller.ex

+
+  swagger_path :create do
+    post("/auth")
+    summary("Obtain access and refresh tokens to authenticate user")


Since this is API documentation, this should be more descriptive so the frontend team knows what to do just by reading API docs. For instance, I think you should specify which HTTP header to put the token in, the token expiry time, etc

evansb · 2018-05-26T04:56:54Z

lib/cadet_web/router.ex

    }
  end

-  scope "/api/swagger" do
+  scope "/v1/swagger" do


Typically we don't scope the swagger doc with API version because it's supposed to cover all versions.

indocomsoft · 2018-05-26T07:06:54Z

@evansb Re: logout needing to pass refresh_token
Because the hook is located on Guardian.revoke(token), so I need to know the token to be invalidated (and since it's JWT this info is not stored anywhere by me). Access token is valid for 1 hour only, so it makes sense to simply invalidate the refresh token. However, if you like, I can let the user pass the access token and invalidate that as well.

Alternatively, I can add a field to user to store his/her refresh_token but I feel this solution is not as elegant since the frontend already stores the refresh_token anyway.

indocomsoft · 2018-05-29T12:13:34Z

@evansb Fixed this according to your code review, should be ready for merging.

* Remove frontend stuffs * Remove frontend config * Change router * Remove failing tests * Remove frontend stuffs * Remove frontend config * Change router * Remove failing tests * Remove unnecessary plugs in router * Revert wrongfully removed conflict resolution * Remove failing test * Run mix format * Implement auth get token * Complete implementation of AuthController * Remove more front-end junk * Mix format * Wrote some preliminary tests * Write tests for AuthController#create * Add tests for AuthController#refresh * Complete testing and add slight change to handle error more gracefully * Remove yet another frontend junk * Mix format * Add comment: to integrate with GuardianDb in the future * Correct one missed conflict * Add instruction in README on API documentation * Change 404 error code to 400 * Change swagger route * Modify auth controller test to conform to 400 bad request * Remove single function call pipe syntax * Change status code to its atom equivalent * Add guardian_db and setup * Add securityDefinitions in swagger * Add more descriptive swagger summary * Fix logout to accept access_token instead * Mix format * Clean-up swagger documentation

indocomsoft added 4 commits May 19, 2018 10:48

Remove frontend stuffs

088033b

Remove frontend config

b051da4

Change router

20d26aa

Remove failing tests

d88bf50

indocomsoft added this to the Sprint 1 milestone May 19, 2018

indocomsoft added 11 commits May 19, 2018 16:28

Remove frontend stuffs

112057c

Remove frontend config

0264864

Change router

fb4c69a

Remove failing tests

21efd07

Remove unnecessary plugs in router

7bf9a25

Revert wrongfully removed conflict resolution

bd2eba5

Remove failing test

5d5ebf4

Run mix format

bf0c9ce

Merge branch 'master' into auth

fcbe737

Merge branch 'remove-frontend' into auth

7ab3937

Implement auth get token

2724f6f

indocomsoft force-pushed the auth branch from 8b9b072 to 2724f6f Compare May 19, 2018 10:51

indocomsoft added 9 commits May 19, 2018 22:14

Complete implementation of AuthController

3e180d7

Remove more front-end junk

91d4427

Mix format

2f37d5e

Wrote some preliminary tests

ee685f3

Write tests for AuthController#create

62a4cdb

Add tests for AuthController#refresh

5f945b7

Complete testing and add slight change to handle error more gracefully

2fb0a5a

Remove yet another frontend junk

b393b57

Mix format

afb72a6

indocomsoft changed the title ~~[WIP] APIfy auth~~ Implement /auth in REST API May 19, 2018

indocomsoft requested a review from evansb May 19, 2018 15:04

Add comment: to integrate with GuardianDb in the future

89bae92

indocomsoft mentioned this pull request May 20, 2018

Remove frontend #32

Merged

indocomsoft added 2 commits May 20, 2018 13:28

Merge branch 'master' into auth

5beea40

Correct one missed conflict

820c857

Add instruction in README on API documentation

848453c

evansb requested changes May 26, 2018

View reviewed changes

indocomsoft added 8 commits May 26, 2018 13:11

Change 404 error code to 400

b562209

Change swagger route

0884ddd

Modify auth controller test to conform to 400 bad request

e28db7d

Remove single function call pipe syntax

268575c

Change status code to its atom equivalent

03960be

Add guardian_db and setup

ec592c2

Add securityDefinitions in swagger

e6d1909

Add more descriptive swagger summary

a0a5290

Merge branch 'master' into auth

2747fb0

indocomsoft mentioned this pull request May 26, 2018

Sprint Goals #37

Closed

28 tasks

indocomsoft added 3 commits May 29, 2018 20:03

Fix logout to accept access_token instead

7256245

Mix format

fc9083c

Clean-up swagger documentation

651754c

evansb approved these changes Jun 8, 2018

View reviewed changes

evansb merged commit f6aab65 into master Jun 8, 2018

indocomsoft deleted the auth branch June 8, 2018 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement /auth in REST API #33

Implement /auth in REST API #33

indocomsoft commented May 19, 2018 •

edited

Loading

coveralls commented May 19, 2018 •

edited

Loading

indocomsoft commented May 20, 2018 •

edited

Loading

evansb May 26, 2018

evansb May 26, 2018

indocomsoft May 26, 2018 •

edited

Loading

evansb May 26, 2018

indocomsoft May 26, 2018

evansb May 26, 2018

evansb May 26, 2018

indocomsoft commented May 26, 2018 •

edited

Loading

indocomsoft commented May 29, 2018

Implement /auth in REST API #33

Implement /auth in REST API #33

Conversation

indocomsoft commented May 19, 2018 • edited Loading

coveralls commented May 19, 2018 • edited Loading

Pull Request Test Coverage Report for Build 236

💛 - Coveralls

indocomsoft commented May 20, 2018 • edited Loading

evansb May 26, 2018

Choose a reason for hiding this comment

evansb May 26, 2018

Choose a reason for hiding this comment

indocomsoft May 26, 2018 • edited Loading

Choose a reason for hiding this comment

evansb May 26, 2018

Choose a reason for hiding this comment

indocomsoft May 26, 2018

Choose a reason for hiding this comment

evansb May 26, 2018

Choose a reason for hiding this comment

evansb May 26, 2018

Choose a reason for hiding this comment

indocomsoft commented May 26, 2018 • edited Loading

indocomsoft commented May 29, 2018

indocomsoft commented May 19, 2018 •

edited

Loading

coveralls commented May 19, 2018 •

edited

Loading

indocomsoft commented May 20, 2018 •

edited

Loading

indocomsoft May 26, 2018 •

edited

Loading

indocomsoft commented May 26, 2018 •

edited

Loading