Add context to "tsh ls" in docs #12583

ptgott · 2022-05-11T19:05:36Z

Create a partial for how the Teleport Auth Service filters Nodes
based on user roles/logins in response to queries.
Add the partial to provide context for example commands that include
"tsh ls".
Make our existing text on Teleport's authorization checks clearer
by enumerating the checks in the order they are executed in
services.RoleSet.CheckAccess.

Note that this does not change guides that instruct the user to create
a new user and role, since a user following these guides will see the
correct "tsh ls" output.

r0mant · 2022-05-11T22:40:13Z

docs/pages/includes/node-logins.mdx

@@ -0,0 +1,30 @@
+When Teleport's Auth Service receives a request to list Teleport Nodes (e.g., to
+display Nodes in the Web UI or via `tsh ls`), it only returns the Nodes that the
+current user is authorized to access.


Nit: I would say "see" (or analogue) instead of "access" because you can see the node but still have not permissions to access it if allowed logins don't allow it.

Suggested change

current user is authorized to access.

current user is authorized to see.

r0mant · 2022-05-11T22:41:05Z

docs/pages/includes/node-logins.mdx

+
+{/*
+
+ TODO: We might want to mention that the Auth Service checks the resource's


Namespaces are basically not used at all in Teleport. All resources always have default namespace and I don't think this will ever change TBH. So I think this "todo" item won't ever get resolved so we may as well just remove it. But it's up to you if you want to keep it.

Thanks—I'll delete this TODO. I think #12580 can be resolved by deleting all references to Teleport namespaces in the docs (looks like there are a couple of --namespace mentions in the CLI reference, but I don't think anything beyond that)

r0mant · 2022-05-11T22:41:44Z

docs/pages/includes/node-logins.mdx

+ TODO: We might want to mention that the Auth Service checks the resource's
+ namespace as well, but we currently do not document resource namespaces.
+ This would not be the appropriate place to include our only namespace
+ documentation. See gravitational/teleport issue #12580.


If you still intend to keep this "todo" item, just paste a link to the issue.

r0mant · 2022-05-11T22:43:17Z

docs/pages/includes/node-logins.mdx

+
+*/}
+
+- None of the user's roles contains a `deny` rule that matches the Node's labels.


English grammar question: is "contains" the correct form here, or should it say "contain"? :) I'd think that "none of the roles contain" is more correct cause it's plural but I may be mistaken.

From what I've read, "none" can be singular or plural, and there's no good rule for this. It probably would sound better as "contain," though.

r0mant · 2022-05-11T22:55:38Z

docs/pages/includes/node-logins.mdx

+*/}
+
+- None of the user's roles contains a `deny` rule that matches the Node's labels.
+- None of the user's roles contains a `deny` rule that matches the user's


I may be misunderstanding what this is referring to, but I'm not sure this applies to listing nodes.

I was attempting to paraphrase RoleSet.CheckAccess, which looks like it would check if one of the matchers, which I thought would be a services.loginMatcher in the case of a Node, matches a deny rule.

r0mant · 2022-05-11T22:56:51Z

docs/pages/includes/node-logins.mdx

+  `traits.logins`.
+- At least one of the user's roles contains an `allow` rule that matches the
+  Node's labels.
+- At least one of the user's roles contains an `allow` rule that matches the


I think mentioning traits makes it a bit more confusing. I would just say that at least one of user's roles has allowed logins for the node. Traits aren't really relevant in this context I think.

Fixes #7051 - Create a partial for how the Teleport Auth Service filters Nodes based on user roles/logins in response to queries. - Add the partial to provide context for example commands that include "tsh ls". - Make our existing text on Teleport's authorization checks clearer by enumerating the checks in the order they are executed in services.RoleSet.CheckAccess. Note that this does not change guides that instruct the user to create a new user and role, since a user following these guides will see the correct "tsh ls" output.

github-actions · 2022-06-23T17:23:48Z

@ptgott See the table below for backport results.

Branch	Result
branch/v10	Create PR
branch/v8	Failed
branch/v9	Failed

Backports #12583 * Add context to "tsh ls" in docs Fixes #7051 - Create a partial for how the Teleport Auth Service filters Nodes based on user roles/logins in response to queries. - Add the partial to provide context for example commands that include "tsh ls". - Make our existing text on Teleport's authorization checks clearer by enumerating the checks in the order they are executed in services.RoleSet.CheckAccess. Note that this does not change guides that instruct the user to create a new user and role, since a user following these guides will see the correct "tsh ls" output. * Respond to PR feedback

Add context to "tsh ls" in docs Backports #12583 * Add context to "tsh ls" in docs Fixes #7051 - Create a partial for how the Teleport Auth Service filters Nodes based on user roles/logins in response to queries. - Add the partial to provide context for example commands that include "tsh ls". - Make our existing text on Teleport's authorization checks clearer by enumerating the checks in the order they are executed in services.RoleSet.CheckAccess. Note that this does not change guides that instruct the user to create a new user and role, since a user following these guides will see the correct "tsh ls" output. * Respond to PR feedback

Backports #12583 * Add context to "tsh ls" in docs Fixes #7051 - Create a partial for how the Teleport Auth Service filters Nodes based on user roles/logins in response to queries. - Add the partial to provide context for example commands that include "tsh ls". - Make our existing text on Teleport's authorization checks clearer by enumerating the checks in the order they are executed in services.RoleSet.CheckAccess. Note that this does not change guides that instruct the user to create a new user and role, since a user following these guides will see the correct "tsh ls" output. * Respond to PR feedback

Add context to "tsh ls" in docs Backports #12583 * Add context to "tsh ls" in docs Fixes #7051 - Create a partial for how the Teleport Auth Service filters Nodes based on user roles/logins in response to queries. - Add the partial to provide context for example commands that include "tsh ls". - Make our existing text on Teleport's authorization checks clearer by enumerating the checks in the order they are executed in services.RoleSet.CheckAccess. Note that this does not change guides that instruct the user to create a new user and role, since a user following these guides will see the correct "tsh ls" output. * Respond to PR feedback

github-actions bot requested review from r0mant, xinding33 and zmb3 May 11, 2022 19:05

github-actions bot added the documentation label May 11, 2022

r0mant reviewed May 11, 2022

View reviewed changes

ptgott requested a review from r0mant May 20, 2022 18:24

r0mant approved these changes May 24, 2022

View reviewed changes

ptgott force-pushed the paul.gottschling/7051-node-logins branch from c228584 to 3315501 Compare May 24, 2022 21:01

ptgott enabled auto-merge (squash) May 24, 2022 21:01

ptgott disabled auto-merge May 24, 2022 21:14

ptgott force-pushed the paul.gottschling/7051-node-logins branch 3 times, most recently from ae36e4a to d83f7f9 Compare June 8, 2022 18:36

zmb3 approved these changes Jun 14, 2022

View reviewed changes

ptgott force-pushed the paul.gottschling/7051-node-logins branch from d83f7f9 to 9dc4dee Compare June 15, 2022 20:30

ptgott added backport/branch/v9 labels Jun 17, 2022

ptgott mentioned this pull request Jun 21, 2022

Reconfigure doc-tests CI build to use new docs image #13457

Closed

ptgott force-pushed the paul.gottschling/7051-node-logins branch from 9dc4dee to 59cfa81 Compare June 23, 2022 16:25

ptgott enabled auto-merge (squash) June 23, 2022 16:25

ptgott added 2 commits June 23, 2022 13:16

Respond to PR feedback

a7fb9d1

ptgott force-pushed the paul.gottschling/7051-node-logins branch from 59cfa81 to a7fb9d1 Compare June 23, 2022 17:16

ptgott merged commit 8c2ced7 into master Jun 23, 2022

ptgott mentioned this pull request Jun 23, 2022

[v10] Add context to "tsh ls" in docs #13816

Merged

ptgott mentioned this pull request Jun 23, 2022

[v8] Add context to "tsh ls" in docs #13817

Merged

ptgott mentioned this pull request Jun 23, 2022

[v9] Add context to "tsh ls" in docs #13818

Merged

ptgott deleted the paul.gottschling/7051-node-logins branch February 16, 2023 19:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add context to "tsh ls" in docs #12583

Add context to "tsh ls" in docs #12583

ptgott commented May 11, 2022

r0mant May 11, 2022

r0mant May 11, 2022

ptgott May 20, 2022

r0mant May 11, 2022

r0mant May 11, 2022

ptgott May 20, 2022

r0mant May 11, 2022

ptgott May 20, 2022

r0mant May 11, 2022

github-actions bot commented Jun 23, 2022

	current user is authorized to access.
	current user is authorized to see.


		{/*

		TODO: We might want to mention that the Auth Service checks the resource's


		*/}

		- None of the user's roles contains a `deny` rule that matches the Node's labels.

Add context to "tsh ls" in docs #12583

Add context to "tsh ls" in docs #12583

Conversation

ptgott commented May 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 23, 2022