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

App Submission: Phantombot #1608

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from
Draft

App Submission: Phantombot #1608

wants to merge 11 commits into from

Conversation

kriakiku
Copy link

@kriakiku kriakiku commented Oct 8, 2024

App Submission

App name

Phantombot

Disclamer

  • The service is required to run over HTTPS; otherwise, users won’t be able to connect their Twitch accounts. As a result, the proxy_server is not included in the docker-compose.
  • I had to retain the root user, as the application fails to start otherwise.

256x256 SVG icon

https://svgshare.com/i/1BJ0.svg

Phantombot

Gallery images

1.png:
Phantombot

1.png (original):
umbrel local_25000_panel_

2.png:
umbrel local_25000_panel_ (1)

3.png:
umbrel local_25000_panel_ (2)

4.png:
umbrel local_25000_

5.png:
umbrel local_25000_oauth_

I have tested my app on:

  • umbrelOS on a Raspberry Pi
  • umbrelOS on an Umbrel Home
  • umbrelOS on Linux VM

@kriakiku
Copy link
Author

kriakiku commented Oct 8, 2024

Copy link

github-actions bot commented Oct 8, 2024

🎉   Linting finished with no errors or warnings   🎉

Thank you for your submission! This is an automated linter that checks for common issues in pull requests to the Umbrel App Store.

Please review the linting results below and make any necessary changes to your submission.

Linting Results

Severity File Description
ℹ️ phantombot/docker-compose.yml External port mapping "${APP_PHANTOM_SERVER_PORT}:${APP_PHANTOM_SERVER_PORT}":
Port mappings may be unnecessary for the app to function correctly. Docker's internal DNS resolves container names to IP addresses within the same network. External access to the web interface is handled by the app_proxy container. Port mappings are only needed if external access is required to a port not proxied by the app_proxy, or if an app needs to expose multiple ports for its functionality (e.g., DHCP, DNS, P2P, etc.).

Legend

Symbol Description
Error: This must be resolved before this PR can be merged.
⚠️ Warning: This is highly encouraged to be resolved, but is not strictly mandatory.
ℹ️ Info: This is just for your information.

@kriakiku
Copy link
Author

kriakiku commented Oct 9, 2024

@nmfretz Hi! Is there anything else required from me for the approval of this merge request?

@kriakiku
Copy link
Author

kriakiku commented Oct 25, 2024

image

Copy link
Contributor

@nmfretz nmfretz left a comment

Choose a reason for hiding this comment

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

Thanks for submitting PhantomBot @kriakiku. This looks like a fantastic addition!

Really sorry for the delay in reviewing this. I'll try and be more prompt in follow-ups here, but sometimes I'll have my focus elsewhere.

I've left a review below. Once addressed I can give the app a test. Let me know if anything is confusing or if I've misunderstood something and I can take another look.

@@ -0,0 +1,14 @@
services:
Copy link
Contributor

Choose a reason for hiding this comment

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

Umbrel's app proxy service should be added here as the first service, which can then point to the phantombot_server_1 container and default port 25000. That way you can remove the port mapping in your server service and then PhantomBot will running behind our transparent proxy and inherit the security properties of our umbrel auth:

firefox example to follow

services:
app_proxy:
environment:
APP_HOST: firefox_server_1
APP_PORT: 3000

  • By default, we only let requests through the proxy if the user already has a valid auth cookie from the Umbrel homescreen. So if the PhantomBot dashboard has no auth of its own, it will still be protected by ours. This gives a good UX because there's zero friction for Umbrel users... they can just access the dashboard without re-entering any credentials if they are already logged in to their Umbrel.
  • It also has the benefit of inheriting other security properties of Umbrel auth, such as 2FA if they have it enabled on their Umbrel. They would then get 2FA security for all apps behind the auth proxy too.
  • And then if you need to allow external connections to PhantomBot running on port 25000 (e.g., not to the UI but to the api or something), you can whitelist certain routes, so that the web UI is protected by auth, but something like /api/* isn't:

e.g., adding this env var to the proxy container

PROXY_AUTH_WHITELIST: "/api/*"
  • You can also disable the auth portion of the app proxy container entirely with this env var:
PROXY_AUTH_ADD: "false"

services:
server:
image: ghcr.io/phantombot/phantombot:3.14.1.0@sha256:dbec9818e40f967ac5aee3abcac5a1857481cbbe6b35400f9a2fa8f1dc638df0
user: 0:900
Copy link
Contributor

Choose a reason for hiding this comment

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

If possible, we want to avoid running services as root. I haven't looked at the phantombot Dockerfile to see how they build the image so we might be stuck with this. But we should check if it is possible to run this non-root as the umbrel user (1000:1000).

volumes:
- ${APP_DATA_DIR}/data:/opt/PhantomBot_data
ports:
- "${APP_PHANTOM_SERVER_PORT}:${APP_PHANTOM_SERVER_PORT}"
Copy link
Contributor

@nmfretz nmfretz Oct 28, 2024

Choose a reason for hiding this comment

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

This ports section can be removed entirely once the app proxy service is added

ports:
- "${APP_PHANTOM_SERVER_PORT}:${APP_PHANTOM_SERVER_PORT}"
environment:
PHANTOMBOT_USEHTTPS: "true"
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't tested this submission yet, but I'd imagine this forces user to use https, so they'd click the app and go to https://umbrel.local:25000, is that right? If so they'll be met with the big scary insecure warning that we shouldn't be teaching the average umbrel user to just click through.

We can set this to false, so the user is accessing over http on their local network.

PHANTOMBOT_USEHTTPS: "true"
PHANTOMBOT_PANELUSER: umbrel
PHANTOMBOT_PANELPASSWORD: $APP_PASSWORD
PHANTOMBOT_BASEPORT: $APP_PHANTOM_SERVER_PORT
Copy link
Contributor

@nmfretz nmfretz Oct 28, 2024

Choose a reason for hiding this comment

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

if this is the default port that phantombot runs on, then this can be removed entirely #1608 (comment)

@@ -0,0 +1 @@
export APP_PHANTOM_SERVER_PORT="25000"
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is the default port, then this entire exports.sh should be removed for simplicity

name: PhantomBot
version: "3.14.1.0"
tagline: PhantomBot is a Twitch chat bot powered by Java
icon: ""
Copy link
Contributor

Choose a reason for hiding this comment

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

This entire icon line can be removed. We'll host the icon here: https://github.com/getumbrel/umbrel-apps-gallery

And the icon rendering logic in umbrelOS will grab the correct icon.

Comment on lines +14 to +24
🛠️ Set-Up Instructions
Required! If you don't perform the initial setup, you will encounter an error when connecting to the control panel!

1. Connect Your Twitch Account
Using the instructions on the page https://umbrel.local:25000/oauth/, create oauth app and connect your Twitch account.

2. Fill Chanel Configuration Fields
Complete the channel and owner fields on the configuration page https://umbrel.local:25000/setup/.

3. Log In
Done! You can log in to the control panel at https://umbrel.local:25000/panel/login using the default credentials.
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome work adding these!

If users can navigate to these links by clicking things within the UI, then I would suggest making these instructions more generic. For example, telling the user to "navigate to the oauth settings" instead of providing a specific link.

My reasoning here is that:

  1. These links may change in future app updates and it will be difficult to remember to update the app description

  2. It's possible that the user accesses their umbrel at a different hostname than umbrel.local. For example, they may access their umbrel via local IP address, or by Tailscale, or if they have multiple Umbrel devices they may be using umbrel-2.local

@kriakiku
Copy link
Author

Hey! I’ll try to make the changes in the coming days.

@nmfretz
Copy link
Contributor

nmfretz commented Nov 21, 2024

image

Converting to draft for now to keep things organized. @kriakiku ping me as soon as you're ready for me to test!

@nmfretz nmfretz marked this pull request as draft November 21, 2024 03:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants