Skip to content

[Featute] Add optional web app flag which requires a login for every action #1329

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

theatischbein
Copy link

Problem

I have read #238 and was also notified about #1316 (which I disagree too because the ACL does not allow it).
I do understand that the web app is supposed to be a dump client, but I still want to manage who uses my server ressources. Since the web app has a login it feels not suitable to add an additonal login step like BASIC-AUTH.

But I see the point of #238 that the web app can be used to subscribe to other ntfy servers, which might not be wanted.

Also I find it a bit confusing in my setup that there are options in the NavigationBar to subscribe or publish notifications on my servers without a login, but the ACL does not allow it. And I do not want others (without an account) to use my instance to subscribe to external server.

Changes

So I added the flag require_login/require-login/NTFY_REQUIRE_LOGIN (Default: false).

If the flag is enabled and there is no Login:

  • NavigationBar: hides All Notifications, Settings, Publisish Notification, Subscribe to topic
  • NavigationBar: Documentation and all other possible entries are still there
  • Endpoint /settings redirects back to main page
  • Notification: Informs that a Login is needed to use this page

Screenshot from 2025-05-05 10-54-35

Missing

I only added the translation for English.

@theatischbein theatischbein force-pushed the feat_optional_require_login branch 3 times, most recently from 52c5c6a to e398140 Compare May 5, 2025 09:38
@theatischbein theatischbein force-pushed the feat_optional_require_login branch from e398140 to 03aeb70 Compare May 5, 2025 09:39
@binwiederhier
Copy link
Owner

binwiederhier commented May 24, 2025

This seems to be missing commits (?!). It does not compile. The Config struct is missing the RequireLogin option, and so is the apiConfigResponse. The config.js doesn't have the field either.

The web app also does not compile:

image

@binwiederhier
Copy link
Owner

binwiederhier commented May 24, 2025

So I fixed the missing pieces (see #1351). I tried it and I am a big fan of the require-login flag, though I am not fan of where the "this page requires a login" page is hooked into. I'd much rather it'd be instead of <Main> or even instead of <Layout>, or that we otherwise dynamically route to a different page. I would likely not want the nav/sidebar either.

I was going to get this into the next release, but it may have to wait, since there is much more to look at.

Thank you for your work

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