Remix auth #47

sverhoeven · 2023-07-19T12:08:03Z

Fixes i-VRESSE/bartender#54

Should be merged together with i-VRESSE/bartender#73

TODO

To test:

Start bartender web service from https://github.com/i-VRESSE/bartender/tree/asym-jwt-only branch with public key.
Setup webapp according to development instructions in README.md
Register + login + give yourself an expertise level
Check on build page that topoaa node has options for your selected expertise level
Configure social login GitHub, Orcid or EGI and try to login with those accounts. PS. Make sure you a logged out from the webapp before trying to log in again.
Start webapp with docker compose using Docker instructions in README.m

x

…able by user + lots of other stuff to make things work and look cleaner

Versions and changed code from `npx create-remix@latest` run in temp dir.

…ker fixed

To ignore postgres-data during format had to add ignore in command. See prettier/prettier#11568 (comment)

…ring

Peter9192

Profile popup doesn't go away when clicked
Checkboxes on admin page shouldn't apply directly, but after a save
Can't change password on first admin user?
Batch checker doesn't work on admin page (with single user)

Peter9192 · 2023-08-14T14:56:20Z

docs/auth.md

+A super user can be made through the admin page (`/admin/users`) or by being the first registered user.
+


I accidentily revoked SU rights for the first registered user... that should not be so easy.

Added confirm dialog in 3821052

Disabling super user toggle when you are the only super user would be nice, but confirm is good enough.

Peter9192 · 2023-08-14T15:05:58Z

package.json

-    "typecheck": "tsc"
+    "typecheck": "tsc",
+    "setup": "prisma generate && prisma migrate deploy && prisma db seed",
+    "docker:dev": "docker compose -f docker-compose.dev.yml up",


It's nice to have this as an npm run alias, but if you stop it (ctrl+c) then the containers will linger on my system (docker ps -a ). I'd like to have either the counterpart with docker compose rm or the docker-compose alternative for docker --rm (is that --abort-on-container-exit?)

Additionally, a command to clear the database would be super helpful, at least for developer purposes.

Can add && docker-compose rm -fsv to script so after you kill the up command the rm command is run.

There is no autoclean argument when using just docker compose up. In 0cfa352 added prune command to clean up.

Reset db command was added in 055807a .

Peter9192 · 2023-08-14T15:30:54Z

README.md

+(Stores data in `./postgres-data`)
+(You can get a psql shell with `npm run psql:dev`)


Add here how to clear the database? And remove the container?

Add clear command in 0cfa352 and a2c50c9 to remove container

Peter9192 · 2023-08-14T15:32:50Z

README.md

+The database can be initialized with
+
+```sh
+npm run setup


What about migrations? Do I need to run this command always?

Clearified in a2c50c9

Peter9192 · 2023-08-14T15:34:28Z

README.md

-If not set, a hardcoded secret is used, which should not be used in production.
-
-The data of the login sessions in stored in the `./sessions` directory.
+See [docs/auth.md](docs/auth.md).


Might be good to say here already that the first user is automatically admin.

Peter9192 · 2023-08-14T15:40:20Z

README.md

+The haddock3 web application must be trusted by the bartender web service using a JWT token.
+An RSA private key is used by the haddock3 web application to sign the JWT token.
+To tell the haddock3 web application where to find the private key, use the `BARTENDER_PRIVATE_KEY` environment variable.


I think this explananation is a bit confusing. What about:

Suggested change

The haddock3 web application must be trusted by the bartender web service using a JWT token.

An RSA private key is used by the haddock3 web application to sign the JWT token.

To tell the haddock3 web application where to find the private key, use the `BARTENDER_PRIVATE_KEY` environment variable.

The haddock3 web application can prove its identity to the bartender web service using a JWT token.

An RSA private key is used by the haddock3 web application to sign the JWT token.

To tell the haddock3 web application where to find the private key, use the `BARTENDER_PRIVATE_KEY` environment variable.

Peter9192 · 2023-08-15T07:04:16Z

README.md

+
+```shell
+npm install
+cp .env.example .env


Add a note/guideline on changing the secret?

Done in 0cfa352

Peter9192

Good stuff! I found a few things in the docker compose file, further mostly some clarification suggestions. While browsing to remix docs, I noticed that they mostly like cookiesessionstorage because you don't need a database for them. So I was wondering, since we have a database anyway, is this really the most suitable choice for storing session info? Furthermore, it would be nice to have a dedicated error page in the same style as the rest of the app. And it would be helpful to clarify (more explicitly) that bartender should not be setting roles, this is done by the web app

Peter9192 · 2023-08-15T10:21:26Z

docker-compose.yml

+      - type: bind
+        source: ./private_key.pem
+        target: /app/src/private_key.pem


Shouldn't the web app be the sole owner (and user) of the private key? Bartender should only need the public key to verify the token, right?

Suggested change

- type: bind

source: ./private_key.pem

target: /app/src/private_key.pem

- type: bind

source: ./public_key.pem

target: /app/src/public_key.pem

Peter9192 · 2023-08-15T13:25:02Z

docker-compose.yml

    environment:
      BARTENDER_API_URL: "http://bartender:8000"
+      DATABASE_URL: postgresql://postgres:postgres@webappdb:5432/postgres


Suggested change

DATABASE_URL: postgresql://postgres:postgres@webappdb:5432/postgres

DATABASE_URL: postgresql://postgres:postgres@webappdb:5432/postgres

BARTENDER_PRIVATE_KEY: /app/src/private_key.pem

Added in 6cfcd2e

Peter9192 · 2023-08-15T13:25:45Z

docker-compose.yml

+        source: ./public_key.pem
+        target: /app/src/public_key.pem


Shouldn't the web app need the private key to generate tokens?

Suggested change

source: ./public_key.pem

target: /app/src/public_key.pem

source: ./private_key.pem

target: /app/src/private_key.pem

Peter9192 · 2023-08-15T13:50:52Z

app/routes/login.tsx

+              id="password"
+              name="password"
+              type="password"
+              minLength={8}


Do we need this min length check on login as well? Since you can't have a password of <8 characters, the password would be wrong anyway. But it's not like this is checking it's valid.

You are right, removed length check on server and client in 6cfcd2e

Peter9192 · 2023-08-15T14:06:36Z

README.md

+## Setup
+


Maybe start with a note that you need to have a running instance of bartender setup before starting this? And point to the section below on how to set it up?

Peter9192 · 2023-08-15T14:36:10Z

app/bartender_token.server.ts

+    // If bartender has been  configured with allowed_roles for an application,
+    // then the a role claim should be in the JWT.


That useful info is a bit hidden 🤔

In 5627bb4 moved to https://github.com/i-VRESSE/bartended-haddock3/blob/remix-auth/docs/bartender.md#haddock3-application

Peter9192 · 2023-08-15T14:42:41Z

app/components/admin/UserTableRow.tsx

        <input
          type="checkbox"
-          checked={user.isSuperuser}
+          checked={user.isAdmin}
          className="checkbox"
          disabled={submitting}
          onChange={() => {
            const data = new FormData();
-            data.set("isSuperuser", user.isSuperuser ? "false" : "true");
+            data.set("isAdmin", user.isAdmin ? "false" : "true");
            onUpdate(data);
          }}


Disable if there's only one admin left?

Peter9192 · 2023-08-15T14:43:18Z

app/components/admin/UserTableRow.tsx

+      <td>
+        <button className="btn-sm btn" disabled>
+          Change password
+        </button>
+      </td>


Confusing to have placeholders that are not implemented without explicitly stating so.

Peter9192 · 2023-08-15T14:43:44Z

app/components/admin/UserTableRow.tsx

+      <td>
+        <input type="checkbox" disabled />
+      </td>


Unclear why this is disabled by default. Remove or explain?

Removed in fcc7c68

Peter9192 · 2023-08-15T14:48:30Z

app/models/user.server.ts

-export async function listRoles(accessToken: string) {
-  const api = buildRolesApi(accessToken);
-  return await api.listRoles();
+export async function assignExpertiseLevel(


Should this also upgrade their jwt token?

Nope expertise level is not in token

See https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.1

Already comparing with bcrypt

sverhoeven · 2023-08-21T12:35:22Z

Good stuff! I found a few things in the docker compose file, further mostly some clarification suggestions. While browsing to remix docs, I noticed that they mostly like cookiesessionstorage because you don't need a database for them. So I was wondering, since we have a database anyway, is this really the most suitable choice for storing session info? Furthermore, it would be nice to have a dedicated error page in the same style as the rest of the app. And it would be helpful to clarify (more explicitly) that bartender should not be setting roles, this is done by the web app

The users browser needs to persist something between page loads we are storing the encrypted user id in the cookie just like https://github.com/remix-run/blues-stack/blob/main/app/session.server.ts#L78 . If you would store session id in cookie and session to user mapping as a db table there are more moving parts without much benefit.

For errors you already created #22 so will deal with better error pages when fixing that issue.

Thanks for reviewing this monster pull request and fixing the deployment error I should have caught.

sverhoeven added 2 commits June 30, 2023 14:41

Use remix-auth for local user

61a6f61

A todo list

0a7a3b6

x

sverhoeven mentioned this pull request Jul 19, 2023

JWT token only i-VRESSE/bartender#73

Merged

4 tasks

sverhoeven and others added 27 commits July 19, 2023 15:44

Able to login with dummy check

369d7be

Trying to to do all things

b01d85a

type fix

e3af0d0

Got GH login to work + bartender token generation in theory

03dee09

Fix lint

1f49596

Make user more haddock3 specific

ddb9d99

Make login button easier to see

b0b471e

Regenerate client

c836fba

Store bartender token in db + make preferred expertise level configur…

aa50570

…able by user + lots of other stuff to make things work and look cleaner

Use verified email from GH account

4b7f6f2

Added untested EGI and Orcid auth strategies

a76ac00

Make offering to the lint gods

3c47a56

Move social login docs from bartender to here

8408972

Tested orcid sandbox, orcid, egi development social logins

5ae665d

Run formatter

e47528d

TODO has been moved to #47 description

1493771

Upgrade to latest remix

07cf174

Versions and changed code from `npx create-remix@latest` run in temp dir.

Use validot on register form + Write down tech choices

e37c357

Switched to remix v2 route file naming + switched to postgresql + doc…

b68e0c7

…ker fixed

Format

e5b166e

To ignore postgres-data during format had to add ignore in command. See prettier/prettier#11568 (comment)

More format

13eff96

fix todo

631959a

Remove console.log

177bec2

Remove console.log

335c13d

Add validation to login form

b9563c0

Go over todos

b5ffdf4

Install packages we import

ed7aead

sverhoeven marked this pull request as ready for review August 9, 2023 15:13

sverhoeven added 3 commits August 10, 2023 12:41

generate svg from email as gravatar

bfa4ed1

Redirect messes up type of actionData so specify type instead of infe…

4a73c29

…ring

Format

c2d6ad2

sverhoeven requested a review from Peter9192 August 10, 2023 10:58

Orcid & EGI lack profile.photos, fallback to generate photo

ab34407

Peter9192 reviewed Aug 15, 2023

View reviewed changes

Peter9192 approved these changes Aug 15, 2023

View reviewed changes

sverhoeven added 12 commits August 21, 2023 08:47

Remove unimplemented batch and action columns

fcc7c68

Add confirmation when toggling admin

3821052

Bind keys to right service

466663e

Use same postgres image

a477c35

Haddock3 web app is issuer

441c87d

See https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.1

Improve docs

055807a

Added secret generate command

0cfa352

Add prune

a2c50c9

Move useful info to docs

5627bb4

Remove password length check during login

6cfcd2e

Already comparing with bcrypt

When password was never set then locallogin always fails

78ec595

Format

b63c372

sverhoeven mentioned this pull request Aug 21, 2023

Better error handling #22

Closed

sverhoeven merged commit 51c98f9 into main Aug 21, 2023
1 check passed

sverhoeven deleted the remix-auth branch August 21, 2023 12:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remix auth #47

Remix auth #47

sverhoeven commented Jul 19, 2023 •

edited

Loading

Peter9192 left a comment

Peter9192 Aug 14, 2023

sverhoeven Aug 21, 2023

sverhoeven Aug 21, 2023

Peter9192 Aug 14, 2023

Peter9192 Aug 14, 2023

sverhoeven Aug 15, 2023

sverhoeven Aug 21, 2023

Peter9192 Aug 14, 2023

sverhoeven Aug 21, 2023

Peter9192 Aug 14, 2023

sverhoeven Aug 21, 2023

Peter9192 Aug 14, 2023

Peter9192 Aug 14, 2023

Peter9192 Aug 15, 2023

sverhoeven Aug 21, 2023

Peter9192 left a comment

Peter9192 Aug 15, 2023

Peter9192 Aug 15, 2023

sverhoeven Aug 21, 2023

Peter9192 Aug 15, 2023

Peter9192 Aug 15, 2023

sverhoeven Aug 21, 2023

Peter9192 Aug 15, 2023

Peter9192 Aug 15, 2023

sverhoeven Aug 21, 2023

Peter9192 Aug 15, 2023

Peter9192 Aug 15, 2023

Peter9192 Aug 15, 2023

sverhoeven Aug 21, 2023

Peter9192 Aug 15, 2023

sverhoeven Aug 21, 2023

sverhoeven commented Aug 21, 2023

		A super user can be made through the admin page (`/admin/users`) or by being the first registered user.

		(Stores data in `./postgres-data`)
		(You can get a psql shell with `npm run psql:dev`)

	DATABASE_URL: postgresql://postgres:postgres@webappdb:5432/postgres
	DATABASE_URL: postgresql://postgres:postgres@webappdb:5432/postgres
	BARTENDER_PRIVATE_KEY: /app/src/private_key.pem

		// If bartender has been configured with allowed_roles for an application,
		// then the a role claim should be in the JWT.

Remix auth #47

Remix auth #47

Conversation

sverhoeven commented Jul 19, 2023 • edited Loading

Peter9192 left a 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

Peter9192 left a 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

sverhoeven commented Aug 21, 2023

sverhoeven commented Jul 19, 2023 •

edited

Loading