split-pro icon indicating copy to clipboard operation
split-pro copied to clipboard

Customizable auth providers, + some improvements

Open Striffly opened this issue 1 year ago • 15 comments

Several changes about auth providers:

  • I’ve added the SMTP parameters to the EmailProvider.
  • I made the SMTP parameters mandatory: without them, it’s difficult to send emails or properly use the EmailProvider.
  • I created an environment variable AUTH_PROVIDERS, which allows making GoogleProvider optional.
  • The same variable is used to display or hide the login solutions on the frontend.

And some improvements :

  • I made sure that the variable FEEDBACK_EMAIL is used instead of [email protected]
  • I've improved console log message & prevented error when using email magic link auth on dev
  • I've implemented ENABLE_SENDING_INVITES env variable
  • I've added JetBrains .idea to gitignored files

Please test and validate! I’ve been quite proactive with some of the changes, so if any adjustments are needed, don’t hesitate to let me know!

Striffly avatar Sep 08 '24 23:09 Striffly

Yes, my goal is to self-host the project, with minio for files and only the EmailProvider for authentication. I will do a full test shortly, using the Docker image (which I have to rebuild myself to test my changes).

Striffly avatar Sep 09 '24 18:09 Striffly

@Striffly Nice, thanks for the PR. I'll merge it coming week. I'll add you to the self-hoster mailing list. Lemme know if you don't want to be added.

more context: https://github.com/oss-apps/split-pro/discussions/93

KMKoushik avatar Sep 09 '24 22:09 KMKoushik

Okay, so I've added a lot of fixes and changes related to my update of auth providers. I’ve also made a few other adjustments that seemed relevant. I’ve detailed everything in the first post of my PR. Sorry, the PR has become a bit more extensive than before; I hope this isn’t a problem.

I can get it working locally without any issues. However, when I try to run the web app with Docker, I get this error:

❌ Invalid environment variables:
{
    "NEXT_PUBLIC_AUTH_PROVIDERS": [
        "Required"
    ]
}

This is related to this : https://github.com/t3-oss/t3-env/issues/244#issuecomment-2242580707

- for client side variables, the values have to be known at build-time as these are (most likely) statically replaced by your framework. So your Dockerfile must provide these (either from an env file or build args)

So, we have two choices:

  • include client-side variables at Docker build time (not very modular)
  • or load them dynamically

I think it’s better for everyone to be able to set their own values, so I went with the second option. I based my solution on the options indicated here : https://github.com/vercel/next.js/discussions/17641#discussioncomment-10142733

You can find the associated code here : https://github.com/Striffly/split-pro/tree/features/customizable-providers-2 (check the last commit especially) However, I haven’t been able to get this solution to work so far; I’m encountering errors when trying to launch the webapp, related to the combination of use server + context provider+ this code :

      void getPublicEnv().then((env) => {
        setEnvState(env);
      });

@KMKoushik let me know what you think about this, and if you have time to take a look at my code 🙏

[EDIT] Related links :

  • https://github.com/ritingliudd01/nextjs-14-get-runtime-env
  • https://nextjs.org/docs/pages/building-your-application/configuring/environment-variables#runtime-environment-variables
  • https://github.com/vercel/next.js/discussions/44628
  • https://www.vorillaz.com/nextjs-docker-env
  • https://github.com/vercel/next.js/discussions/17641

Striffly avatar Sep 11 '24 04:09 Striffly

@Striffly the solution won't work cuz we don't use app router, i'm moving away from using NEXT_PUBLIC url now as you can see it here. https://github.com/oss-apps/split-pro/commit/592a5d50396309494f73761f8c6c9efaaa1fff5c

so a better solution would be to read these environment variables in the server and give as props! lemme know if you have any question

KMKoushik avatar Sep 11 '24 19:09 KMKoushik

Thanks for your feedback @KMKoushik, thanks to you I solved in 30 minutes this problem I had spent 5 hours on the day before 😅 So now everything's good for my PR!

Striffly avatar Sep 11 '24 22:09 Striffly

The last thing I'd like to add to my PR is a change in the Docker Compose to allow using Minio instead of an AWS bucket. However, I can't connect to it; I keep getting authentication errors.

# prod/compose.yml

name: split-pro-prod

services:
  postgres:
    image: postgres:16
    container_name: splitpro-db-prod
    restart: always
    environment:
      - POSTGRES_USER=${POSTGRES_USER:?err}
      - POSTGRES_PASSWORD=${POSTGRES_PASSWORD:?err}
      - POSTGRES_DB=${POSTGRES_DB:?err}
    healthcheck:
      test: ['CMD-SHELL', 'pg_isready -U ${POSTGRES_USER}']
      interval: 10s
      timeout: 5s
      retries: 5
    # ports:
    #   - "5432:5432"
    volumes:
      - database:/var/lib/postgresql/data

  splitpro:
    # image: ossapps/splitpro:latest
    build: # for testing purpose
      context: ../../
      dockerfile: docker/Dockerfile
    container_name: splitpro
    restart: always
    ports:
      - ${PORT:-3000}:${PORT:-3000}
    environment:
      - PORT=${PORT:-3000}
      - DATABASE_URL=${DATABASE_URL:?err}
      - NEXTAUTH_URL=${NEXTAUTH_URL:?err}
      - NEXTAUTH_SECRET=${NEXTAUTH_SECRET:?err}
      - AUTH_PROVIDERS=${AUTH_PROVIDERS:?err}
      - ENABLE_SENDING_INVITES=${ENABLE_SENDING_INVITES:?err}
      - FROM_EMAIL=${FROM_EMAIL:?err}
      - EMAIL_SERVER_HOST=${EMAIL_SERVER_HOST:?err}
      - EMAIL_SERVER_PORT=${EMAIL_SERVER_PORT:?err}
      - EMAIL_SERVER_USER=${EMAIL_SERVER_USER:?err}
      - EMAIL_SERVER_PASSWORD=${EMAIL_SERVER_PASSWORD:?err}
      - GOOGLE_CLIENT_ID=${GOOGLE_CLIENT_ID}
      - GOOGLE_CLIENT_SECRET=${GOOGLE_CLIENT_SECRET}
      - R2_ACCESS_KEY=${R2_ACCESS_KEY}
      - R2_SECRET_KEY=${R2_SECRET_KEY}
      - R2_BUCKET=${R2_BUCKET}
      - R2_URL=${R2_URL}
      - R2_PUBLIC_URL=${R2_PUBLIC_URL}
      - WEB_PUSH_PRIVATE_KEY=${WEB_PUSH_PRIVATE_KEY}
      - WEB_PUSH_PUBLIC_KEY=${WEB_PUSH_PUBLIC_KEY}
      - WEB_PUSH_EMAIL=${WEB_PUSH_EMAIL}
      - FEEDBACK_EMAIL=${FEEDBACK_EMAIL}
      - DISCORD_WEBHOOK_URL=${DISCORD_WEBHOOK_URL}
    depends_on:
      postgres:
        condition: service_healthy

  minio:
    image: bitnami/minio:latest
    container_name: splitpro-minio-prod
    restart: always
    environment:
      - MINIO_ROOT_USER=${R2_ACCESS_KEY}
      - MINIO_ROOT_PASSWORD=${R2_SECRET_KEY}
      - MINIO_DEFAULT_BUCKETS=${R2_BUCKET}
    ports:
      - '9100:9000'
      - '9101:9001'
    volumes:
      - minio_data:/bitnami/minio/data
    healthcheck:
      test: ["CMD-SHELL", "curl --silent --fail localhost:9000/minio/health/live || exit 1"]
      interval: 30s
      timeout: 20s
      retries: 3

volumes:
  database:
  minio_data:
# .env

# Storage: any S3 compatible storage will work, for self hosting can use minio
R2_ACCESS_KEY=minioadmin
R2_SECRET_KEY=miniopassword
R2_BUCKET=mybucket
R2_URL=http://splitpro-minio-prod:9100
# public url of the storage, https://developers.cloudflare.com/r2/buckets/public-buckets/
R2_PUBLIC_URL=http://localhost:9100

This could be part of a new PR once this one is accepted, but if you have a bit of time, could you take a look at it? 🙏

Striffly avatar Sep 11 '24 22:09 Striffly

@Striffly I'll verify and merge this over the weekend. it's quite a big PR and need to be tested properly.

Amazing work BTW

KMKoushik avatar Sep 12 '24 00:09 KMKoushik

@Striffly This PR is ready to be reviewed and merged right?

KMKoushik avatar Sep 13 '24 20:09 KMKoushik

@KMKoushik yes !

I would have just liked your feedback on my last message regarding the docker-compose, but as I mentioned, it can easily be the subject of another PR, this one is already quite packed :slightly_smiling_face:

Striffly avatar Sep 13 '24 23:09 Striffly

@Striffly sure thing, i'll test this today and let you know, i used minio before, will check.

KMKoushik avatar Sep 13 '24 23:09 KMKoushik

Can't wait to see this merged. It finally lays the ground to custom OAuth providers. See my previous request.

livingsilver94 avatar Sep 17 '24 19:09 livingsilver94

Waiting for this PR to finally start self host it!

thegabriele97 avatar Sep 18 '24 07:09 thegabriele97

And for minio question, you need user name and password https://github.com/unsend-dev/unsend/blob/main/docker/dev/compose.yml#L27

Can take from env for production.

KMKoushik avatar Sep 20 '24 21:09 KMKoushik

TYSM great work, this PR looks good with some minor changes. lemme know if you can work on this or i can do this myself.

I've provided feedback on your comments, I'll leave it to you to decide which changes need to be made or not. I won’t be able to revisit this PR for the next two weeks, so if you have some time before then to do these changes that would be great!

And for minio question, you need user name and password https://github.com/unsend-dev/unsend/blob/main/docker/dev/compose.yml#L27

Can take from env for production.

In the config I mentioned earlier, they are indeed present no ? In your example, you don’t create a bucket: doesn’t that cause an issue? That’s why I used the bitnami/minio image in my case.

Striffly avatar Sep 20 '24 22:09 Striffly

In the config I mentioned earlier, they are indeed present no ? In your example, you don’t create a bucket: doesn’t that cause an issue? That’s why I used the bitnami/minio image in my case.

i never faced the bucket issue, not 100% sure. your docker example looks good. i would re verify the ports and stuff

KMKoushik avatar Sep 21 '24 20:09 KMKoushik

Thank you so much again. this is great

KMKoushik avatar Oct 14 '24 05:10 KMKoushik