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

[APM] Fix indefinite loading state in agent settings for unauthorized user roles #44970

Merged

Conversation

@ogupte
Copy link
Contributor

commented Sep 6, 2019

Addresses #43326 (in conjunction with elastic/elasticsearch#46423)

  • displays message to user when a failure occurred in settings initialization
  • fixes incorrect settings link path
  • moves agent config index creation to plugin setup step

Screen Shot 2019-09-08 at 10 52 29 PM

@ogupte ogupte requested a review from elastic/apm-ui as a code owner Sep 6, 2019

@ogupte ogupte changed the title - handle unauthorized error to return empty list of agent settings [APM] Fix indefinite loading state in agent settings for unauthorized user roles Sep 6, 2019

@ogupte ogupte requested a review from bmorelli25 Sep 6, 2019

@ogupte ogupte added the v7.4.0 label Sep 6, 2019

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

@@ -7,7 +7,7 @@ import React from 'react';
import { APMLink, APMLinkExtendProps } from './APMLink';

const SettingsLink = (props: APMLinkExtendProps) => {
return <APMLink path="/" {...props} />;
return <APMLink path="/settings" {...props} />;

This comment has been minimized.

Copy link
@dgieselaar

dgieselaar Sep 6, 2019

Contributor

woops 😅 thanks for catching this.

{status === 'success' && !hasConfigurations ? (
emptyStatePrompt
) : (
{status === 'failure' ? (

This comment has been minimized.

Copy link
@dgieselaar

dgieselaar Sep 6, 2019

Contributor

Not a super big fan of nested ternaries in terms of readability, might be just me though.

This comment has been minimized.

Copy link
@sqren

sqren Sep 6, 2019

Member

I agree. I just didn't have any better suggestions :)

@@ -20,13 +20,19 @@ import { transactionSampleRateRt } from '../../common/runtime_types/transaction_
// get list of configurations
export const agentConfigurationRoute = createRoute(core => ({
path: '/api/apm/settings/agent-configuration',
handler: async req => {
await createApmAgentConfigurationIndex(core.http.server);
handler: async (req, _, h) => {

This comment has been minimized.

Copy link
@dgieselaar

dgieselaar Sep 6, 2019

Contributor

are we using h? can't see it being used below.

This comment has been minimized.

Copy link
@ogupte

ogupte Sep 9, 2019

Author Contributor

my mistake for leaving this in

const setup = await setupRequest(req);
return await listConfigurations({ setup });
} catch (error) {
if (error.statusCode === 403) {

This comment has been minimized.

Copy link
@dgieselaar

dgieselaar Sep 6, 2019

Contributor

If we catch 403 authorization errors, how does the user ever see failurePrompt? What other status codes should we expect here? just 500?

This comment has been minimized.

Copy link
@ogupte

ogupte Sep 9, 2019

Author Contributor

failurePrompt is now make more generic and the statusCode check was removed

'xpack.apm.settings.agentConf.configTable.failurePromptText',
{
defaultMessage:
'Whoops! The settings management failed to initialize. This might be because the user that is logged in does not have permission to read or write the APM agent configuration index. The next time a user with adequate privileges loads this screen, Kibana will complete this step.'

This comment has been minimized.

Copy link
@sqren

sqren Sep 6, 2019

Member

This text is still confusing. Our implementation details wrt creating the index lazily should not shine through to the end-user.

If we assume the apm_user has read/write access to the agent configuration index we have a couple of use cases:

  • An unknown backend error occurred (network glitch, database corruption, anything). We do not handle this in the rest of APM UI except for showing the automatic toast that the request failed. Feel free to explicitly handle the failure state. In that text the text needs to be something vague like "The list of agent configurations could not be fetched".
  • The user has neither read or write permission: they'd get the above error. That's better than most other places in APM UI.
  • The user has read permission but not write permission: the user should be able to see the list (and shouldn't see errors regardless if the index could not be created)
  • The user has read and write permissions: the user should be able to see the list
return await listConfigurations({ setup });
} catch (error) {
if (error.statusCode === 403) {
// return an empty list if user is not authorized

This comment has been minimized.

Copy link
@sqren

sqren Sep 6, 2019

Member

I think you should only do this for createApmAgentConfigurationIndex. I don't think we should swallow errors for listConfigurations this way.

This comment has been minimized.

Copy link
@sqren

sqren Sep 6, 2019

Member

This assumes that the apm_user as a minimum has read access to the index.

This comment has been minimized.

Copy link
@ogupte

ogupte Sep 9, 2019

Author Contributor

The latest commit removed all the assumptions and displays a message to the user that a failure occurred when loading settings.

Index creation uses callWithInternalUser during plugin setup, so any index creation failures should not occur when a user loads the settings screen. but it will error if the user does not have permission to read from the .apm-agent-configuration index. as per your list of use cases above.

When testing with a user with the role apm_user, i discovered that it does not have read permissions on this index. Since it's not appropriate to call callWithInternalUser for querying this index, it will show the failure message. The alternative is to give the apm_user role the permission to read the agent config index. I created a PR to update the role, however it looks like it will not get approval (elastic/elasticsearch#46423).

ogupte added 2 commits Sep 6, 2019
- handle unauthorized error to return empty list of agent settings
- display to user when a failure occurred in settings initialization
- fix incorrect settings link path
- moved agent config index creation to the plugin setup step
- make failure text reflect a general failure to load settings

@ogupte ogupte force-pushed the ogupte:apm-43326-handle-settings-init-failure branch from c369642 to 4b2303b Sep 9, 2019

) : (
emptyStatePrompt
)
) : null}

This comment has been minimized.

Copy link
@ogupte

ogupte Sep 9, 2019

Author Contributor

The previous status === 'success' && !hasConfigurations logic was rending emptyStatePrompt when status was loading so the new nested logic makes sure that nothing is rendered at that time. This is especially jarring if there's a failure message since it used to flash the emptyStatePrompt immediately prior to the failure message.

I'm open to alternatives for the nested ternary. :)

This comment has been minimized.

Copy link
@sqren

sqren Sep 9, 2019

Member

What about a new component:

function Foo(status) {
  if (status === 'failure') {
    return failurePrompt;
  }

  if (status === 'success') {
    if (!hasConfigurations) {
      return emptyStatePrompt;
    }

    return (
      <ManagedTable
        noItemsMessage={<LoadingStatePrompt />}
        columns={COLUMNS}
        items={data}
        initialSortField="service.name"
        initialSortDirection="asc"
        initialPageSize={50}
      />
    );
  }
}

This comment has been minimized.

Copy link
@ogupte

ogupte Sep 9, 2019

Author Contributor

created a new component for this called SettingsList (the old SettingsList is now just Settings/index since it renders the settings page.

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2019

'xpack.apm.settings.agentConf.configTable.failurePromptText',
{
defaultMessage:
'The list of agent configurations could not be fetched'

This comment has been minimized.

Copy link
@sqren

sqren Sep 9, 2019

Member

I know I'm going back and forth on this but my assumptions have changed a little.
If we do not expect the apm_user to have read access, and we therefore expect many users to see this issue because of permission problems, it might make sense to hint about that.

Eg. "The list of agent configurations could not be fetched. Your user may not have the sufficient permissions."

To be clear, I was more against the fact that the user was seeing an error for something they shouldn't be concerned about (they were seeing an issue if they didn't have write access, even if they just wanted to view/read the list).

@sqren
sqren approved these changes Sep 9, 2019
Copy link
Member

left a comment

Looks good. Mostly nits.

- Rename Settings/SettingsList -> Settings/index
- Add Settings/SettingsList to render only the list/messaging
- Mention permissions issue in failure message
@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2019

@ogupte ogupte merged commit 1f04a1f into elastic:master Sep 9, 2019

55 checks passed

API integration tests node scripts/functional_tests --config test/api_integration/config.js --bail --debug
Details
Browser tests yarn run grunt test:browser-ci
Details
Build kbn_tp_sample_panel_action yarn build
Details
CLA All commits in pull request signed
Details
Check core API changes node scripts/check_core_api_changes
Details
Check file casing node scripts/check_file_casing --quiet
Details
Check licenses node scripts/check_licenses --dev
Details
Firefox smoke test node scripts/functional_tests --bail --debug --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/kibana-firefoxSmoke/node/linux-immutable/install/kibana --include-tag smoke --config test/functional/config.firefox.js
Details
Functional tests / Group 1 yarn run grunt run:functionalTests_ciGroup1
Details
Functional tests / Group 10 yarn run grunt run:functionalTests_ciGroup10
Details
Functional tests / Group 11 yarn run grunt run:functionalTests_ciGroup11
Details
Functional tests / Group 12 yarn run grunt run:functionalTests_ciGroup12
Details
Functional tests / Group 2 yarn run grunt run:functionalTests_ciGroup2
Details
Functional tests / Group 3 yarn run grunt run:functionalTests_ciGroup3
Details
Functional tests / Group 4 yarn run grunt run:functionalTests_ciGroup4
Details
Functional tests / Group 5 yarn run grunt run:functionalTests_ciGroup5
Details
Functional tests / Group 6 yarn run grunt run:functionalTests_ciGroup6
Details
Functional tests / Group 7 yarn run grunt run:functionalTests_ciGroup7
Details
Functional tests / Group 8 yarn run grunt run:functionalTests_ciGroup8
Details
Functional tests / Group 9 yarn run grunt run:functionalTests_ciGroup9
Details
Internationalization check node scripts/i18n_check --ignore-missing
Details
Interpreter functional tests node scripts/functional_tests --config test/interpreter_functional/config.js --bail --debug --kibana-install-dir ./build/oss/kibana-8.0.0-SNAPSHOT-linux-x86_64
Details
Jest integration tests yarn run grunt test:jest_integration
Details
Jest tests yarn run grunt test:jest
Details
Kibana visual regression tests yarn run percy exec node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/kibana-visualRegression/node/linux-immutable/install/kibana --config test/visual_regression/config.ts
Details
Mocha tests node scripts/mocha
Details
Plugin functional tests node scripts/functional_tests --config test/plugin_functional/config.js --bail --debug --kibana-install-dir ./build/oss/kibana-8.0.0-SNAPSHOT-linux-x86_64
Details
Project tests yarn run grunt test:projects
Details
Type check node scripts/type_check
Details
TypeScript - all files belong to a TypeScript project node scripts/check_ts_projects
Details
Verify NOTICE.txt node scripts/notice --validate
Details
Verify dependency versions yarn run grunt verifyDependencyVersions
Details
X-Pack Chrome Functional tests / Group 1 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-ciGroup1/node/linux-immutable/install/kibana --include-tag ciGroup1
Details
X-Pack Chrome Functional tests / Group 10 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-ciGroup10/node/linux-immutable/install/kibana --include-tag ciGroup10
Details
X-Pack Chrome Functional tests / Group 2 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-ciGroup2/node/linux-immutable/install/kibana --include-tag ciGroup2
Details
X-Pack Chrome Functional tests / Group 3 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-ciGroup3/node/linux-immutable/install/kibana --include-tag ciGroup3
Details
X-Pack Chrome Functional tests / Group 4 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-ciGroup4/node/linux-immutable/install/kibana --include-tag ciGroup4
Details
X-Pack Chrome Functional tests / Group 5 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-ciGroup5/node/linux-immutable/install/kibana --include-tag ciGroup5
Details
X-Pack Chrome Functional tests / Group 6 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-ciGroup6/node/linux-immutable/install/kibana --include-tag ciGroup6
Details
X-Pack Chrome Functional tests / Group 7 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-ciGroup7/node/linux-immutable/install/kibana --include-tag ciGroup7
Details
X-Pack Chrome Functional tests / Group 8 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-ciGroup8/node/linux-immutable/install/kibana --include-tag ciGroup8
Details
X-Pack Chrome Functional tests / Group 9 node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-ciGroup9/node/linux-immutable/install/kibana --include-tag ciGroup9
Details
X-Pack Jest node scripts/jest --ci --verbose
Details
X-Pack Mocha yarn test
Details
X-Pack SIEM cyclic dependency test node legacy/plugins/siem/scripts/check_circular_deps
Details
X-Pack firefox smoke test node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-firefoxSmoke/node/linux-immutable/install/kibana --include-tag smoke --config test/functional/config.firefox.js
Details
X-Pack visual regression tests yarn run percy exec node scripts/functional_tests --debug --bail --kibana-install-dir /var/lib/jenkins/workspace/elastic+kibana+pull-request/JOB/x-pack-visualRegression/node/linux-immutable/install/kibana --config test/visual_regression/config.js
Details
elasticsearch-ci/docs Build finished.
Details
eslint node scripts/eslint --no-cache
Details
kibana-ci Build finished.
Details
percy/kibana Visual review automatically approved, no visual changes found.
Details
prbot:outdated run `node scripts/update_prs 44970` to update
prbot:release note labels
prbot:release version labels
sasslint node scripts/sasslint
Details
ogupte added a commit to ogupte/kibana that referenced this pull request Sep 9, 2019
[APM] Fix indefinite loading state in agent settings for unauthorized…
… user roles (elastic#44970)

* - handle unauthorized error to return empty list of agent settings
- display to user when a failure occurred in settings initialization
- fix incorrect settings link path

* - moved agent config index creation to the plugin setup step
- make failure text reflect a general failure to load settings

* - Rename Settings/SettingsList -> Settings/index
- Add Settings/SettingsList to render only the list/messaging
- Mention permissions issue in failure message
ogupte added a commit to ogupte/kibana that referenced this pull request Sep 9, 2019
[APM] Fix indefinite loading state in agent settings for unauthorized…
… user roles (elastic#44970)

* - handle unauthorized error to return empty list of agent settings
- display to user when a failure occurred in settings initialization
- fix incorrect settings link path

* - moved agent config index creation to the plugin setup step
- make failure text reflect a general failure to load settings

* - Rename Settings/SettingsList -> Settings/index
- Add Settings/SettingsList to render only the list/messaging
- Mention permissions issue in failure message
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 10, 2019
Merge branch 'master' of github.com:elastic/kibana into chore/console…
…-to-np-ready

* 'master' of github.com:elastic/kibana: (138 commits)
  [Canvas] i18n work on workpad header (and a few header CTAs) and convert to typescript (elastic#44943)
  update close/delete system index modals (elastic#45037)
  TS return type of createIndexPatternSelect (elastic#45107)
  [ML] Fix focus chart updating. (elastic#45146)
  [ML] Data frame transform: Fix progress in wizard create step. (elastic#45116)
  [Graph] Re-enable functional test (elastic#44683)
  [SIEM] unique table id for each top talkers table (elastic#45014)
  [SIEM] ip details heading draggable (elastic#45179)
  [Maps][File upload] Set complete on index pattern creation (elastic#44423)
  [Maps] unmount map embeddable component on destroy (elastic#45183)
  [SIEM] Adds error toasts to MapEmbeddable component (elastic#45088)
  fix redirect to maintain search query string (elastic#45184)
  [APM] One-line trace summary (elastic#44842)
  [Infra UI] Display non-metric details on Node Detail page (elastic#43551)
  [Maps][File upload] Removing bbox from parsed file pending upstream lib fix (elastic#45194)
  [Logs UI] Improve live streaming behavior when scrolling (elastic#44923)
  [APM] Fix indefinite loading state in agent settings for unauthorized user roles (elastic#44970)
  [Reporting] Rewrite addForceNowQuerystring to getFullUrls (elastic#44851)
  [Reporting/ESQueue] Improve logging of doc-update events (elastic#45077)
  [Reporting] Make screenshot capture less noisy by default (elastic#45185)
  ...
ogupte added a commit that referenced this pull request Sep 10, 2019
[APM] Fix indefinite loading state in agent settings for unauthorized…
… user roles (#44970) (#45206)

* - handle unauthorized error to return empty list of agent settings
- display to user when a failure occurred in settings initialization
- fix incorrect settings link path

* - moved agent config index creation to the plugin setup step
- make failure text reflect a general failure to load settings

* - Rename Settings/SettingsList -> Settings/index
- Add Settings/SettingsList to render only the list/messaging
- Mention permissions issue in failure message
ogupte added a commit to ogupte/kibana that referenced this pull request Sep 11, 2019
[APM] Fix indefinite loading state in agent settings for unauthorized…
… user roles (elastic#44970)

* - handle unauthorized error to return empty list of agent settings
- display to user when a failure occurred in settings initialization
- fix incorrect settings link path

* - moved agent config index creation to the plugin setup step
- make failure text reflect a general failure to load settings

* - Rename Settings/SettingsList -> Settings/index
- Add Settings/SettingsList to render only the list/messaging
- Mention permissions issue in failure message
ogupte added a commit that referenced this pull request Sep 11, 2019
[APM] Fix indefinite loading state in agent settings for unauthorized…
… user roles (#44970) (#45207)

* - handle unauthorized error to return empty list of agent settings
- display to user when a failure occurred in settings initialization
- fix incorrect settings link path

* - moved agent config index creation to the plugin setup step
- make failure text reflect a general failure to load settings

* - Rename Settings/SettingsList -> Settings/index
- Add Settings/SettingsList to render only the list/messaging
- Mention permissions issue in failure message
@sqren

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

@ogupte fyi: added the apm-test-plan-7.4.0 label so this makes it into our test plan.

@sqren

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

This works as expected. However a new issue has been opened related to this: #45610

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.