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.

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

feat!: securing the Admin UI #2588

Merged
merged 39 commits into from Apr 23, 2024
Merged

feat!: securing the Admin UI #2588

merged 39 commits into from Apr 23, 2024

Conversation

JoblersTune
Copy link
Collaborator

@JoblersTune JoblersTune commented Mar 20, 2024

Changes proposed in this pull request

  • Securing the Admin UI using Ory Kratos

Context

fixes #2200
fixes #2492
fixes #2495
fixes #2494
fixes #2493

Checklist

  • Related issues linked using fixes #number
  • Tests added/updated
  • Documentation added
  • Make sure that all checks pass
  • Bruno collection updated

@JoblersTune JoblersTune self-assigned this Mar 20, 2024
@github-actions github-actions bot added pkg: frontend Changes in the frontend package. type: documentation Improvements or additions to documentation labels Mar 20, 2024
Copy link

netlify bot commented Mar 20, 2024

Deploy Preview for brilliant-pasca-3e80ec canceled.

Name Link
🔨 Latest commit 2682599
🔍 Latest deploy log https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/66260f698611d80008660721

@JoblersTune JoblersTune changed the title Securing the Admin UI feat: securing the Admin UI Mar 20, 2024
@JoblersTune
Copy link
Collaborator Author

JoblersTune commented Mar 20, 2024

This is a work in progress. Note all the TODO comments in the code as well as all outstanding issues for this PR which also list the remaining items that need attention.

To try out this PR:

  1. Hopefully the README instructions are clear
  2. pnpm localenv:compose up
  3. docker exec -it admin-container-name npm run invite-user -- example@mailer.com where admin-container-name should be the actual docker container name of whichever ASE you are acting as (either cloud-nine, or happy-life). E.g. docker exec -it rafiki-cloud-nine-admin-1 npm run invite-user -- example@mailer.com
  4. This script will log a recovery link to the console. Copy that link into the browser and it will direct you to a user settings page where you can enter a password, save it and you're good to go.
  5. You can also delete a user with this script docker exec -it rafiki-cloud-nine-admin-1 npm run delete-user -- example@mailer.com

Points I'd love feedback on:

  1. Please give me specific feedback on error handling. I always feel unsure what is best.
  2. Please also give feedback on the TODO items which are questions in the code.
  3. Is there a cleaner way to check the Kratos session without needing so much repeated code in every loader?
    (Note it is not sufficient to only do the check in the root file because of how Remix works that check doesn't necessarily run when navigating on the same page only if you open a new tab)

@github-actions github-actions bot added the type: localenv Local playground label Mar 22, 2024
@JoblersTune JoblersTune marked this pull request as ready for review March 22, 2024 17:57
@BlairCurrey
Copy link
Contributor

  1. Is there a cleaner way to check the Kratos session without needing so much repeated code in every loader?
    (Note it is not sufficient to only do the check in the root file because of how Remix works that check doesn’t necessarily run when navigating on the same page only if you open a new tab)

I take your word for it, and that's consistent with what I'm reading, but I actually can't reproduce. I removed all redirects except for the root one and still cannot access any of the routes (gets redirected to /auth).

Apparently remix suggests just reusing some function everywhere, so not really anything different than what we have. https://remix.run/docs/en/main/guides/faq#how-can-i-have-a-parent-route-loader-validate-the-user-and-protect-all-child-routes

It appears this is a common problem for people and there has been lots of talk about remix middleware but nothing yet:

@JoblersTune

This comment was marked as resolved.

@BlairCurrey

This comment was marked as resolved.

chore: including a Admin UI registration link automatically when running in dev mode
@github-actions github-actions bot added the pkg: documentation Changes in the documentation package. label Apr 15, 2024
Copy link
Contributor

@mkurapov mkurapov left a comment

Choose a reason for hiding this comment

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

A few comments, but basically there 👍

This should be a breaking change, correct? If so we should add "!" in the title to signify this:

feat!: securing the Admin UI

(This makes it easier to generate release notes with the different breaking changes)

@@ -1,13 +1,14 @@
{
"include": ["remix.env.d.ts", "**/*.ts", "**/*.tsx"],
"compilerOptions": {
"lib": ["DOM", "DOM.Iterable", "ES2019"],
"lib": ["DOM", "DOM.Iterable", "ES2019", "ESNext", "ESNext.Promise"],
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need ES2019 if we have ESNext?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't remember anymore at what point I included these. They don't seem to be necessary. So I removed the ESNext stuff and we can keep with ES2019.


RUN --mount=type=cache,id=pnpm,target=/pnpm/store \
pnpm fetch \
| grep -v "cross-device link not permitted\|Falling back to copying packages from store"

FROM base AS prod-deps
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess HMR necessitates different run commands and dev deps but I wonder if we could unify it a bit more up to that point?

If you only changed the CMD in the original Dockerfile to either run remix-serve or remix-build, would it still work?


We have secured access to the Admin UI using [Ory Kratos](https://www.ory.sh/docs/kratos/ory-kratos-intro), a secure and fully open-source identity and user management solution. Check it out on [GitHub](https://github.com/ory/kratos). Since access to the UI is on an invitation-only basis the registration flow is not publicly available. As such, in order to access the UI you'll need to add a new user with the invite-user script. Run `docker exec -it <admin-container-name> npm run invite-user -- example@mail.com` and it will output recovery link to the terminal. The recovery link doubles as the invitation method. Copy and paste this link in your browser and you will automatically be logged in and directed to the account settings page. The next step is changing your password. We're using a simple email and password authentication method.

We've also included a script to remove users: `docker exec -it <admin-container-name> npm run delete-user -- example@mail.com`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can explicitly state rafiki-cloud-nine-admin-1 or rafiki-happy-life-admin-1 somewhere here to make it even easier

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

| If you only changed the CMD in the original Dockerfile to either run remix-serve or remix-build, would it still work?

It takes me down a rabbit hole of issues, I have to change pnpm to npm for the command, then I must ensure the build steps are remove because tsc is no longer available, fair enough, since we build a few lines above, but then the next is "Error: unable to determine transport target for "pino-pretty"" And I don't think we'd want to move that out of dev dependencies. So we can keep trying to jump over these hurdles but ultimately we are running a dev build process without our dev dependencies which is not ideal.

currently remix-run/dev is a dev dependency which I feel it should be - needed to run remix in dev mode
tailwindcss is a dev dependency to build our css files
and pino-pretty is a dev dependency to make our logs prettier for dev use

So in short, I'd argue, no, it doesn't work.

@JoblersTune JoblersTune changed the title feat: securing the Admin UI feat!: securing the Admin UI Apr 17, 2024
@mkurapov
Copy link
Contributor

Is there a way we can change this message to not mention social-sign in? (and maybe format the minutes nicely?)

Screenshot 2024-04-18 at 12 35 28

@melissahenderson melissahenderson removed their request for review April 18, 2024 11:07
@JoblersTune
Copy link
Collaborator Author

Is there a way we can change this message to not mention social-sign in? (and maybe format the minutes nicely?)

Screenshot 2024-04-18 at 12 35 28

There isn't a good way to change this. It's a generic message returned from Kratos. They have had other people complain about this before too. There is no way to customise it and you can't easily choose not to display it without also not displaying other messages you may receive, like, "Your changes have been saved!"

The only way I could see how to change this is check if this is the message we receive and in that case rather display our own custom message but that feels very messy.

@JoblersTune JoblersTune merged commit 4f501cb into main Apr 23, 2024
69 checks passed
@JoblersTune JoblersTune deleted the sj/2200-admin-ui-auth branch April 23, 2024 14:12
@sabineschaller sabineschaller added the breaking Issue/PR that introduces breaking changes label May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Issue/PR that introduces breaking changes pkg: documentation Changes in the documentation package. pkg: frontend Changes in the frontend package. type: documentation Improvements or additions to documentation type: localenv Local playground
Projects
None yet
4 participants