Implement /auth with IVLE token #48

ning-y · 2018-06-13T18:35:23Z

Features

Remove email/password authentication
Implement authentication with IVLE token
1. Frontend sends an IVLE authentication token to /auth
2. Backend receives the IVLE authentication token as Cadet.Accounts.Form.Login
3. Backend uses IVLE authentication token to retrieve said token's corresponding NUSNET ID. This is also our outsourced 'authentication step'. IVLE api will return 200 if token is legitimate, otherwise 500.
4. Every NUSNET ID is associated with a user (Cadet.Accounts.User). Backend then responds with signed Tokens.
5. /auth will implicitly register the NUSNET ID into the database if not already registered.

Todos

I'm used to developing incrementally, especially for languages and frameworks that are quite new to me; so I'd prefer to open separate issues/PRs for these.

Check that the user reads CS1101S. The current implementation only checks that the user has an IVLE account.
Retrieve the user's role in CS1101S and assign the role field accordingly. The register method currently hardcodes each new user as a :student.
Implement API call to retrieve name. This is an API call we will be using from frontend to backend immediately after login (there is a profile button we need to return upon login success that is labelled with the user's name).

Dev notes

The ivle api key is saved as an environment variable and retrieved as a module attribute. Meaning, you must set the appropriate IVLE_KEY value on compile time. This is reflected in the READMEs.
You may come across an error such as (Postgrex.Error) ERROR 42703 (undefined_column): column "token" of relation "authorizations" does not exist on testing. This is because an early migration makes use of ecto_enum at Cadet.Accounts.Provider. Since this PR changes that Enum, and apparently modifying enums is tricky (?), you will need to manually reset your test databases (mix test only migrates, not resets by default). Run the following:

$ export MIX_ENV=test
$ mix ecto.drop
$ mix ecto.create
$ mix ecto.migrate
$ mix run priv/repo/seeds.exs  # if you are at git root
$ unset MIX_ENV

If you do not unset MIX_ENV, you're gonna have a bad time (for that shell session at least).

ning-y · 2018-06-13T18:37:17Z

I need some guidance on how to test functions that depend on external API calls (and functions that in turn depend on those). Referring to the module Cadet.Accounts.Ivle at `lib/cadet/accounts/ivle.ex'.

Update: Taking a look at ExVCR now.

coveralls · 2018-06-13T18:38:33Z

Pull Request Test Coverage Report for Build 432

33 of 34 (97.06%) changed or added relevant lines in 6 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.4%) to 94.416%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/cadet/auth/guardian.ex	3	4	75.0%

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

Totals
Change from base Build 411:	-0.4%
Covered Lines:	186
Relevant Lines:	197

💛 - Coveralls

remo5000 · 2018-06-14T05:11:27Z

lib/cadet/accounts/ivle.ex

+    case HTTPoison.get(api_url(path, token)) do
+      {:ok, response} ->
+        if String.length(response.body) > 2 do
+          {:ok, String.replace(response.body, ~s("), "")}


If there are other " characters in the response (apart from the start and end), I'm thinking it would be represented as \" as well. If this line replaces all the quotes, the \ might remain. Is this the case? Not sure about the ~s(") syntax here.

Nope, \ is not a literal character. When printed out to console, \" together represents a literal double quote within a string. ~s(") is a sigil denoting a string, it is equivalent to writing "\"".

iex(1)> ~s(") == "\"" true

String.replace looks a bit iffy -- how about Poison.decode!(response.body) == "" instead?

Refactor String.replace with Poison.decode! in 6120a44.

ning-y · 2018-06-15T06:57:15Z

lib/cadet/accounts/accounts.ex

    changeset = Registration.changeset(%Registration{}, attrs)

-    if changeset.valid?() do
+    if changeset.valid? do


@indocomsoft The new style guide in #50 says that zero-arity calls should end with parenthesis, but I think predicate functions should be excluded, because their trailing ? obviously indicates that they are predicate functions, not variables. Which do we go with? Your call.

Well, in the first place, changeset is a map (https://hexdocs.pm/ecto/Ecto.Changeset.html#t:t/0), so valid? is just a key in the map, so there should definitely be no paranthesis here.

indocomsoft

Wow! Thank you so much for taking the time to make this pull request -- overall the code is well-done, just a few changes like with construct, pattern-matching in tests, etc.

indocomsoft · 2018-06-16T08:47:32Z

README.md


+Setup environment variables
+
+    export IVLE_KEY="1vl3keyy"


This is not a real key right? Haha maybe you should put some directions like change this or sth. Also this should be in .env file instead, maybe create a .env.example

No worries, not a real key. Moved this to the .env file with 5ab080e and 3383e51.

indocomsoft · 2018-06-16T08:48:52Z

fixture/vcr_cassettes/accounts/sign_in#1.json

@@ -0,0 +1,29 @@
+[


Is it possible to move the fixture directory to text/fixture instead?

Done with 518307c.

indocomsoft · 2018-06-16T08:52:43Z

lib/cadet/accounts/accounts.ex

+  def sign_in(nusnet_id, token) do
+    auth = Repo.one(Query.nusnet_id(nusnet_id))

    if auth == nil do


You should reverse the conditional

if auth do # equivalent to if auth != nil else end

Done with 2b892a6.

indocomsoft · 2018-06-16T08:57:53Z

lib/cadet/accounts/accounts.ex

    if auth == nil do
-      nil
+      case Ivle.fetch_name(token) do
+        {:ok, name} ->


Use with construct instead to prevent nested case https://hexdocs.pm/elixir/Kernel.SpecialForms.html#with/1

Done with c8ae66d.

indocomsoft · 2018-06-16T09:01:00Z

lib/cadet/accounts/ivle.ex

+    case HTTPoison.get(api_url(path, token)) do
+      {:ok, response} ->
+        if String.length(response.body) > 2 do
+          {:ok, String.replace(response.body, ~s("), "")}


String.replace looks a bit iffy -- how about Poison.decode!(response.body) == "" instead?

indocomsoft · 2018-06-16T09:04:49Z

lib/cadet_web/controllers/auth_controller.ex

-            Guardian.encode_and_sign(user, %{}, token_type: "refresh", ttl: {52, :weeks})
-
-          render(conn, "token.json", access_token: access_token, refresh_token: refresh_token)
+      case Ivle.fetch_nusnet_id(login.ivle_token) do


with construct is better here to prevent pyramid of doom https://hexdocs.pm/elixir/Kernel.SpecialForms.html#with/1

Done with b2834fc.

indocomsoft · 2018-06-16T09:05:53Z

lib/cadet_web/controllers/auth_controller.ex

-    response(400, "Missing parameter(s)")
-    response(403, "Wrong login attributes")
+    response(400, "Missing or invalid parameter")
+    response(500, "Internal server error")


Eh, this happens?

Yep, IVLE responds with internal server error we send a wrong API key, and I think setting an invalid API key on our end is internal-server-error-esque.

Fair enough

indocomsoft · 2018-06-16T09:07:08Z

test/cadet/accounts/accounts_test.exs

+        role: :student
+      })
+
+    assert user.name == "happy user"


Use pattern matching instead, e.g. assert %{name: "happy user", role: :student} = user. Similarly for the rest

Done with 735169f.

indocomsoft · 2018-06-16T09:10:53Z

test/cadet/accounts/accounts_test.exs

+    assert {:ok, []} = Accounts.set_nusnet_id(user, "E012345")
+  end
+
+  # TODO: A user may not have multiple NUSNET_IDs?


I think it's a good idea to validate against this, but probably can be done in a separate PR. This PR is very big already haha

Yep. Didn't know that this TODO would get pass credo though. Guess it ignores the test files.

indocomsoft · 2018-06-16T09:16:13Z

test/cadet_web/controllers/auth_controller_test.exs

  alias CadetWeb.AuthController
  alias Cadet.Auth.Guardian

+  @token String.replace(inspect(System.get_env("TOKEN")), ~s("), "")


Not sure what this does, why not a simple System.get_env("TOKEN")? Also what is this TOKEN environment variable?

The token is the authentication token from ivle for the user. Added a brief description in the module docs of tests using @token, and improved the readability with 7075f93.

ning-y · 2018-06-16T19:51:59Z

@indocomsoft Made the changes, rebased to master, let me know what you think. Thanks!

indocomsoft

Looking very good, just some small changes

indocomsoft · 2018-06-17T11:51:28Z

.env

 export CADET_WEBPACK_PORT=4001

 # Webpack entry filename
 export CADET_WEBPACK_ENTRY=app


The format for .env file is just

KEY1=value1 KEY2=value2

The comments are fine tho

Done with 8d017f9.

indocomsoft · 2018-06-17T11:52:47Z

.env


 # Port which Webpack should be serving from in development
 export CADET_WEBPACK_PORT=4001



I think it's better to rename this to .env.example and add instruction in README to rename this to .env and modify the content accordingly.

Done with 8d017f9, and README updated accordingly.

indocomsoft · 2018-06-17T11:58:15Z

lib/cadet/accounts/accounts.ex

+          # Ivle.fetch_name/1 responds with :bad_request if token is invalid
+          {:error, :bad_request}
+
+        {:error, :internal_server_error} ->


This clause is superfluous since {:error, _} -> will match this too. In elixir, case-style matching is done sequentially.

Fixed with 2fbf69e. Meant to be explicit, but I'll use comments.

indocomsoft · 2018-06-17T12:05:40Z

lib/cadet/accounts/ivle.ex


  @api_url "https://ivle.nus.edu.sg/api/Lapi.svc"
-  @api_key System.get_env("IVLE_KEY")
+  @api_key Dotenv.load!().values["IVLE_KEY"]


If your intention is just to obtain value from .env file, you should use Dotenv.load. This is because Dotenv.load! will export the value into system environment (this effect might or might not be desirable) which adds unnecessary overhead

Fixed with 706363b.

indocomsoft · 2018-06-17T12:30:58Z

test/fixture/vcr_cassettes/accounts/sign_in#1.json

      "body": "\"LEE NING YUAN\"",
      "headers": {
        "Cache-Control": "private",
        "Content-Type": "application/json",


Maybe move test/fixture to test/fixtures? Haha a bit confusing to have both directories

My bad, typo in the config file. Done with 04d2a1a.

indocomsoft · 2018-06-17T12:34:30Z

lib/cadet/accounts/ivle.ex

+      {:ok, %{body: ~s(""), status_code: 200}} ->
+        # IVLE responsed 200 with body == ~s("") if token is invalid
+        {:error, :bad_request}
    end


I think you should handle the case {:error, _}?

Or else the server thread would crash for no case clause error (or maybe this is the desired behaviour?)

It's the desired behavior. The rationale is to prevent 'silent' failures. Frankly, I'm not sure what the best practice is with elixir phoenix, or api-building, so I'll go with your judgement on this one. Let me know

indocomsoft · 2018-06-17T13:11:49Z

lib/cadet/accounts/ivle.ex

@@ -0,0 +1,75 @@
+defmodule Cadet.Accounts.Ivle do


(Sorry I missed this during the first review) I think the module name should be Cadet.Accounts.IVLE https://github.com/lexmag/elixir-style-guide#camelcase-modules

Done with 429ca34. Do you prefer IVLETest or IVLE_Test?

indocomsoft · 2018-06-17T13:12:37Z

lib/cadet_web/controllers/auth_controller.ex

-    response(400, "Missing parameter(s)")
-    response(403, "Wrong login attributes")
+    response(400, "Missing or invalid parameter")
+    response(500, "Internal server error")


Fair enough

As per the standard phoenix 1.3.0 structure. http://phoenixframework.org/blog/phoenix-1-3-0-released

ning-y · 2018-06-18T02:53:37Z

@indocomsoft Second round of fixes done, do take a look, thanks!

Looking at test coverage

indocomsoft

Just need to add one test case

indocomsoft · 2018-06-18T03:03:33Z

lib/cadet_web/controllers/auth_controller.ex

-      end
-    else
-      send_resp(conn, :bad_request, "Missing parameters")
+      {:signin, {:error, reason}} ->


This clause is missing test coverage hmm

Done with eaa77fc.

indocomsoft

Still not covered :/

I've analysed the cause -- let me know what you think

indocomsoft · 2018-06-18T08:51:33Z

lib/cadet/accounts/ivle.ex

+
+      {:ok, %{body: ~s(""), status_code: 200}} ->
+        # IVLE responsed 200 with body == ~s("") if token is invalid
+        {:error, :bad_request}


I think you should handle the case {:error, _}

Gotcha, done with 0a9c589.

indocomsoft · 2018-06-18T08:52:52Z

test/cadet_web/controllers/auth_controller_test.exs

+      use_cassette "auth_controller/v1/auth#1", custom: true do
+        conn =
+          post(conn, "/v1/auth", %{
+            "login" => %{"ivle_token" => 'token'}


Note that in elixir 'charlist' and "binary" are 2 different ways of storing strings https://elixir-lang.org/getting-started/binaries-strings-and-char-lists.html

it should be "token" instead.

More to the point, it still doesn't cover: reading the report generated from running mix coveralls.html test/cadet_web/controllers/auth_controller_test.exs:77, the error is "caught" at {:fetch, {:ok, nusnet_id}} <- {:fetch, IVLE.fetch_nusnet_id(login.ivle_token)} since api_fetch/2 returns {:error, :bad_request} on %{body: ~s(""), status_code: 200}.

One way to solve this might be to handle {:error, _} in api_fetch/2, then purposely fail the request IVLE.fetch_name/1 if that makes sense.

My bad, I should've checked the coveralls link. Fixed with 59a315f using custom cassettes. The fix didn't need the catch "don't care" clause, but I'll leave that change in.

indocomsoft · 2018-06-18T10:14:47Z

lib/cadet/accounts/ivle.ex

+      {:ok, _} ->
+        # IVLE responds 200 with body == ~s("") if token is invalid
+        {:error, :bad_request}
+    end


Haha what I meant was adding another clause here {:error, _} -> {:error, :internal_server_error} or something like that, unless the intention is to let the server thread just crash on connection failure (which I think would also return HTTP 500)

If they both return 500, I would rather let it crash. It'd be easier to catch unexpected bahaviour that way. Again, no experience with building APIs and web stuff here, so I'll default to your judgement. I've reverted it, but lmk what you'd prefer and I'll make the change.

This reverts commit 0a9c589.

indocomsoft

LGTM.

Since we're talking about Erlang/Elixir here, I guess just let it crash

indocomsoft · 2018-06-18T10:49:22Z

Merged this for you -- maybe you could open issues in this PR's To-Do's

* Move context files folders with the same name As per the standard phoenix 1.3.0 structure. http://phoenixframework.org/blog/phoenix-1-3-0-released * Change user fields to include NUSNET ID * Change /auth to take ivle_token (was email, pw) * Add HTTPoison dependency for requests to IVLE * Change email in authorizations to nusnet_id * Remove NUSNET ID from users * Remove password from registration, authorization * Remove comeonin tokens * Add IVLE API call to /auth * Register users automatically if first time login * Improve documentation, fix some credo --strict's * Squash migrations into a single file * Add tests for Ivle module via ExVCR * Add test for fetch_name in Cadet.Accounts.Ivle * Fine tune /auth error codes * Add tests for sign_in in Cadet.Accounts * Add tests for AuthController using ExVCR * Fix failing travis.ci tests * Remove password fields from registration test * Set IVLE_KEY in .env rather than environment var * Move ExVCR fixture to test/fixture * Fix Ivle module using env var instead of dot env * Reverse conditional for readability * Improve readability for Ivle's api_fetch/2 * Improve readability for AuthController's create/2 * Improve readability for Account's sign_in/2 * Improve tests with assert pattern match * Improve readability and docs for @token in tests * Fix credo offences due to ff89294 * Fix failing credo tests because of bad cassettes * Move .env file as .env.example, update README * Fix redundant pattern matching * Rename module Ivle -> IVLE * Move test/fixture -> test/fixtures * Change Dotenv.load! -> Dotenv.load * Add test for bad auth_controller create/2 sign_in * Catch all :errors for api_fetch * Fix missing coverage for signin error * Revert "Catch all :errors for api_fetch" This reverts commit 0a9c589.

ning-y requested review from indocomsoft, remo5000 and sreycodes June 13, 2018 18:35

remo5000 reviewed Jun 14, 2018

View reviewed changes

ning-y commented Jun 15, 2018

View reviewed changes

ning-y force-pushed the account-ivle branch from 67c4a76 to bc2c1d9 Compare June 16, 2018 04:12

indocomsoft requested changes Jun 16, 2018

View reviewed changes

ning-y force-pushed the account-ivle branch 2 times, most recently from 8b91219 to 0573059 Compare June 16, 2018 19:45

indocomsoft requested changes Jun 17, 2018

View reviewed changes

ning-y added 17 commits June 18, 2018 10:45

Move context files folders with the same name

89cccb5

As per the standard phoenix 1.3.0 structure. http://phoenixframework.org/blog/phoenix-1-3-0-released

Change user fields to include NUSNET ID

4a9d700

Change /auth to take ivle_token (was email, pw)

fac9126

Add HTTPoison dependency for requests to IVLE

2a2d8ba

Change email in authorizations to nusnet_id

28fe00d

Remove NUSNET ID from users

62c5817

Remove password from registration, authorization

4190763

Remove comeonin tokens

3e8a27d

Add IVLE API call to /auth

7a7e055

Register users automatically if first time login

b9a7a86

Improve documentation, fix some credo --strict's

7a6a046

Squash migrations into a single file

263296b

Add tests for Ivle module via ExVCR

a9925f8

Add test for fetch_name in Cadet.Accounts.Ivle

68c56c8

Fine tune /auth error codes

edad9d3

Add tests for sign_in in Cadet.Accounts

0eef9d0

Add tests for AuthController using ExVCR

20c465e

ning-y added 8 commits June 18, 2018 10:48

Improve readability and docs for @token in tests

73de001

Fix credo offences due to ff89294

634ccd7

Fix failing credo tests because of bad cassettes

f11f519

Move .env file as .env.example, update README

6298799

Fix redundant pattern matching

3508e83

Rename module Ivle -> IVLE

2c2cede

Move test/fixture -> test/fixtures

f1e09aa

Change Dotenv.load! -> Dotenv.load

2a5329a

ning-y force-pushed the account-ivle branch from 706363b to 2a5329a Compare June 18, 2018 02:48

indocomsoft previously approved these changes Jun 18, 2018

View reviewed changes

indocomsoft requested changes Jun 18, 2018

View reviewed changes

Add test for bad auth_controller create/2 sign_in

eaa77fc

remo5000 mentioned this pull request Jun 18, 2018

Add API specs for Controllers #41

Merged

ning-y mentioned this pull request Jun 18, 2018

Store IVLE API key in dotenv file source-academy/frontend#111

Merged

indocomsoft requested changes Jun 18, 2018

View reviewed changes

ning-y added 2 commits June 18, 2018 17:29

Catch all :errors for api_fetch

0a9c589

Fix missing coverage for signin error

59a315f

indocomsoft reviewed Jun 18, 2018

View reviewed changes

Revert "Catch all :errors for api_fetch"

ae97a8a

This reverts commit 0a9c589.

ning-y force-pushed the account-ivle branch from 765c14a to ae97a8a Compare June 18, 2018 10:41

indocomsoft approved these changes Jun 18, 2018

View reviewed changes

indocomsoft merged commit 526917d into master Jun 18, 2018

indocomsoft mentioned this pull request Jun 20, 2018

Specification for a 4th user role #54

Closed

ning-y deleted the account-ivle branch June 22, 2018 05:13

indocomsoft modified the milestone: Sprint 5 Jun 24, 2018


		# Port which Webpack should be serving from in development
		export CADET_WEBPACK_PORT=4001

Implement /auth with IVLE token #48

Implement /auth with IVLE token #48

Uh oh!

Conversation

ning-y commented Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Features

Todos

Dev notes

Uh oh!

ning-y commented Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 432

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

indocomsoft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ning-y Jun 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ning-y Jun 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ning-y Jun 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ning-y commented Jun 16, 2018

ning-y commented Jun 13, 2018 •

edited

Loading

ning-y commented Jun 13, 2018 •

edited

Loading

coveralls commented Jun 13, 2018 •

edited

Loading

ning-y Jun 16, 2018 •

edited

Loading

ning-y Jun 16, 2018 •

edited

Loading

ning-y Jun 16, 2018 •

edited

Loading

indocomsoft Jun 17, 2018 •

edited

Loading