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

[hasDataService] Permissions issue #132478

Closed
majagrubic opened this issue May 18, 2022 · 17 comments
Closed

[hasDataService] Permissions issue #132478

majagrubic opened this issue May 18, 2022 · 17 comments
Assignees
Labels
bug Fixes for quality problems that affect the customer experience Feature:Data Views Data Views code and UI - index patterns before 8.0 impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:needs-research This issue requires some research before it can be worked on or estimated Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL.

Comments

@majagrubic
Copy link
Contributor

majagrubic commented May 18, 2022

Describe the bug:
When testing the new index hasData service, I have found two 3 permission issues. Two refer to when a user has no read index privileges.

Scenario one:

  1. There are 3 indices
  2. User has read access to 1 of the indices, but not to the other two
  3. The service will return false and we will show a no data screen, thus blocking access to Discover. The user should be able to see the index they have access to.

Scenario two:

  1. There are 3 indices
  2. User has no read permissions to any indices
  3. The service will return false and will show a no data screen. This is not as severe as a the first part (as from the user's standpoint there really is no data they could see), but it would be nice if the service would throw some sort of an error that we could leverage then to display an appropriate message.

The third issue refers to a scenario where the user has read access to the index, but no view_index_metadata privileges. In that case the user has no permissions to view if they have permissions 🤯 I am not sure what the expected scenario here is but apparently this is a valid use-case. It feels like we shouldn't be blocking users from the access to the index they have access to.

Can we get the fix for this prioritized for 8.3.0? cc @shivindera

@majagrubic majagrubic added the bug Fixes for quality problems that affect the customer experience label May 18, 2022
@botelastic botelastic bot added the needs-team Issues missing a team label label May 18, 2022
@majagrubic majagrubic added Team:AppServicesSv and removed needs-team Issues missing a team label labels May 18, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServicesSv)

@mattkime
Copy link
Contributor

mattkime commented May 18, 2022

Its frustrating that the resolve index api uses the privledges that it does (requiring view_index_metadata). This is something we could take up with ES - which would likely be slow. Alternatively, we can use a search query to return indices. Once upon a time this is how we determined what indices existed - https://github.com/elastic/kibana/blob/7.17/src/plugins/index_pattern_editor/public/lib/get_indices.ts#L81

I'd like to leave this in @shivindera 's hands - let me know if you need anything else to resolve this.

@shivindera
Copy link
Contributor

Its frustrating that the resolve index api uses the privledges that it does (requiring view_index_metadata). This is something we could take up with ES - which would likely be slow. Alternatively, we can use a search query to return indices. Once upon a time this is how we determined what indices existed - https://github.com/elastic/kibana/blob/7.17/src/plugins/index_pattern_editor/public/lib/get_indices.ts#L81

I'd like to leave this in @shivindera 's hands - let me know if you need anything else to resolve this.

I will add that check to hasESData service. Also, ES does not return any permission error or anything, it does a 500 (Internal Server Error)

@Dosant
Copy link
Contributor

Dosant commented May 19, 2022

Also, ES does not return any permission error or anything, it does a 500 (Internal Server Error)

Looks like es does return permission error.
500 is coming from kibana server because we don't handle errors in resolveIndex call.

This is how the error from es looks like in case there are no indices with view_index_metadata:

{
  "error": {
    "root_cause": [
      {
        "type": "security_exception",
        "reason": "action [indices:admin/resolve/index] is unauthorized for user [singleindex] with roles [single-index], this action is granted by the index privileges [view_index_metadata,manage,all]"
      }
    ],
    "type": "security_exception",
    "reason": "action [indices:admin/resolve/index] is unauthorized for user [singleindex] with roles [single-index], this action is granted by the index privileges [view_index_metadata,manage,all]"
  },
  "status": 403
}

It seems like for a quick fix, we simply could handle this 403 and return true on hasData. Basically assume the user has data in case we can't answer, for sure, for security reasons.
Also, in general, for any kind of error in hasData call, it seems like we should assume the user has data and treat empty state as a progressive enhancement.
Now it looks like we treat any error as no data because of:

For a better, but more complex fix, I think we could leave resolveIndices API as the primary check, but we could handle that 403 and then fallback to search or fields caps API. I think on this point it is worse moving that service logic server-side

@majagrubic
Copy link
Contributor Author

majagrubic commented May 19, 2022

  1. For the view metadata issue, can we use the solution recommended by es-security team? https://www.elastic.co/guide/en/elasticsearch/reference/current/search-field-caps.html#search-field-caps
    That way we're not risking running into an exception. I believe it's similar to how search would work

  2. I believe using that API would also solve the issue of "user has access to some indices but not all"? We should absolutely return true from hasEsData in that case, as Anton pointed out - it's better to assume there is data in that case and let a user create data views from indices they have access to.

  3. In all other cases, instead of returning true in case ES returns 403, how about the service throws a PermissionError or something similar? I think this is the cleanest way to handle this. That way the UI can catch the error and display appropriate error message.

@mattkime
Copy link
Contributor

My concern with using field caps - The list of indices where this field has the same type family, or null if all indices have the same type family for the field. - if I'm reading that correctly, it means that in some circumstances it might return an empty set even if there are indices.

After mulling this over a bit, aside from how we fix this now I think we should request a change to the permissions for the resolve api.

@majagrubic
Copy link
Contributor Author

majagrubic commented May 19, 2022

Since this is possibly turning into a larger architectural discussion and an issue we had been fighting with in the past, I'd like to put this on @stacey-gammon & @jportner 's radar and hopefully get their input on this as well.

@stacey-gammon
Copy link
Contributor

It seems riskier to switch to field caps, esp given @mattkime's concerns. For an immediate fix to resolve the blocker, I like @Dosant's suggestion of returning true instead of false when any error is encountered. I also like @majagrubic's suggestion about having the API throw a PermissionError, but as an immediate fix it may require more upstream work, so I'd hold off on going down that path for now.

@stacey-gammon
Copy link
Contributor

As for a longer-term solution, I think we should involve the ES team. Give them our requirements and see what they suggest for a solution!

@javanna
Copy link
Member

javanna commented May 19, 2022

I am happy to discuss your requirements and get more context on the hasDataService. I agree that using field_caps for lack of a better API that requires the right permissions is not a good solution, or at least that is the mental summary I made while reading this issue and the related slack conversation.

@majagrubic
Copy link
Contributor Author

majagrubic commented May 19, 2022

My concern with returning true when an error is encountered is - would it not return true even if the security exception is valid, ie. if the user has no read privileges to the index? In case that true is returned, the UI will trigger a subsequent hasUserDataView call - what happens then? How can we distinguish that a user has no privileges for a data view built on top of an index they don't have an access to, but not to an index itself? Apologies if this is a stupid question, but I want to be sure we don't run into a scenario where we grant access to data that user does not have an access to, which is a bit worse than the other way around.

@exalate-issue-sync exalate-issue-sync bot added the impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. label May 19, 2022
@shivindera
Copy link
Contributor

shivindera commented May 20, 2022

For now I have added a check to hit the search api if the call to resolveAPI fails. - #132618 .

@javanna Let us know when the permission thing is fixed to use read, then we can remove the fix.

@majagrubic
Copy link
Contributor Author

majagrubic commented May 20, 2022

With the latest change linked above I can confirm that the issues 1 and 2 are resolved.

Issue with view_index_metadata permissions is partially resolved: the hasEsData check will be successful, and the user will now be presented with “no data views” screen, which is good. However, when they attempt creating a data view, it will appear as they have no data again, thus unable to create a data view:
permissions

This was an issue in previous versions as well, but will now become more prominent, since we'll offer data view creation from 4 additional places. I would treat this still as high priority for 8.3.0, but given it's a bug, I believe it can be fixed after FF.

@rayafratkina
Copy link
Contributor

+1 I think we should try to fix this after FF. Thank you, all!

@exalate-issue-sync exalate-issue-sync bot added impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. and removed impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. labels Aug 12, 2022
@petrklapka petrklapka added Feature:Data Views Data Views code and UI - index patterns before 8.0 Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. and removed Team:AppServicesSv labels Nov 28, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@javanna
Copy link
Member

javanna commented Nov 30, 2022

I found this issue in my inbox due to recent notifications. I caught up on what it was about and noticed that the permission issue was fixed in Elasticsearch, see elastic/elasticsearch#87052 .

@davismcphee davismcphee added the loe:needs-research This issue requires some research before it can be worked on or estimated label Sep 8, 2023
@mattkime
Copy link
Contributor

Closing based on elastic/elasticsearch#87052 and the large number of changes that have happened to this service since this issue was created.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Data Views Data Views code and UI - index patterns before 8.0 impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:needs-research This issue requires some research before it can be worked on or estimated Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL.
Projects
None yet
Development

No branches or pull requests

10 participants