Project

General

Profile

Actions

Bug #4462

closed

Bundle permissions_type_recursion() with 'automatic detection' use a recursion with files

Added by Nicolas PERRON over 8 years ago. Updated 4 months ago.

Status:
Released
Priority:
1
Assignee:
Nicolas PERRON
Category:
Generic methods
Target version:
Severity:
UX impact:
User visibility:
Effort required:
Priority:
0
Regression:

Description

The bundle permissions_type_recusion() can detect automatically if target is a file or folder but the behavior is not good.
When the target is a directory, the recursion is explicitly set to 0 and if this is file, a recursion is applied (but it applies on directories too).

Actions #1

Updated by Nicolas PERRON over 8 years ago

  • Status changed from New to Pending technical review
  • % Done changed from 0 to 100
  • Pull Request set to https://github.com/Normation/ncf/pull/39

Pull Request URL added: https://github.com/Normation/ncf/pull/39

Jon, could you review it please ?

Actions #2

Updated by Nicolas PERRON over 8 years ago

  • Assignee changed from Nicolas PERRON to Jonathan CLARKE
Actions #3

Updated by Jonathan CLARKE over 8 years ago

  • Status changed from Pending technical review to Discussion
  • Assignee changed from Jonathan CLARKE to Nicolas PERRON

Nicolas, I don't understand what the problem is here.

The current implementation applies two promises to a directory (one for it's contents, the same as if it was a file, and one for the directory itself).

What is wrong here? Please explain your problem, and include in your PR an acceptance test to demonstrate the problem, and prove that it is fixed, and to ensure it never comes back.

Actions #4

Updated by Nicolas PERRON over 8 years ago

Jonathan CLARKE wrote:

Nicolas, I don't understand what the problem is here.

The current implementation applies two promises to a directory (one for it's contents, the same as if it was a file, and one for the directory itself).

What is wrong here? Please explain your problem, and include in your PR an acceptance test to demonstrate the problem, and prove that it is fixed, and to ensure it never comes back.

The problems are:
  • The use of body depth_search 'recurse' display messages: "warning: depth_search (recursion) is promised for a base object '[...]' that is not a drectory"
    • We should apply depth_search only on folders so we need to detect them to make a distinction between files and folders.
  • The use of body depth_search 'recurse' on directories will apply the permissions into the folder content and subfolder but not on the folder itself. This is due to the attribute 'include_basedir' which is set to false by default.
    • We should use the body depth_search 'recurse_with_base' instead which set 'include_basedir' to 'true'.
Actions #5

Updated by Jonathan CLARKE over 8 years ago

Nicolas PERRON wrote:

Jonathan CLARKE wrote:

Nicolas, I don't understand what the problem is here.

The current implementation applies two promises to a directory (one for it's contents, the same as if it was a file, and one for the directory itself).

What is wrong here? Please explain your problem, and include in your PR an acceptance test to demonstrate the problem, and prove that it is fixed, and to ensure it never comes back.

The problems are:
  • The use of body depth_search 'recurse' display messages: "warning: depth_search (recursion) is promised for a base object '[...]' that is not a drectory"
    • We should apply depth_search only on folders so we need to detect them to make a distinction between files and folders.

Ah OK, agreed.

  • The use of body depth_search 'recurse' on directories will apply the permissions into the folder content and subfolder but not on the folder itself. This is due to the attribute 'include_basedir' which is set to false by default.
    • We should use the body depth_search 'recurse_with_base' instead which set 'include_basedir' to 'true'.

This is already addressed by an acceptance test (https://github.com/Normation/ncf/blob/master/tree/30_generic_methods/permissions_type_recursion.cf), and that is what the "is_target_directory" class is used for in this method. I don't understand why this doesn't work?

Actions #6

Updated by Nicolas CHARLES over 8 years ago

Jonathan CLARKE wrote:

Nicolas PERRON wrote:

Jonathan CLARKE wrote:

Nicolas, I don't understand what the problem is here.

The current implementation applies two promises to a directory (one for it's contents, the same as if it was a file, and one for the directory itself).

What is wrong here? Please explain your problem, and include in your PR an acceptance test to demonstrate the problem, and prove that it is fixed, and to ensure it never comes back.

The problems are:
  • The use of body depth_search 'recurse' display messages: "warning: depth_search (recursion) is promised for a base object '[...]' that is not a drectory"
    • We should apply depth_search only on folders so we need to detect them to make a distinction between files and folders.

Ah OK, agreed.

  • The use of body depth_search 'recurse' on directories will apply the permissions into the folder content and subfolder but not on the folder itself. This is due to the attribute 'include_basedir' which is set to false by default.
    • We should use the body depth_search 'recurse_with_base' instead which set 'include_basedir' to 'true'.

This is already addressed by an acceptance test (https://github.com/Normation/ncf/blob/master/tree/30_generic_methods/permissions_type_recursion.cf), and that is what the "is_target_directory" class is used for in this method. I don't understand why this doesn't work?

It's a bit more tricky. If I'm correct, Nicolas tried to set permission 650 on the folder, but it failed
It's a feature (bug?) of CFEngine, you cannot set permission 4xx or 6xx on a folder, it will automatically change it to 5xx or 7xx (note that you can 2xx and 1xx)
I'm not sure it is a real issue (who wants to 6xx a folder ?)

Actions #7

Updated by Nicolas PERRON over 8 years ago

  • Assignee changed from Nicolas PERRON to Jonathan CLARKE

Nicolas CHARLES wrote:

Jonathan CLARKE wrote:

Nicolas PERRON wrote:

Jonathan CLARKE wrote:

Nicolas, I don't understand what the problem is here.

The current implementation applies two promises to a directory (one for it's contents, the same as if it was a file, and one for the directory itself).

What is wrong here? Please explain your problem, and include in your PR an acceptance test to demonstrate the problem, and prove that it is fixed, and to ensure it never comes back.

The problems are:
  • The use of body depth_search 'recurse' display messages: "warning: depth_search (recursion) is promised for a base object '[...]' that is not a drectory"
    • We should apply depth_search only on folders so we need to detect them to make a distinction between files and folders.

Ah OK, agreed.

  • The use of body depth_search 'recurse' on directories will apply the permissions into the folder content and subfolder but not on the folder itself. This is due to the attribute 'include_basedir' which is set to false by default.
    • We should use the body depth_search 'recurse_with_base' instead which set 'include_basedir' to 'true'.

This is already addressed by an acceptance test (https://github.com/Normation/ncf/blob/master/tree/30_generic_methods/permissions_type_recursion.cf), and that is what the "is_target_directory" class is used for in this method. I don't understand why this doesn't work?

It's a bit more tricky. If I'm correct, Nicolas tried to set permission 650 on the folder, but it failed
It's a feature (bug?) of CFEngine, you cannot set permission 4xx or 6xx on a folder, it will automatically change it to 5xx or 7xx (note that you can 2xx and 1xx)
I'm not sure it is a real issue (who wants to 6xx a folder ?)

You're right, my test failed because I've tried to set permission 650 on a folder and as you said this is a feature/bug in CFEngine.

Nevertheless, I've made these changes in order to stop having these warning: warning: depth_search (recursion) is promised for a base object '[...]' that is not a drectory which add noises into the logs.

Actions #8

Updated by Nicolas CHARLES over 8 years ago

  • Assignee changed from Jonathan CLARKE to Nicolas PERRON

Nicolas,

Thank you for this PR !
I've made a slight remark on the recurse_with_base if the class is_type_files is defined
Could you correct this and self merge ?

thank you !

Actions #9

Updated by Nicolas PERRON over 8 years ago

  • Status changed from Discussion to Pending release

Applied in changeset commit:db6e5aefb1d643c014cbd44793ad460b5d07727b.

Actions #10

Updated by Nicolas PERRON over 8 years ago

Applied in changeset commit:5fb1aabc6a6cc746ee3e8ba51e25c19f6ff6e792.

Actions #11

Updated by Vincent MEMBRÉ about 6 years ago

  • Status changed from Pending release to Released
Actions #12

Updated by Alexis Mousset 4 months ago

  • Target version changed from 0.x to ncf-0.x
  • Priority set to 0
Actions #13

Updated by Alexis Mousset 4 months ago

  • Project changed from 41 to Rudder
  • Category set to Generic methods
Actions

Also available in: Atom PDF