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

[Fleet]: Spaces which are not accessible is displayed a "?" under Agent policy settings for custom user having access only for Space2. #193827

Open
amolnater-qasource opened this issue Sep 24, 2024 · 13 comments · May be fixed by #201251
Assignees
Labels
bug Fixes for quality problems that affect the customer experience impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Team:Fleet Team label for Observability Data Collection Fleet team

Comments

@amolnater-qasource
Copy link

Kibana Build details:

VERSION: 8.16.0 SNAPSHOT
BUILD: 78483
COMMIT: 6a9663fa3a874877f0ca034970e72af8ab70be66

Role:
space: SPACE2

Integrations: All
Fleet: Read
Agents: Read
Agent policies: All
Settings: All

Image
Image

Preconditions:

  1. 8.16.0-SNAPSHOT Kibana cloud environment should be available.
  2. New User should be created with above defined role.
  3. An agent policy should be provided access to all the spaces.

Steps to reproduce:

  1. Login with the above user to the second space.
  2. Now navigate to Fleet>Agent policy settings.
  3. Observe the spaces that are not accessible to this user are displayed as ?.
    Image

Expected Result:
Spaces which are not accessible should not be displayed even as ? under Agent policy settings for custom user having access only for Space2.

Screen Recording:

Linux.Agent.policy.3.-.Agent.policies.-.Fleet.-.Elastic.-.Google.Chrome.2024-09-24.13-34-25.mp4

Feature:
https://github.com/elastic/ingest-dev/issues/2903
https://github.com/elastic/ingest-dev/issues/1664

@amolnater-qasource amolnater-qasource added bug Fixes for quality problems that affect the customer experience impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Team:Fleet Team label for Observability Data Collection Fleet team labels Sep 24, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@amolnater-qasource
Copy link
Author

@muskangulati-qasource Please review.

@muskangulati-qasource
Copy link

Secondary review on this ticket is Done

@jillguyonnet jillguyonnet self-assigned this Nov 19, 2024
@jillguyonnet
Copy link
Contributor

Hey @nchaulet can I check the expectations with you on this one?

The reason why the space selector is showing ? for unavailable spaces directly comes from the saved objects API. For example, assuming a user that only has access to space1 and an agent policy that has default, space1 and space2, then the saved object returned for this policy will have "namespaces": ["space1", "?", "?"]. Interestingly, though, the document retrieved by directly querying the .kibana_ingest index from space1 does have the space "namespaces": ["default", "space1", "space2"], so maybe there could be a way to retrieve all ids with some backend change. I'm not sure, however, if that would be the best approach.

I'm wondering if it wouldn't make more sense to only display spaces available to the user. We could consider adding some information for the user, e.g. an additional sentence in the Spaces input description and/or how many spaces the policy is assigned to that the user hasn't got access to.

One thing to note is that the space selector options are fetched independently and don't appear in the dropdown if the user doesn't have access to them. For instance, in the above example, if the policy only had default and space1, then the user in space1 would see space1 and ? selected and You've selected all available options in the dropdown. So only showing the selected spaces the user has access to would seem consistent with that.

Alternatively, we could still show all the selected spaces and replace the ? label something more meaningful and/or add a tooltip. The downside of that is these will still appear to be editable, even though the policy won't save if the user tries to e.g. remove a space they don't have access to.

@nchaulet
Copy link
Member

nchaulet commented Nov 19, 2024

@jillguyonnet I think we probably could display a label instead of ? something like:

spaces: ["space1", "Space you cannot access", "Space you cannot access"]

We should probably also disable that field as the user will not have enough permissions to make space changes and remove it from the API call has otherwise it seems it will not be handled correctly when saving
Image

@jillguyonnet
Copy link
Contributor

Thanks for your feedback @nchaulet - just thinking, could we be in a situation where the user is allowed to add/remove some spaces but not others? For example:

  • assuming spaces default, space1, space2
  • with a policy assigned to all three spaces
  • a user who has full access to space1 and space2

Could this user be allowed to add/remove space1 and space2 but not default? In that case the input would need to remain enabled and it would look like: space1, space2, Space you cannot access. They could try to remove the default space, but there will be an error when trying to save the changes.

Otherwise, if they can't make any space changes, it would indeed make sense to disable the input altogether and remove the Create space link.

I can test the above theory tomorrow.

@kpollich
Copy link
Member

Could we filter out spaces that the current user can't access entirely rather than displaying a badge? I think it'd be technically considered information leakage here and the badge itself it not informative in anyway. If there are no accessible spaces, disable the input entirely as @jillguyonnet has suggested.

@nchaulet
Copy link
Member

I think if the user do not have access to all spaces, he should not able to make any spaces change to that policy, so disabling that selector seems the right thing to do.

@jillguyonnet
Copy link
Contributor

Could we filter out spaces that the current user can't access entirely rather than displaying a badge?

My initial suggestion was to hide these, as this is not information relevant to the user.

I also thought that showing them would give the false impression that the user can edit them, which is not optimal UX. If we disable the space selector for users that don't have access to all spaces like Nicolas suggested, however, this is not an issue.

I think if the user do not have access to all spaces, he should not able to make any spaces change to that policy, so disabling that selector seems the right thing to do.

I've been doing a bit of testing, details below. The space selector works well for a user with access to all selected spaces, but it is broken otherwise, even if the user tries to add/remove a space they have access to. This is something that may be surprising to the user. For this reason (assuming this behaviour is correct), then it might be better to have some indication of why they can't make edits. One option would be showing the badges of the unavailable spaces (with a suitable label). An alternative would be to hide these badges and add an explanation in the input description.

Summary

Assuming that the user should NOT be able to edit the policy spaces if they don't have access to all selected spaces.

If the user doesn't have access to all selected spaces:

  • Disable input
  • Hide the Create space link

In addition, one of the following:

  1. Hide badges for spaces they don't have access to and add an indication (e.g. a tooltip with message You need access to all selected spaces to make edits.).
  2. Keep these badges and replace label with e.g. Unavailable space.

@jillguyonnet
Copy link
Contributor

Some screenshots to illustrate my previous comment:

  • Spaces: Default, Bar, Foo, Quux
  • Agent policy with all of these assigned

The default admin user can see these:
Image

I created a user with full access to Default and Foo:
Image

This user sees:
Image

And they cannot make edits (here, they tried to remove the Foo space and it resulted in a bad state):
Image

Similarly, they couldn't add another space they have access to. If this is expected, then it would make sense to disable the selector as described above.

@jillguyonnet
Copy link
Contributor

Another observation I found while investigating: as I mentioned previously, the ? space ids come from the saved objects API, more precisely from soClient.get (used in agentPolicy.get):

const agentPolicySO = await soClient.get<AgentPolicySOAttributes>(savedObjectType, id);

However, soClient.find (used in agentPolicy.list) returns all the space ids:

agentPoliciesSO = await soClient.find<AgentPolicySOAttributes>({
...baseFindParams,
filter,
});

This means there is a difference between GET fleet/agent_policies and GET fleet/agent_policies/<policyId>: the former will return all the space ids, regardless of user access to these spaces.

In the example above, if the user with access to Default and Foo runs these queries they will see:

  • GET fleet/agent_policies
    {
    ...
          "space_ids": [
            "default",
            "bar",
            "quux",
            "foo"
          ],
    ...
    }
    
  • GET fleet/agent_policies/<policyId>
    {
    ...
          "space_ids": [
            "default",
            "foo",
            "?",
            "?"
          ],
    ...
    }
    

This seems like this could be an information leakage concern, WDYT?

@jillguyonnet
Copy link
Contributor

Hey @kpollich @nchaulet I've opened #201251 with the second option (disable input and keep the badges), let me know your feedback. It's an easy change to hide the badges instead.

@jillguyonnet
Copy link
Contributor

Created #201400 to capture #193827 (comment).

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 impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Team:Fleet Team label for Observability Data Collection Fleet team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants