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: Mutiny Wallet #785

Closed
wants to merge 2 commits into from

Conversation

benthecarman
Copy link

@benthecarman benthecarman commented Sep 28, 2023

@benthecarman benthecarman force-pushed the mutiny-web branch 5 times, most recently from 290cd39 to 35bbe71 Compare September 29, 2023 04:48
@benthecarman benthecarman marked this pull request as ready for review September 29, 2023 04:48
@benthecarman
Copy link
Author

Ready for review!

@ParthJadhav
Copy link
Contributor

#787
Ran workflow from above PR, Let me know if something isn't right.

Here is the result:

Checklist:

docker-compose.yml

  • 🛑 Image ghcr.io/mutinywallet/mutiny-web:v0.4.20@sha256:16e4730bf0049453a74bc41cd622c4d37e631fc2d305b0cb7188a323c7c301eb does not support both linux/amd64 and linux/arm architectures
  • ✅ Versioned Images
  • ✅ No proxy auth errors

umbrel-app.yml

  • 🛑 dependencies, : required fields missing in umbrel-app.yml
  • ✅ All optional fields present
  • ✅ Category value is correct

@ParthJadhav
Copy link
Contributor

dependencies is required because currently there's a bug on the frontend which causes the app page to not render if dependencies are missing.

Will be fixed by: getumbrel/umbrel#1705

@benthecarman
Copy link
Author

@ParthJadhav fixed!

@ParthJadhav
Copy link
Contributor

Hey @benthecarman that looks good. Thanks for the changes.

When I tested it locally it worked properly but when I tested it on a cloud instance where I had multiple other apps. It showing me snort - nostr interface instead of mutiny-wallet.

I even deleted snort - nostr and restarted Umbrel. Still the problem persists.


beside from that, When I go to receive money there's a copy to clipboard button won't working since the connection is over http and not https (Ref). This can be solved by doing something like this. OR the full key can be shown so the user can copy it manually.

@ok300
Copy link
Contributor

ok300 commented Sep 30, 2023

When I tested it locally it worked properly but when I tested it on a cloud instance where I had multiple other apps. It showing me snort - nostr interface instead of mutiny-wallet.

Not 100% sure, but this might be due to an APP_PORT conflict. Snort is also using APP_PORT: 80.

@benthecarman you have APP_PORT: 80 in your docker-compose.yml. It might work if you choose one that's not used by anyone else (see query for used ports).

Had a similar issue months ago and that's what fixed it for me.

@ParthJadhav what do you think? Could this be it?

@ParthJadhav
Copy link
Contributor

When I tested it locally it worked properly but when I tested it on a cloud instance where I had multiple other apps. It showing me snort - nostr interface instead of mutiny-wallet.

Not 100% sure, but this might be due to an APP_PORT conflict. Snort is also using APP_PORT: 80.

@benthecarman you have APP_PORT: 80 in your docker-compose.yml. It might work if you choose one that's not used by anyone else (see query for used ports).

Had a similar issue months ago and that's what fixed it for me.

@ParthJadhav what do you think? Could this be it?

The app ports seems to be fine.

APP_PORT: 80 in APP_PROXY points to the port inside the container. port: 14499 in umbrel-app.yml is the one which is exposed for users to use.

http://umbrel-os.local:14499 (from your browser) -> http://localhost:80 (Inside the container)

@benthecarman
Copy link
Author

@ParthJadhav that sounds like a port issue, are you sure APP_PORT: 80 isn't the problem?

@benthecarman
Copy link
Author

Tried something that might work

Copy link
Contributor

@ParthJadhav ParthJadhav left a comment

Choose a reason for hiding this comment

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

Amazing, That works now. Only thing remaining this to be merged.

@TonyGiorgio
Copy link

Recommending the disability of security features is the wrong approach. Further, it's not just the copy button, there's other browser APIs that depend on HTTPS like camera. And your suggestion to use deprecated APIs and methods is not good.

This should only be ran in an HTTPS context, either umbrel should provide this or users should not use HTTPS workarounds but use this with tailscale explicitly.

@ParthJadhav
Copy link
Contributor

Hey @TonyGiorgio,

These are some thread regarding the security concerns:

https://github.com/getumbrel/umbrel/blob/master/SECURITY.md
getumbrel/umbrel#985 (comment)

It's you're right that it's not optimal to disable the security features but it's the team's first priority and hopefully it would be resolved soon.

Until then it would be amazing if we could have the features proposed in the above PR.

@TonyGiorgio
Copy link

Hey @TonyGiorgio,

These are some thread regarding the security concerns:

https://github.com/getumbrel/umbrel/blob/master/SECURITY.md
getumbrel/umbrel#985 (comment)

It's you're right that it's not optimal to disable the security features but it's the team's first priority and hopefully it would be resolved soon.

Until then it would be amazing if we could have the features proposed in the above PR.

It's not going to work for us. Https is a requirement. It's far more than just a copy button not working.

What options do we have in having umbrel users installing certs and informing them this is a requirement?

@nmfretz
Copy link
Contributor

nmfretz commented Oct 5, 2023

Thanks for submitting Mutiny Wallet @benthecarman! Excited for this to be self-hosted.

It's not going to work for us. Https is a requirement. It's far more than just a copy button not working.

What options do we have in having umbrel users installing certs and informing them this is a requirement?

Understood @TonyGiorgio. Let us discuss internally and get back to you.

@TonyGiorgio
Copy link

Thanks for submitting Mutiny Wallet @benthecarman! Excited for this to be self-hosted.

It's not going to work for us. Https is a requirement. It's far more than just a copy button not working.
What options do we have in having umbrel users installing certs and informing them this is a requirement?

Understood @TonyGiorgio. Let us discuss internally and get back to you.

I believe I've seen people manually set up their own DNS or tailscale on their umbrel's before. Even if it's something we have to instruct users in the umbrel description, that's an option we can work with.

@nmfretz
Copy link
Contributor

nmfretz commented Jan 17, 2024

@TonyGiorgio @benthecarman, sorry for the delay.

We discussed this internally. The https-via-tailscale option may be too finicky for users to set up since it will require running tailscale commands in docker and also configuring the main umbrel nginx which then won't persist on update.

One interim option while umbrelOS doesn't support https out-of-the-box could be to serve an SSL cert as part of the mutiny web server. This would involve leg-work on your end in order to implement and walk users through the UX of accepting the cert if http is detected. This option might break the auth proxy so that may need to be disabled.

Another possible option with different drawbacks is to instruct users to set up a cloudflare tunnel. There is now a cloudflare app on Umbrel which walks users through setting up a tunnel and public hostnames: https://apps.umbrel.com/app/cloudflared

  • This would require users to configure a hostname to the mutiny nginx container directly (bypassing umbrel auth).
  • The drawback here is that the tunnel would be public and anyone could access the mutiny web server since there is no in-built auth.
  • We could include the cloudflare app as a dependency in the Mutiny app's umbrel-app.yml so that users are required to install it before installing Mutiny (although this doesn't stop them from not using it afterwards).

Obviously we can just wait to add Mutiny Wallet to the app store until we have https support in umbrelOS, but I wanted to give you some options to consider.

@TonyGiorgio
Copy link

Thanks for that information.

We've been very happy with our start9 integration and I think we'll leave it at that for now. That's more effort than I think we (and what I expect out of your type of users) to be able to setup and I don't want to support it.

@benthecarman can you close this? We've gone through a few iterations of docker versioning and setup since then.

@benthecarman benthecarman deleted the mutiny-web branch January 17, 2024 22:11
@nmfretz
Copy link
Contributor

nmfretz commented Jan 18, 2024

Thanks for your efforts on this anyways @benthecarman and @TonyGiorgio, we really appreciate it.

We'll reach out when we are providing https out-of-the-box. In the meantime, best of luck with continued success for Mutiny Wallet!

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.

5 participants