Add JWT auth #3

tevariou · 2020-11-27T19:03:36Z

Fixes

Fixes #2 by @tevariou

Description

Add a login page
Replace basic authentication with JWT auth
Fix yarn timeout
Shared volume between api and nginx is now limited to django static files and the socket file
Change env variables management
The uwsgi server is run by a dedicated non root user

Technical details

The Patient model still has the code attribute that has a unique constraint. An UUID is autogenerated if the field is not filled by the client. It can be used as a study code to avoid subject duplicates.

simonvadee · 2020-11-30T15:06:15Z

api/.env

+DJANGO_SUPERUSER_PASSWORD=${DJANGO_SUPERUSER_PASSWORD:-password}
+DJANGO_SUPERUSER_USERNAME=${DJANGO_SUPERUSER_USERNAME:-admin}
+DJANGO_SUPERUSER_EMAIL=${DJANGO_SUPERUSER_EMAIL:[email protected]}
+DJANGO_SECRET_KEY=${DJANGO_SECRET_KEY:-secret}
+DJANGO_DEBUG=${DJANGO_DEBUG:-1}
+DJANGO_ALLOWED_HOSTS="${DJANGO_ALLOWED_HOSTS:-localhost 127.0.0.1 [::1]}"


this should be in the docker-compose file, and the .env should not be comitted

simonvadee · 2020-12-04T10:26:53Z

postgres/.env

+POSTGRES_DB=${POSTGRES_DB:-avc}
+POSTGRES_USER=${POSTGRES_USER:-postgres}
+POSTGRES_PASSWORD=${POSTGRES_PASSWORD:-password}
+POSTGRES_HOST=${POSTGRES_HOST:-db}
+POSTGRES_PORT=${POSTGRES_PORT:-5432}


this should be in the docker-compose file, and the .env should not be comitted

un .env (pas commit parce qu'il peut contenir des secrets) qui vient override les defaults du docker-compose

The variables in the docker-compose file take precedence on the .env files https://docs.docker.com/compose/environment-variables/#the-env-file docker-compose > shell > .env > dockerfile. What is usually done is to have multiple docker-compose files: https://docs.docker.com/compose/extends/ && https://docs.docker.com/compose/production/

One base docker-compose.yml, one docker-compose.override.yml for local dev with one or multiple .env.local files added in .gitignore and one docker-compose.prod.yml. The .env and .env.local are merged and the variables in the local file will take precedence on the base in case of duplicate.

Here two issues I think this solves:

Make front development easy. No need to build the front app which takes time. Use of .env files is also idiomatic in React. To override the variables to connect with a remote API, simply add variables in a .env.local file (no edit of the base configuration needed and there are gitignored).

Running the app the easiest way possible with minimal edit.

So i've made some changes, tell me what you think!

works for me 👌

I advise you not to do this way.

There is now 6 files to manage configuration.

Having a single .env in the key=value format (no fancy yaml stuff) allows you to use python-dotenv in load config in the same manner than docker. This makes it very easy to switch from the local development environment to a containerized application. Furthermore, true dotenv files (key=value format) are supported by IDEs (idea, vscode), which allows you to reuse them for debugging.

In your proposed setup, casually changing configuration while developping would have to be done by modifying the settings.py configuration file, which would show up in you diff.

Putting separate production compose files is interesting for documentation, but it has little value for developpment.

simonvadee · 2020-12-04T10:27:13Z

nginx/.env

@@ -0,0 +1,2 @@
+NGINX_PORT=${NGINX_PORT:-8080}


simonvadee · 2020-12-04T10:28:16Z

app/Dockerfile

-#WORKDIR /app
-#ENV PATH /app/node_modules/.bin:$PATH
-#COPY ./package.json .
-#RUN yarn
-#COPY . .
-#RUN yarn build
-#
-#FROM busybox
-#WORKDIR /app
-#COPY --from=builder /app/build .
-
-FROM busybox
+FROM node:15.3.0
 WORKDIR /app
-COPY ./build .
+ENV PATH /app/node_modules/.bin:$PATH
+COPY ./package.json .
+COPY ./.yarnrc .
+COPY ./yarn.lock .
+RUN yarn
+COPY . .
+RUN yarn build


this produces a very large image for just static files in the end, why not using 2-stage build ?

simonvadee · 2020-12-04T10:28:23Z

app/.env

@@ -0,0 +1 @@
+REACT_APP_API_URL=${API_URL:-http://localhost:8080/api}


simonvadee · 2020-12-07T15:44:15Z

app/package.json

@@ -44,7 +44,7 @@
    "@types/uuid": "^8.3.0"
  },
  "scripts": {
-    "start": "react-scripts start",
+    "start": "env $(cat .env) react-scripts start",


isn't the .env automatically understood by react ?

simonvadee · 2020-12-07T15:45:12Z

app/src/navigation/Private.tsx

+// const ME = gql`
+//   query me {
+//     me {
+//       id
+//       name
+//       email
+//     }
+//   }
+// `;


don't you want to remove these comments ?

I copied the component from cohort and it used graphQL to handle user logged status. I juste kept some code commented in case I need it later because for now authentication persistance is not wired yet. (if you refresh the page, authentication credentials are lost)

simonvadee · 2020-12-07T15:45:44Z

app/src/screens/Login.tsx

+      name: "password",
+      password: true,
+      label: "Password",
+      // validationRules: { required: "Field required" },


why is it commented out ?

simonvadee · 2020-12-07T15:46:07Z

app/src/screens/Login.tsx

+      <FormBuilder<LoginData>
+        properties={properties}
+        formId="login-form"
+        defaultValues={{ username: "toto", password: "" }}


do we want to keep these ?

No we don't

simonvadee · 2020-12-07T15:46:24Z

app/src/state/user.ts

+  // userName: string
+  // displayName: string
+  // firstName: string
+  // lastName: string
+  // deidentified: boolean


simonvadee · 2020-12-07T15:47:15Z

app/src/utils/api.ts

@@ -0,0 +1,20 @@
+import { AUTH_API_URL } from "../constants";
+
+const API_URL = `${process.env.REACT_APP_AUTH_API_URL}`;


why not using AUTH_API_URL ?

Removed in next commit ;)

simonvadee · 2020-12-07T15:47:48Z

app/src/utils/api.ts

+  //   method: "POST",
+  //   body: JSON.stringify({ username, password }),
+  // });
+  const loginData = await fetch(`http://localhost:8080/api/token/`, {


AUTH_API_URL ?

simonvadee · 2020-12-07T15:49:40Z

docker-compose.prod.yml

+    volumes:
+      - app_data:/app


I don't think this volume is necessary

This volume is actually shared with nginx. That's where the react static files are stored.

simonvadee · 2020-12-07T15:57:34Z

docker-compose.override.yml

+#    env_file:
+#      - ./postgres/.env.local


does it work if I only uncomment these lines but not the ones below ?

As long as the file exist, yes! I need to comment those lines since docker will return an error if the files doesn't exist and will not run (instead of ignoring them which would have been nicer).

simonvadee · 2020-12-07T16:01:20Z

postgres/.env

+POSTGRES_DB=${POSTGRES_DB:-avc}
+POSTGRES_USER=${POSTGRES_USER:-postgres}
+POSTGRES_PASSWORD=${POSTGRES_PASSWORD:-password}
+POSTGRES_HOST=${POSTGRES_HOST:-db}
+POSTGRES_PORT=${POSTGRES_PORT:-5432}


works for me 👌

…tr/feat/token_auth

simonvadee · 2020-12-07T18:16:12Z

app/src/navigation/Private.tsx

@@ -6,7 +6,7 @@ import { useDispatch } from "react-redux";
 // import { gql } from 'apollo-boost'

 import { ACCES_TOKEN } from "../constants";
-import { login } from "state/user";
+// import { login } from "state/user";


vmttn · 2020-12-08T09:46:30Z

api/Dockerfile

+RUN groupadd -r api && useradd --create-home --no-log-init -r -g api api
+USER api:api
+WORKDIR /home/api
+ENV PYTHONPATH=/home/api


This is already the case if the cwd is /home/api

It seems that it's not. uwsgi crash returning a ModuleNotFoundError: No module named 'avc_forms' if I don't specify the python path. PYTHONPATH is undefined in the container if I don't define it manually here.

I see you problem. You can set module=avc_forms.wsgi in the uwsgi.ini, which a pretty standard way.

Note: regarding the uwsgi.ini file, there's a lot of stuff in there. Did you find that everything was required?

The module and the mount attributes seem to be mutually exclusive. The others are either required (chdir, mount, manage-script-name, socket, chmod-socket, uid, guid) or standard server configuration. I did a bit of cleaning.

vmttn · 2020-12-08T09:46:50Z

api/Dockerfile

+USER api:api
+WORKDIR /home/api
+ENV PYTHONPATH=/home/api
+ENV PATH /home/api/.local/bin:${PATH}


Why is this required ?

Warning supression such as Consider adding this directory to PATH or, if you prefer to suppress this warning, use --no-warn-script-location. during python package install and it doesn't find uwsgi when it's time to run the server.

vmttn · 2020-12-08T09:48:34Z

api/Dockerfile

+WORKDIR /home/api
+ENV PYTHONPATH=/home/api
+ENV PATH /home/api/.local/bin:${PATH}
+ENV DJANGO_SETTINGS_MODULE=avc_forms.settings


Putting this here is not required (use a default in the uwsgi.py).

django-admin returns No Django settings specified if I don't specify this (in docker-entrypoint.sh I'm using django-admin to create a superuser). It seems the default is correctly set in uwsgi.py though.

You can use python manage.py createsuperuser to create a superuser.

As a rule of thumb, use the manage.py over django-admin when the codebase is available.

api/avc_forms/settings.py

api/docker-entrypoint.sh

vmttn · 2020-12-08T09:58:22Z

api/requirements.txt

 django-filter==2.4.0
 djangorestframework==3.12.2
+djangorestframework-simplejwt==4.6.0
 Markdown==3.3.3
 psycopg2==2.8.6


psycopg2 is always builded, hence it requires OS packages (at least for bionic). This could be documented in a CONTRIBUTING.md.

vmttn · 2020-12-08T10:00:10Z

app/Dockerfile

+COPY ./yarn.lock .
+RUN yarn
+COPY . .
+RUN yarn build

 FROM busybox


Out of curiosity, why did you use busybox ? (btw you should set a version)

I wanted to use scratch since I only want to put the build directory in the app-data volume but It crash if I don't specify a CMD to run something... so I use busybox here since it's the smallest image I could find and it shutdowns gracefully.

Ok I see.

In terms of engineering, I think it would be best to try to reuse the same image at the company (namely python:3.x-slim), to reduce the cognitive load (same files, same tricks).

But it is your call.

vmttn · 2020-12-08T10:58:09Z

nginx/templates/nginx.conf.template

@@ -1,5 +1,5 @@
 upstream django {
-    server unix:/api/avc_forms/avc_forms.sock;
+    server unix:/api/avc_forms.sock;


…tr/feat/token_auth

simonvadee · 2020-12-09T15:47:28Z

api/avc_forms/settings.py

@@ -11,8 +11,11 @@
 """

 from pathlib import Path
+# from dotenv import load_dotenv, find_dotenv


catch them all !

unexpected pokemon ?

- Login/Logout - Patient creation - Patient imports from CSV are sent to the API - Patient deletion is wired with the API - Note: Patient edit is not wired yet

- Handles entries edit (patient editing) - Added notifications system - Some improvements in token managment

tevariou added 4 commits November 27, 2020 19:13

Add jwt auth

7b1cf20

Refactor env vars mgmt

55fcb76

fix: pg env for api

6c8a9d2

Add doc

8f26924

tevariou changed the title ~~Tr/feat/token auth~~ Add JWT auth Nov 27, 2020

tevariou added 3 commits November 28, 2020 02:45

Run api as non root

140181a

fix: yarn

e43cbe9

restart api on failure

bfe27e3

tevariou requested review from simonvadee, BPierrick and vmttn November 30, 2020 10:40

simonvadee reviewed Dec 4, 2020

View reviewed changes

BPierrick approved these changes Dec 4, 2020

View reviewed changes

tevariou added 2 commits December 4, 2020 17:09

fix: app and api dockerfiles

142a92a

feat: split docker-compose

b5f61ab

tevariou requested a review from simonvadee December 5, 2020 12:09

tevariou and others added 3 commits December 5, 2020 20:03

fix: add comment in override if .env.local does not exist

a1d3d69

App changes to login with API

20c52aa

fix: cors issues

7c72b73

simonvadee reviewed Dec 7, 2020

View reviewed changes

tevariou assigned BPierrick and tevariou Dec 7, 2020

BPierrick and others added 4 commits December 7, 2020 18:27

[App] Auth / Rapid fix

79a31a0

Login form validation rules fix

03fc181

feat: bump django version

ceed5c0

Merge branch 'tr/feat/token_auth' of github.com:arkhn/AVC-Forms into …

fb8808f

…tr/feat/token_auth

simonvadee approved these changes Dec 7, 2020

View reviewed changes

fix: ignore override

b0d830c

vmttn suggested changes Dec 8, 2020

View reviewed changes

fix: env in dockerfile

20dbc4f

tevariou and others added 4 commits December 9, 2020 12:49

fix: busybox version

e7c8337

Added disable rules over some inputs

5ace132

fix: code field in model, set uuid by default

3841953

Merge branch 'tr/feat/token_auth' of github.com:arkhn/AVC-Forms into …

cb4b147

…tr/feat/token_auth

simonvadee reviewed Dec 9, 2020

View reviewed changes

BPierrick and others added 4 commits December 9, 2020 18:14

[APP] Wired authentication & Patients basic requests with API

3b648fc

- Login/Logout - Patient creation - Patient imports from CSV are sent to the API - Patient deletion is wired with the API - Note: Patient edit is not wired yet

refactor: deployment

dc44112

merge

d2b68aa

fix: cleaning

76cba8f

tevariou requested a review from vmttn December 9, 2020 22:30

tevariou and others added 2 commits December 11, 2020 12:31

fix: useless attributes

f4c4cb8

[APP] Wire front to API

ba0d080

- Handles entries edit (patient editing) - Added notifications system - Some improvements in token managment

BPierrick force-pushed the tr/feat/token_auth branch from 33444ff to ba0d080 Compare December 14, 2020 16:50

tevariou added 2 commits December 14, 2020 21:38

fix: env mgmt

d5e8a6c

fix: bump version

7fcc096

tevariou merged commit 9f69b10 into master Dec 15, 2020

tevariou deleted the tr/feat/token_auth branch December 15, 2020 10:17

		@@ -0,0 +1 @@
		REACT_APP_API_URL=${API_URL:-http://localhost:8080/api}

		@@ -0,0 +1,20 @@
		import { AUTH_API_URL } from "../constants";

		const API_URL = `${process.env.REACT_APP_AUTH_API_URL}`;

Add JWT auth #3

Add JWT auth #3

Conversation

tevariou commented Nov 27, 2020 • edited Loading

Fixes

Description

Technical details

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tevariou Dec 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tevariou Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tevariou commented Nov 27, 2020 •

edited

Loading

tevariou Dec 5, 2020 •

edited

Loading

tevariou Dec 9, 2020 •

edited

Loading