Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions src/api/account_api.js
Original file line number Diff line number Diff line change
Expand Up @@ -676,6 +676,9 @@ module.exports = {
iam_path: {
type: 'string',
},
username: {
type: 'string',
},
}
},
reply: {
Expand Down
33 changes: 26 additions & 7 deletions src/sdk/accountspace_fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -794,15 +794,34 @@ class AccountSpaceFS {
}

async _check_username_already_exists(action, params, requesting_account) {
const owner_account_id = this._get_owner_account_argument(requesting_account, params);
const owner_account_id = this._get_owner_account_argument(requesting_account);
const username = params.username;
const name_exists = await this.config_fs.is_account_exists_by_name(username, owner_account_id);
if (name_exists) {
dbg.error(`AccountSpaceFS.${action} username already exists`, username);
const message_with_details = `User with name ${username} already exists.`;
const { code, http_code, type } = IamError.EntityAlreadyExists;
throw new IamError({ code, message: message_with_details, http_code, type });
const file_name_exists = await this.config_fs.is_account_exists_by_name(username, owner_account_id);
if (file_name_exists) {
this._throw_error_if_account_already_exists(action, username);
}
const is_username_lowercase_exists_under_owner = await this._check_if_account_exists_under_the_owner(
requesting_account, username);
if (is_username_lowercase_exists_under_owner) {
this._throw_error_if_account_already_exists(action, username);
}
}

_throw_error_if_account_already_exists(action, username) {
dbg.error(`AccountSpaceFS.${action} username already exists`, username);
const message_with_details = `User with name ${username} already exists.`;
const { code, http_code, type } = IamError.EntityAlreadyExists;
throw new IamError({ code, message: message_with_details, http_code, type });
}

async _check_if_account_exists_under_the_owner(requesting_account, username) {
const members = await this._list_config_files_for_users(requesting_account, undefined);
for (const member of members) {
if (member.username.toLowerCase() === username.toLowerCase()) {
return true;
}
}
return false;
}

async _copy_data_from_requesting_account_to_account_config(action, requesting_account, params) {
Expand Down
1 change: 1 addition & 0 deletions src/sdk/accountspace_nb.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ class AccountSpaceNB {
s3_access: true,
allow_bucket_creation: true,
owner: requesting_account._id.toString(),
username: params.username,
iam_path: params.iam_path,
roles: ['admin'],
// TODO: default_resource remove
Expand Down
20 changes: 13 additions & 7 deletions src/server/system_services/account_server.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,14 @@ const check_new_azure_connection_timeout = 20 * 1000;
*
*/
async function create_account(req) {
const action = IAM_ACTIONS.CREATE_USER;
let iam_arn;
if (req.rpc_params.owner) {
const user_name = account_util.get_iam_username(req.rpc_params.email.unwrap());
const action = IAM_ACTIONS.CREATE_USER;
account_util._check_if_requesting_account_is_root_account(action, req.account,
{ username: user_name, path: req.rpc_params.iam_path });
account_util._check_username_already_exists(action, req.rpc_params.email, user_name);
iam_arn = iam_utils.create_arn_for_user(req.account._id.toString(), user_name,
{ username: req.rpc_params.username, path: req.rpc_params.iam_path });
account_util._check_username_already_exists(action, req.rpc_params.email,
req.rpc_params.username);
iam_arn = iam_utils.create_arn_for_user(req.account._id.toString(), req.rpc_params.username,
req.rpc_params.iam_path || IAM_DEFAULT_PATH);
} else {
account_util.validate_create_account_permissions(req);
Expand Down Expand Up @@ -1234,8 +1234,14 @@ async function update_user(req) {
let iam_path = requested_account.iam_path;
let user_name = account_util.get_iam_username(requested_account.name.unwrap());
// Change to complete user name
const new_username = account_util.get_account_name_from_username(req.rpc_params.new_username, requesting_account._id.toString());
account_util._check_username_already_exists(action, new_username, req.rpc_params.new_username);
const is_username_update = req.rpc_params.new_username !== undefined &&
req.rpc_params.new_username !== req.rpc_params.username;
if (is_username_update) {
const email_new_username = account_util.get_account_name_from_username(
req.rpc_params.new_username,
requesting_account._id.toString());
account_util._check_username_already_exists(action, email_new_username, req.rpc_params.new_username);
}
account_util._check_if_requested_account_is_root_account_or_IAM_user(action, requesting_account, requested_account);
account_util._check_if_requested_is_owned_by_root_account(action, requesting_account, requested_account);
if (req.rpc_params.new_iam_path) iam_path = req.rpc_params.new_iam_path;
Expand Down
135 changes: 135 additions & 0 deletions src/test/integration_tests/api/iam/test_iam_basic_integration.js
Original file line number Diff line number Diff line change
Expand Up @@ -927,6 +927,141 @@ mocha.describe('IAM basic integration tests - happy path', async function() {
});
});

mocha.describe('IAM advanced integration tests', async function() {
mocha.describe('IAM User API', async function() {
const username = 'Mateo';
const username_lowercase = username.toLowerCase();
const username_uppercase = username.toUpperCase();

mocha.describe('IAM CreateUser API', async function() {
mocha.before(async () => {
// create a user
const input = {
UserName: username
};
const command = new CreateUserCommand(input);
const response = await iam_account.send(command);
_check_status_code_ok(response);
});

mocha.after(async () => {
// delete a user
const input = {
UserName: username
};
const command = new DeleteUserCommand(input);
const response = await iam_account.send(command);
_check_status_code_ok(response);
});

mocha.it('create a user with username that already exists should fail', async function() {
try {
const input = {
UserName: username
};
const command = new CreateUserCommand(input);
await iam_account.send(command);
assert.fail('create user with existing username - should throw an error');
} catch (err) {
const err_code = err.Error.Code;
assert.equal(err_code, IamError.EntityAlreadyExists.code);
}
});

mocha.it('create a user with username that already exists (lower case) should fail', async function() {
try {
const input = {
UserName: username_lowercase
};
const command = new CreateUserCommand(input);
await iam_account.send(command);
assert.fail('create user with existing username (lower case) - should throw an error');
} catch (err) {
const err_code = err.Error.Code;
assert.equal(err_code, IamError.EntityAlreadyExists.code);
}
});

mocha.it('create a user with username that already exists (upper case) should fail', async function() {
try {
const input = {
UserName: username_uppercase
};
const command = new CreateUserCommand(input);
await iam_account.send(command);
assert.fail('create user with existing username (upper case) - should throw an error');
} catch (err) {
const err_code = err.Error.Code;
assert.equal(err_code, IamError.EntityAlreadyExists.code);
}
});
});

mocha.describe('IAM UpdateUser API', async function() {
mocha.before(async () => {
// create a user
const input = {
UserName: username
};
const command = new CreateUserCommand(input);
const response = await iam_account.send(command);
_check_status_code_ok(response);
});

mocha.after(async () => {
// delete a user
const input = {
UserName: username
};
const command = new DeleteUserCommand(input);
const response = await iam_account.send(command);
_check_status_code_ok(response);
});

mocha.it('update a user with same username', async function() {
const input = {
UserName: username,
NewUserName: username,
};
const command = new UpdateUserCommand(input);
const response = await iam_account.send(command);
_check_status_code_ok(response);
});

mocha.it('update a user with new username that already exists (lower case) should fail', async function() {
try {
const input = {
UserName: username,
NewUserName: username_lowercase,
};
const command = new UpdateUserCommand(input);
await iam_account.send(command);
assert.fail('update user with existing username (lower case) - should throw an error');
} catch (err) {
const err_code = err.Error.Code;
assert.equal(err_code, IamError.EntityAlreadyExists.code);
}
});

mocha.it('update a user with new username that already exists (upper case) should fail', async function() {
try {
const input = {
UserName: username,
NewUserName: username_uppercase,
};
const command = new UpdateUserCommand(input);
await iam_account.send(command);
assert.fail('update user with existing username (upper case) - should throw an error');
} catch (err) {
const err_code = err.Error.Code;
assert.equal(err_code, IamError.EntityAlreadyExists.code);
}
});
});
Comment on lines +1000 to +1060
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

UpdateUser “duplicate username” tests don’t actually create a conflicting user

In the IAM UpdateUser API block you only ever create a single user (username). The two negative tests:

  • update a user with new username that already exists (lower case) should fail
  • update a user with new username that already exists (upper case) should fail

attempt to set NewUserName to username_lowercase / username_uppercase for that same user, but there is no other user with those names. As written, these tests will only fail if the backend incorrectly treats a rename of a user to a different casing of its own name as a conflict, and they do not validate “username already exists” behavior between distinct users.

To actually exercise case-insensitive uniqueness on update, you should create a second, conflicting user and then try to rename the first user to that name (in different casings). For example:

 mocha.describe('IAM UpdateUser API', async function() {
+                const conflicting_username = `${username}_conflict`;
+                const conflicting_username_lowercase = conflicting_username.toLowerCase();
+                const conflicting_username_uppercase = conflicting_username.toUpperCase();
+
                 mocha.before(async () => {
-                    // create a user
-                    const input = {
-                        UserName: username
-                    };
-                    const command = new CreateUserCommand(input);
-                    const response = await iam_account.send(command);
-                    _check_status_code_ok(response);
+                    // create two users so we can test collisions on update
+                    for (const name of [username, conflicting_username]) {
+                        const input = { UserName: name };
+                        const command = new CreateUserCommand(input);
+                        const response = await iam_account.send(command);
+                        _check_status_code_ok(response);
+                    }
                 });
 
                 mocha.after(async () => {
-                    // delete a user
-                    const input = {
-                        UserName: username
-                    };
-                    const command = new DeleteUserCommand(input);
-                    const response = await iam_account.send(command);
-                    _check_status_code_ok(response);
+                    // delete both users
+                    for (const name of [username, conflicting_username]) {
+                        const input = { UserName: name };
+                        const command = new DeleteUserCommand(input);
+                        const response = await iam_account.send(command);
+                        _check_status_code_ok(response);
+                    }
                 });
@@
                 mocha.it('update a user with new username that already exists (lower case) should fail', async function() {
                     try {
                         const input = {
                             UserName: username,
-                            NewUserName: username_lowercase,
+                            NewUserName: conflicting_username_lowercase,
                         };
@@
                 mocha.it('update a user with new username that already exists (upper case) should fail', async function() {
                     try {
                         const input = {
                             UserName: username,
-                            NewUserName: username_uppercase,
+                            NewUserName: conflicting_username_uppercase,
                         };

This way you test that updating one user to a name already used by another user (in any casing) correctly yields IamError.EntityAlreadyExists, while still allowing no-op updates with the same username.

🤖 Prompt for AI Agents
In src/test/integration_tests/api/iam/test_iam_basic_integration.js around lines
1000-1060, the two negative UpdateUser tests only create a single user
(username) so they never exercise a conflict with a distinct existing user;
create a second user with the conflicting name (username_lowercase /
username_uppercase) before attempting the UpdateUser call (either in the
mocha.before for this describe block or at the start of each negative test),
then attempt to rename the original user to that second user’s name (different
casing) and assert EntityAlreadyExists, and finally ensure the second user is
deleted in the mocha.after cleanup (or removed at test end) so both created
users are cleaned up.


});
});

/**
* _check_status_code_ok is an helper function to check that we got an response from the server
* @param {{ $metadata: { httpStatusCode: number; }; }} response
Expand Down
11 changes: 9 additions & 2 deletions src/util/account_util.js
Original file line number Diff line number Diff line change
Expand Up @@ -352,15 +352,22 @@ function _check_if_requesting_account_is_root_account(action, requesting_account
}
}

function _check_username_already_exists(action, email, username) {
const account = system_store.get_account_by_email(email);
function _check_username_already_exists(action, email_wrapped, username) {
const account = _check_if_account_exists_by_email(email_wrapped);
if (account) {
dbg.error(`AccountSpaceNB.${action} username already exists`, username);
const message_with_details = `User with name ${username} already exists.`;
throw new RpcError('ENTITY_ALREADY_EXISTS', message_with_details);
}
}

function _check_if_account_exists_by_email(email_wrapped) {
const accounts = system_store.data.accounts || [];
return accounts.find(account =>
account.email.unwrap().toLowerCase() === email_wrapped.unwrap().toLowerCase()
);
}

function _check_if_requested_account_is_root_account_or_IAM_user(action, requesting_account, requested_account) {
const is_requested_account_root_account = _check_root_account(requested_account);
dbg.log1(`AccountSpaceNB.${action} requested_account ID: ${requested_account._id} name: ${requested_account.name}`,
Expand Down