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.

Sign up for GitHub

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

feat: disable buttons to try sample data or upload a file for a read-only user #197991

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

feat: disable buttons to try sample data or upload a file for a read-only user #197991

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ea7734e
feat: disable buttons to try sample data or upload a file for a read-…
paulinashakirova Oct 28, 2024
c5cad45
switch to using application.capabilities.roles.save
paulinashakirova Oct 28, 2024
d60626e
replace to use savedObjectsManagement
paulinashakirova Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
144 changes: 144 additions & 0 deletions src/plugins/home/public/application/components/add_data/__snapshots__/add_data.test.tsx.snap
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

23 changes: 22 additions & 1 deletion src/plugins/home/public/application/components/add_data/add_data.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,17 @@ beforeEach(() => {
});

const applicationStartMock = {
capabilities: { navLinks: { integrations: true } },
capabilities: {
navLinks: { integrations: true },
rulesSettings: { writeFlappingSettingsUI: true },
},
} as unknown as ApplicationStart;

const applicationStartMockRestricted = {
capabilities: {
navLinks: { integrations: true },
rulesSettings: { writeFlappingSettingsUI: false },
},
} as unknown as ApplicationStart;

const addBasePathMock = jest.fn((path: string) => (path ? path : 'path'));
Expand All @@ -46,4 +56,15 @@ describe('AddData', () => {
);
expect(component).toMatchSnapshot();
});
test('disable sample data and upload buttons for a read-only user', () => {
const component = shallowWithIntl(
<AddData
addBasePath={addBasePathMock}
application={applicationStartMockRestricted}
isDarkMode={false}
isCloudEnabled={false}
/>
);
expect(component).toMatchSnapshot();
});
});
4 changes: 4 additions & 0 deletions src/plugins/home/public/application/components/add_data/add_data.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ interface Props {
export const AddData: FC<Props> = ({ addBasePath, application, isDarkMode, isCloudEnabled }) => {
const { trackUiMetric, guidedOnboardingService } = getServices();
const canAccessIntegrations = application.capabilities.navLinks.integrations;
const onlyReadAccess = !application.capabilities.rulesSettings.writeFlappingSettingsUI;
Copy link
Contributor

@Dosant Dosant Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't have a lot of experience with capabilities, but I don't think this is a good flag for this readonly state. rulesSettings.writeFlappingSettingsUI seem to be a alerting framework
specific capability and we shouldn't use it outside of it.

I think this is the right direction, but I don't know if there is an existing capability that could work for us here (need to look for it) otherwise we might have to register a new one

Copy link
Contributor

@Dosant Dosant Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For the the sampel data, I think application.capabilities.indexPatterns.save would be a good capability. To create sample data you'd have to be able to create a (dataview) so it would at least be a required condition.

for the fileDataViz not sure. haven't worked with it before - need review the code an see if there anything we could use

Copy link
Contributor

@Dosant Dosant Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Also the screenshot from the issue #138547 is pointing out an issue on different component

Copy link
Contributor Author

@paulinashakirova paulinashakirova Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In the comment below the body of the issue there was this suggestion
#138547 (comment), which made sense, so I followed it.
Do you think it is less beneficial to disable the whole accordion?

Copy link
Contributor

@Dosant Dosant Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I was thinking a bit more about the issue and did some testing. I think the issue is not that simple and we will need to find some middleground between the effort and the result. Some investigation is needed, here are some pointers:

  • In general, just disabling the buttons is not a good UX. It either need to have a reason (for example in a tooltip) explaining why it is disable or simply hidden completely.
  • Hidding or disabling the buttons in add_data component is not enough, because there are other ways to navigate to app/home#/tutorial_directory/sampleData page . Also maybe we shouldn't disable those buttons in add_data component at all , because there is an external link from that page to a demo environment that isn't restricted by permissions
  • application.capabilities.savedObjectsManagement.edit is a good guess for checking if the user is able to add sample data, but it might give false positives and negatives. Ideally we should investigate exact permissions that are needed for adding sample data and then decide what to do with them. This can be done by reviewing the code, analysing internal errors and trial and error.
  • I am not sure if it is correct to treat sample data and import together. They likely need different inner permissions. I was exploring the upload functionality and found that there is a more specific check inside

    kibana/x-pack/plugins/data_visualizer/public/application/file_data_visualizer/components/file_data_visualizer_view/file_data_visualizer_view.js

    Lines 72 to 75 in 3177b03

    const hasPermissionToImport = await this.props.fileUpload.hasImportPermission({
    checkCreateDataView: false,
    checkHasManagePipeline: true,
    });
    . It might be that reviewing it might help to better understand how permissions work and what how to better fix it.
  • Another problem is that when you don't have enough permissions and try to add sample data we show "Internal server error" If we'd start from showing the proper "Permissions" error that we get from elasticsearch, that would be better for a user and for us to debug and improve on the issue

It might be that after the investigation we will return to a simple fix like we currently have. But to do it with confidence we'd need to understand how it all works end to end

Copy link
Contributor Author

@paulinashakirova paulinashakirova Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for your detailed review.
I will convert this PR to draft for now, since it doesn't fall into the "papercut" category.

Might reopen in the future.


if (canAccessIntegrations) {
return (
<KibanaPageTemplate.Section
Expand Down Expand Up @@ -122,6 +124,7 @@ export const AddData: FC<Props> = ({ addBasePath, application, isDarkMode, isClo
data-test-subj="addSampleData"
href={addBasePath('#/tutorial_directory/sampleData')}
iconType="documents"
disabled={onlyReadAccess}
>
<FormattedMessage
id="home.addData.sampleDataButtonLabel"
Expand All @@ -135,6 +138,7 @@ export const AddData: FC<Props> = ({ addBasePath, application, isDarkMode, isClo
data-test-subj="uploadFile"
href={addBasePath('#/tutorial_directory/fileDataViz')}
iconType="importAction"
disabled={onlyReadAccess}
>
<FormattedMessage
id="home.addData.uploadFileButtonLabel"
Expand Down