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

Prevent setting default index set readonly. #3339

Merged
merged 4 commits into from Jan 12, 2017

Conversation

Projects
None yet
2 participants
@dennisoelkers
Member

dennisoelkers commented Jan 11, 2017

Description

Motivation and Context

Without this change it was possible to update the default index set and make it read only, leading to unwanted behavior. After this change, when updating an index set we check if it is the default index set and kindly refuse setting it read only.

Fixes #3331.

Additionally:

Removing superfluous id property from IndexSetUpdateRequest.

The id property of the IndexSetUpdateRequest DTO is unneeded, as the id of the index set to update is already given in the URL. It is actually misleading and requires an additional unneeded check to see if the ids of the URL and the body are consistent. Therefore the id is removed from the IndexSetUpdateRequest and the one from the URL is used only.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

dennisoelkers added some commits Jan 11, 2017

Prevent setting default index set readonly.
Without this change it was possible to update the default index set and
make it read only, leading to unwanted behavior. After this change, when
updating an index set we check if it is the default index set and kindly
refuse setting it read only.

Fixes #3331.
Removing superfluous id property from IndexSetUpdateRequest.
The id property of the IndexSetUpdateRequest DTO is unneeded, as the id
of the index set to update is already given in the URL. It is actually
misleading and requires an additional unneeded check to see if the ids
of the URL and the body are consistent.
Therefore the id is removed from the IndexSetUpdateRequest and the one
from the URL is used only.

@dennisoelkers dennisoelkers added this to the 2.2.0 milestone Jan 11, 2017

@kroepke kroepke self-assigned this Jan 11, 2017

@kroepke

Additionally it would be great to disable the UI menu element "Set as default" if we know the corresponding index set is no writable in the first place.

Other than those comments, looks good to me!

final IndexSetConfig defaultIndexSet = indexSetService.getDefault();
final boolean isDefaultSet = oldConfig.equals(defaultIndexSet);
if (isDefaultSet && !updateRequest.isWritable()) {

This comment has been minimized.

@kroepke

kroepke Jan 11, 2017

Member

This writable check here isn't enough to prevent readonly indices to become default, actually switching the current default index set is done in setDefault below, which is being used by the UI.

@kroepke kroepke assigned dennisoelkers and unassigned kroepke Jan 11, 2017

@dennisoelkers

This comment has been minimized.

Member

dennisoelkers commented Jan 12, 2017

Good points! Thanks much.

dennisoelkers added some commits Jan 12, 2017

@dennisoelkers

This comment has been minimized.

Member

dennisoelkers commented Jan 12, 2017

Added requested changes.

@kroepke

thanks, lgtm!

@kroepke kroepke merged commit faf0800 into master Jan 12, 2017

4 checks passed

ci-web-linter Jenkins build graylog-pr-linter-check 1270 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@kroepke kroepke deleted the issue-3331 branch Jan 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment