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

[DOCS] Add docs for Application Privileges #32635

Merged
merged 19 commits into from
Aug 24, 2018
Merged

Conversation

tvernum
Copy link
Contributor

@tvernum tvernum commented Aug 6, 2018

  • Adds docs for new _xpack/security/privileges/ APIs
  • Update docs for Has Privileges API to include "application"
    privilege checking
  • Update docs for defining roles (API + guide) to include
    "application" privileges, and "global" privileges

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-docs

@lcawl
Copy link
Contributor

lcawl commented Aug 9, 2018

The has privileges API page links to https://www.elastic.co/guide/en/elastic-stack-overview/master/security-privileges.html . Should application privileges be referenced there as well?

@@ -57,6 +65,13 @@ GET _xpack/security/user/_has_privileges
"names": [ "inventory" ],
"privileges" : [ "read", "write" ]
}
],
"application": [
Copy link
Contributor

Choose a reason for hiding this comment

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

The intro for this example should be changed to say "...checks whether the current user has a specific set of cluster, index, and application privileges"

Copy link
Contributor

Choose a reason for hiding this comment

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

Done!

specified resources. May be either application privilege names, or the names of
actions that are granted by those privileges
`resources`::: (list) A list of resource names against which the privileges
should be checked
Copy link
Contributor

Choose a reason for hiding this comment

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

This description and example aren't enough to explain application privileges, in my opinion. We should add a link to a page that explains the concept in more detail.

{xpack-ref}/defining-roles.html#roles-application-priv[Application Privileges].

==== Request

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we separate these into separate pages for the various APIs (similar to the separate pages for creating and deleting ML objects e.g. https://www.elastic.co/guide/en/elasticsearch/reference/master/ml-apis.html)? If so, I can do that split.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that would be great. I followed the example from other security APIs and didn't realise we had a precedent for separate pages.


==== Description

Application privileges allow Elasticsearch to act as an _oracle_ for security
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this use of the term "oracle" well known? It didn't seem obvious when I search on that term.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I don't think it's well enough known, but unfortunateIy couldn't find a better word. Any suggestions?
Probably I just need to restructure this sentence to not need a noun to describe ES's role here.

Copy link
Contributor

Choose a reason for hiding this comment

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

may be ... to act as the source of truth for security decisions that ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I hate that source of truth phrase (probably a reaction to working on too many "single view of customer" projects). You'd need to offer a lot of 🍻 to convince me to put it in this doc 😈

@lcawl
Copy link
Contributor

lcawl commented Aug 9, 2018

It seems to me that we should describe the concept of application privileges in a high-level overview page (e.g. add them to https://www.elastic.co/guide/en/elastic-stack-overview/master/authorization.html).

@tvernum
Copy link
Contributor Author

tvernum commented Aug 10, 2018

It seems to me that we should describe the concept of application privileges in a high-level overview

Yeah, I started by describing the APIs and then expanded from there, and I think it led to the wrong place.
We do need to document application privileges more clearly if only because Kibana will start adding them to roles, and administrators will see them in responses and want to know what they mean.

@tvernum
Copy link
Contributor Author

tvernum commented Aug 10, 2018

@lcawl Looking at this more closely, I don't think the Overview page makes sense, because it currently only documents very high level concepts (e.g. it doesn't even explain the difference between cluster and index privileges).

My propsal would be to add something to privileges.asciidoc (<<security-privileges>>). That doc is linked from the overview page.

I'll start by writing something as a new section in privileges.asciidoc, and if it seems to be the wrong level of detail for that page we can move it to be a standalone doc and link it in.

@lcawl
Copy link
Contributor

lcawl commented Aug 16, 2018

Re: #32635 (comment), I've created elastic/stack-docs#104 to add that information in the Stack Overview.

Copy link
Contributor

@lcawl lcawl left a comment

Choose a reason for hiding this comment

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

LGTM, though might be affected by #32879

@lcawl lcawl force-pushed the docs/app-privs branch 2 times, most recently from df236b3 to bb285a0 Compare August 18, 2018 08:03

==== Examples

The following example deletes the `write` application privilege from the
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: write -> read

Copy link
Contributor

@albertzaharovits albertzaharovits left a comment

Choose a reason for hiding this comment

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

LGTM Tim. I see it's rather scant, but since these APIs and data structures are for inner use - inside the stack - it's better this way; No need to get too abstract here, as this is docs for programmers 🤓
If I were to add smth I would emphasize the has_privilege API as this is really the crux of it; without it we only define entities and APIs to manage them. Specifically, there might be some examples.
One last thing I would like to know, from a programmer point of view, is if there are any referential integrity checks between the privilege name in the role descriptor and the defined privileges. Do I need to define all privileges before a role descriptor can use them? What happens If I delete a privilege that is referred to in a role? Just drop a line on the topic, and it will suit me :)

To check a user's application privileges, use the
<<security-api-has-privileges,has privileges API>>.

////
Copy link
Member

Choose a reason for hiding this comment

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

Let's just delete this commented out part now that the support for this has been removed

included as path parameters, the body of the request is a JSON object that
includes the following fields:

`application`:: (string) The name of the application to which this privilege
Copy link
Member

Choose a reason for hiding this comment

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

technically this is accepted but is optional. I suggest we remove it.

belongs. This field is optional, but if it exists it must match the application
name in the path parameter.

`name`:: (string) The name of the privilege to store. This field is optional,
Copy link
Member

Choose a reason for hiding this comment

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

technically this is still accepted but is optional. I suggest we remove it.

{
"myapp": {
"read": {
"application": "myapp",
Copy link
Member

Choose a reason for hiding this comment

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

if we do remove the field from above, I suggest we also remove this line

"myapp": {
"read": {
"application": "myapp",
"name": "read",
Copy link
Member

Choose a reason for hiding this comment

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

if we do remove the field from above, I suggest we also remove this line

PUT /_xpack/security/privilege
{
"myapp": {
"read": {
Copy link
Member

Choose a reason for hiding this comment

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

nit: seems like this could be formatted better in regards to indentation

@lcawl lcawl added v6.4.1 and removed v6.4.0 labels Aug 23, 2018
@lcawl
Copy link
Contributor

lcawl commented Aug 23, 2018

I rebased and ran gradle successfully locally, so I think they're good to go.

@lcawl
Copy link
Contributor

lcawl commented Aug 23, 2018

NOTE: I also updated the page titles of all security APIs that both create and update something (e.g. from "create user API" to "create or update user API"). This type of title matches what they do in Kibana and I think it's clearer: https://www.elastic.co/guide/en/kibana/current/role-management-api-put.html

@lcawl lcawl merged commit a211d24 into elastic:master Aug 24, 2018
martijnvg added a commit that referenced this pull request Aug 24, 2018
* es/master: (62 commits)
  [DOCS] Add docs for Application Privileges (#32635)
  Add versions 5.6.12 and 6.4.1
  Do NOT allow termvectors on nested fields (#32728)
  [Rollup] Return empty response when aggs are missing (#32796)
  [TEST] Add some ACL yaml tests for Rollup (#33035)
  Move non duplicated actions back into xpack core (#32952)
  Test fix - GraphExploreResponseTests should not randomise array elements Closes #33086
  Use `addIfAbsent` instead of checking if an element is contained
  TESTS: Fix Random Fail in MockTcpTransportTests (#33061)
  HLRC: Fix Compile Error From Missing Throws (#33083)
  [DOCS] Remove reload password from docs cf. #32889
  HLRC: Add ML Get Buckets API (#33056)
  Watcher: Improve error messages for CronEvalTool (#32800)
  Search: Support of wildcard on docvalue_fields (#32980)
  Change query field expansion (#33020)
  INGEST: Cleanup Redundant Put Method (#33034)
  SQL: skip uppercasing/lowercasing function tests for AZ locales as well (#32910)
  Fix the default pom file name (#33063)
  Switch ml basic tests to new style Requests (#32483)
  Switch some watcher tests to new style Requests (#33044)
  ...
martijnvg added a commit that referenced this pull request Aug 24, 2018
* es/6.x: (58 commits)
  [DOCS] Add docs for Application Privileges (#32635)
  Add versions 5.6.12 and 6.4.1
  [Rollup] Return empty response when aggs are missing (#32796)
  [TEST] Add some ACL yaml tests for Rollup (#33035)
  Test fix - GraphExploreResponseTests should not randomise array elements Closes #33086
  Use `addIfAbsent` instead of checking if an element is contained
  HLRC: Fix Compile Error From Missing Throws (#33083)
  [DOCS] Remove reload password from docs cf. #32889
  Use a dedicated ConnectionManger for RemoteClusterConnection (#32988)
  Watcher: Improve error messages for CronEvalTool (#32800)
  HLRC: Add ML Get Buckets API (#33056)
  Change query field expansion (#33020)
  Search: Support of wildcard on docvalue_fields (#32980)
  Add beta label to MSI on install Elasticsearch page (#28126)
  SQL: skip uppercasing/lowercasing function tests for AZ locales as well (#32910)
  [DOCS] Drafts Elasticsearch 6.4.0 release notes (#33039)
  Fix the default pom file name (#33063)
  Fix backport of switch ml basic tests to new style Requests (#32483)
  Switch ml basic tests to new style Requests (#32483)
  Switch some watcher tests to new style Requests (#33044)
  ...
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Aug 24, 2018
* ccr: (71 commits)
  Make CCR QA tests build again (elastic#33113)
  Add hook to skip asserting x-content equivalence (elastic#33114)
  Muted testListenersThrowingExceptionsDoNotCauseOtherListenersToBeSkipped
  [Rollup] Move getMetadata() methods out of rollup config objects (elastic#32579)
  fixed not returning response instance
  Muted testEmptyAuthorizedIndicesSearchForAllDisallowNoIndices
  Update Google Cloud Storage Library for Java (elastic#32940)
  Remove unsupported Version.V_5_* (elastic#32937)
  Required changes after merging in master branch.
  [DOCS] Add docs for Application Privileges (elastic#32635)
  Add versions 5.6.12 and 6.4.1
  Do NOT allow termvectors on nested fields (elastic#32728)
  [Rollup] Return empty response when aggs are missing (elastic#32796)
  [TEST] Add some ACL yaml tests for Rollup (elastic#33035)
  Move non duplicated actions back into xpack core (elastic#32952)
  Test fix - GraphExploreResponseTests should not randomise array elements Closes elastic#33086
  Use `addIfAbsent` instead of checking if an element is contained
  TESTS: Fix Random Fail in MockTcpTransportTests (elastic#33061)
  HLRC: Fix Compile Error From Missing Throws (elastic#33083)
  [DOCS] Remove reload password from docs cf. elastic#32889
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>docs General docs changes :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC v6.4.1 v6.5.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants