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

client/web: add new readonly mode #10999

Merged
merged 1 commit into from
Feb 8, 2024
Merged

client/web: add new readonly mode #10999

merged 1 commit into from
Feb 8, 2024

Conversation

willnorris
Copy link
Member

The new read-only mode is only accessible when running tailscale web by passing a new -readonly flag. This new mode is identical to the existing login mode with two exceptions:

  • the management client in tailscaled is not started (though if it is already running, it is left alone)

  • the client does not prompt the user to login or switch to the management client. Instead, a message is shown instructing the user to use other means to manage the device.

I implemented this as a new mode rather than a separate boolean flag on the server, since otherwise it was not obvious to me what it would mean to have a client in manage mode with a read-only flag. Those things are mutually exclusive, so adding a new mode seemed appropriate.

Updates #10979

@@ -58,6 +58,7 @@ type Server struct {
devMode bool
cgiMode bool
pathPrefix string
readonly bool
Copy link
Member

Choose a reason for hiding this comment

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

not used?

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch. That was from before I decided to add a new server mode.

// ReadOnlyServerMode is identical to LoginServerMode,
// but does not present a login button to switch to manage mode,
// even if the management client is running and reachable.
ReadOnlyServerMode ServerMode = "readonly"
Copy link
Member

Choose a reason for hiding this comment

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

nit: maybe add to the docs here why we are adding this

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -94,20 +96,24 @@ func runWeb(ctx context.Context, args []string) error {
if prefs, err := localClient.GetPrefs(ctx); err == nil {
existingWebClient = prefs.RunWebClient
}
if !existingWebClient {
if !existingWebClient && !webArgs.readonly {
Copy link
Member

Choose a reason for hiding this comment

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

I think you also have to check down below in the cleanup on ctx.Done() to not stop it if we were in readonly mode.

Could just add a var here processStartedManagmentClient or something that we can set in this if and just use below.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -224,7 +229,7 @@ function LoginPopoverContent({
) : (
// User is connected over tailscale, but doesn't have permission to manage.
<p className="text-gray-500 text-xs">
You dont have permission to make changes to this device, but you
You don't have permission to make changes to this device, but you
Copy link
Member

Choose a reason for hiding this comment

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

unintentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

this was intentional. This was using a smart quote, which is typographically correct, but we don't actually use it anywhere else, so this change makes things uniform. I believe this came over from copy/pasting the text from Figma.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think our linter catches it over here, but on the admin panel we enforce smart quotes I believe.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, interesting! I'll go look at that real quick and potentially update the other way here

Copy link
Member Author

Choose a reason for hiding this comment

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

reverted this change. I'll do a follow-up PR to add the curly-quotes linter rule and fix the web client.

Comment on lines 167 to 168
This web interface is running in read-only mode. Use the Tailscale
CLI or a different client app to manage this device.
Copy link
Member

Choose a reason for hiding this comment

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

I like having this snippet here, although I fear it's a little confusing because of how general it is. I think we should leave it at "This web interface is running in read-only mode." and provide a link out to a KB that explains a little more based on platform.

Copy link
Member Author

Choose a reason for hiding this comment

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

done. I'm using the existing tailscale.com/s/ KB link for now, since it just goes to the main articles about the web interface. If we do end linking to specific sections of that page, we'll want to switch this for a specific link about readonly mode.

<p className="text-gray-500 text-xs">
This web interface is running in read-only mode.{" "}
<a
href="https://tailscale.com/s/web-client-connection"
Copy link
Member

Choose a reason for hiding this comment

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

Anything set up over there yet?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not specific to read-only mode. I'll actually go ahead and make this a dedicated short link for readonly mode, and then work on a PR to add docs before the 1.60 release.

@@ -224,7 +229,7 @@ function LoginPopoverContent({
) : (
// User is connected over tailscale, but doesn't have permission to manage.
<p className="text-gray-500 text-xs">
You dont have permission to make changes to this device, but you
You don't have permission to make changes to this device, but you
Copy link
Member

Choose a reason for hiding this comment

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

I don't think our linter catches it over here, but on the admin panel we enforce smart quotes I believe.

The new read-only mode is only accessible when running `tailscale web`
by passing a new `-readonly` flag. This new mode is identical to the
existing login mode with two exceptions:

 - the management client in tailscaled is not started (though if it is
   already running, it is left alone)

 - the client does not prompt the user to login or switch to the
   management client. Instead, a message is shown instructing the user
   to use other means to manage the device.

Updates #10979

Signed-off-by: Will Norris <will@tailscale.com>
@willnorris willnorris merged commit 128c99d into main Feb 8, 2024
47 checks passed
@willnorris willnorris deleted the will/webui-readonly branch February 8, 2024 18:11
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.

None yet

3 participants