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

NSFS | NC | CLI | Account add/update when uid = 0 and gid != 0 bug fix #7788

Merged
merged 1 commit into from
Feb 5, 2024

Conversation

romayalon
Copy link
Contributor

@romayalon romayalon commented Feb 4, 2024

Explain the changes

  1. is_undefined() checks using if (!value), which return true when uid/gid is 0, causing the account add/update to fail.

Issues: Fixed #xxx / Gap #xxx

  1. Fixed NSFS | NC | CLI | Account add/update is failing when uid = 0 and gid != 0 and vice versa #7787

Testing Instructions:

  1. manage_nsfs.js account add --name foo --email foo@example.com --uid 0 --gid 10 2>/dev/null
  • Doc added/updated
  • Tests added

@romayalon romayalon changed the title NSFS | NC | CLI | Account add/update when uid = 0 but and gid != 0 bug fix NSFS | NC | CLI | Account add/update when uid = 0 and gid != 0 bug fix Feb 4, 2024
src/cmd/manage_nsfs.js Show resolved Hide resolved
src/test/unit_tests/test_nc_nsfs_cli.js Outdated Show resolved Hide resolved
src/test/unit_tests/test_nc_nsfs_cli.js Outdated Show resolved Hide resolved
@shirady
Copy link
Contributor

shirady commented Feb 4, 2024

@romayalon please notice there is a similar issue that is not covered by this fix:
sudo node src/cmd/manage_nsfs account add --name foo --email foo@example.com --user 0 (user name is 0).

{
  "error": {
    "code": "InvalidAccountNSFSConfig",
    "message": "Account config should not be empty, should contain UID, GID or user",
    "detail": {
      "distinguished_name": 0
    }
  }
}

@romayalon romayalon force-pushed the romy-fix-uid-gid-check branch from 42f88d7 to 72fa722 Compare February 5, 2024 14:46
@romayalon
Copy link
Contributor Author

@shirady As discussed on Slack - this is a different type of issue that will addressed by replacing the STRING_OR_NUMBER type with string only.

@romayalon please notice there is a similar issue that is not covered by this fix: sudo node src/cmd/manage_nsfs account add --name foo --email foo@example.com --user 0 (user name is 0).

{
  "error": {
    "code": "InvalidAccountNSFSConfig",
    "message": "Account config should not be empty, should contain UID, GID or user",
    "detail": {
      "distinguished_name": 0
    }
  }
}

@romayalon romayalon force-pushed the romy-fix-uid-gid-check branch from 72fa722 to a5cead5 Compare February 5, 2024 15:05
Copy link
Contributor

@shirady shirady left a comment

Choose a reason for hiding this comment

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

You can leave the comment unhandled and mentioned it as a GAP (the fix is more important)

@@ -876,7 +876,7 @@ function validate_options_type(input_options_with_data) {
/// UTILS ///
///////////////////////////////

function is_undefined(value) {
function is_string_undefined(value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If it will not be temporary as we want (as mentioned in the comment) I would consider these points:

  • If you can add a JSDoc to the function.
  • I prefer functions with verbs and change the variable name to indicate that it should only be used for names., for example:
Suggested change
function is_string_undefined(value) {
function check_string_undefined(str) {
  • Edge case - what will be when someone uses a bucket or an account with the name 0? (it still can happen, unlike a user with 0 that will be removed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bucket and account are SensitiveStrings and on SensitiveString we check str.unwrap() === 'undefined'

Signed-off-by: Romy <35330373+romayalon@users.noreply.github.com>
@romayalon romayalon force-pushed the romy-fix-uid-gid-check branch from a5cead5 to c1dcafa Compare February 5, 2024 15:44
@romayalon romayalon merged commit 2fe053f into noobaa:master Feb 5, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NSFS | NC | CLI | Account add/update is failing when uid = 0 and gid != 0 and vice versa
3 participants