-
-
Notifications
You must be signed in to change notification settings - Fork 681
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
Add disable option to web-root #249
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
- Can you add docs? Mostly the table, I don't think we need a special section for this in the config docs; just the table(s) are enough.
- And the
server.yml
entry - And maybe a quick test for it.
Thanks
server/config.go
Outdated
@@ -126,5 +127,6 @@ func NewConfig() *Config { | |||
VisitorEmailLimitBurst: DefaultVisitorEmailLimitBurst, | |||
VisitorEmailLimitReplenish: DefaultVisitorEmailLimitReplenish, | |||
BehindProxy: false, | |||
DisableWebUI: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you name this WebEnabled
instead and make it true
by default? That will invert the if
statements.
cmd/serve_linux.go
Outdated
@@ -55,6 +56,7 @@ var flagsServe = []cli.Flag{ | |||
altsrc.NewIntFlag(&cli.IntFlag{Name: "visitor-email-limit-burst", EnvVars: []string{"NTFY_VISITOR_EMAIL_LIMIT_BURST"}, Value: server.DefaultVisitorEmailLimitBurst, Usage: "initial limit of e-mails per visitor"}), | |||
altsrc.NewDurationFlag(&cli.DurationFlag{Name: "visitor-email-limit-replenish", EnvVars: []string{"NTFY_VISITOR_EMAIL_LIMIT_REPLENISH"}, Value: server.DefaultVisitorEmailLimitReplenish, Usage: "interval at which burst limit is replenished (one per x)"}), | |||
altsrc.NewBoolFlag(&cli.BoolFlag{Name: "behind-proxy", Aliases: []string{"P"}, EnvVars: []string{"NTFY_BEHIND_PROXY"}, Value: false, Usage: "if set, use X-Forwarded-For header to determine visitor IP address (for rate limiting)"}), | |||
altsrc.NewBoolFlag(&cli.BoolFlag{Name: "disable-webui", Aliases: []string{"d"}, EnvVars: []string{"NTFY_DISABLE_WEBUI"}, Value: false, Usage: "if set, disables WebUI"}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you call this flag disable-web
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize the flag didn't need to have the same name as the variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually. Darn. This is tricky. Because this is the server.yml file config option. I wanted to name it web-enabled
, but boolean flags with a true default don't work. I don't know how to solve this.
Added docs and tests, my editor uses gofumpt btw. |
You're gonna hate me for this, but what about this: why not make the
You could turn the My reasoning is this: I hate having new parameters. And I also hate having a parameter called "disable-x". While that's okay in the CLI, it's super weird in yml. What do you think? |
Looks great. Thank you for your contribution! |
Hey @Curid, fancy finding you on this project, too. Running via Docker method. Here is my docker-compose.yml:
Here is the web-root section of my server.yml: I could be wrong, but it seems I can anonymously access the webui and even the Settings. I am even able to delete my "james" user anonymously, with no authentication required to do so. I would expect only an Admin to have that level of access. |
I think I see now. With the option set to "disable" the root url does throw a 404 error. Adding /app top the end still loads the webui. But the Users don't show in the webui if I load it on another device, so it only shows my "james" user because that is stored locally int he web browser. Sound about right? |
If adding /app at the end of the URL still loads the web app when it's set to disable, that is probably a bug. But yes, you are correct about the local storage. The web app is just a simple client (like the Android app): you can open it and look around, but you cannot publish/subscribe to topics unless you put in auth credentials |
Closes #238