Skip to content

Comments

[NEW] Implementation of the User create Interface in Apps.engine#15209

Closed
Cool-fire wants to merge 1 commit intoRocketChat:developfrom
Cool-fire:create-bot-user
Closed

[NEW] Implementation of the User create Interface in Apps.engine#15209
Cool-fire wants to merge 1 commit intoRocketChat:developfrom
Cool-fire:create-bot-user

Conversation

@Cool-fire
Copy link
Contributor

No description provided.

user.joinDefaultChannels = true;
}

Roles.findUsersInRole('admin').forEach((adminUser) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the loop through each user who has an admin role? What if there are ten of them, wouldn't this code run multiple times and result in several users being defined or errors happening?

Also, @RocketChat/backend how should we handle the Apps creating users? Ideally we don't want to "associate" the user being created to an admin user who didn't actually create the new user.

Copy link
Contributor Author

@Cool-fire Cool-fire Oct 5, 2019

Choose a reason for hiding this comment

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

@graywolf336 while creating rocket.cat also uses same type of loop, but since the rocket.cat is created at begining there is only single admin,but in this case as you said there can be multiple admins, which may lead to errors. My bad @graywolf336, can you please suggest how ideally the bots should be created in the server without getting errors.

Copy link
Contributor

@graywolf336 graywolf336 Oct 7, 2019

Choose a reason for hiding this comment

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

Until we hear back from @RocketChat/backend about what to do, just get any admin user and use them.

Copy link
Member

Choose a reason for hiding this comment

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

Also, @RocketChat/backend how should we handle the Apps creating users? Ideally we don't want to "associate" the user being created to an admin user who didn't actually create the new user.

actually this admin user is just used for validation purposes.. for now it will not be associated and I think it is fine to do what @graywolf336 said, just get any admin and use it.

async create(user, appId) {
this.orch.debugLog(`The App ${ appId } is requesting to create a new user`);

user.password = 'pass';
Copy link
Member

Choose a reason for hiding this comment

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

is there any purpose by having a fixed password? I'd recommend using a random password and having that password sent via email.

Suggested change
user.password = 'pass';
user.password = Random.id();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sampaiodiego The idea was to have a fixed password, and when the bot logs in for the first time it has to change the password since user.requirePasswordChange is set to true. This way we don't have to send the mail to the bot email. @graywolf336 @sampaiodiego Your thoughts??


import { saveUser } from '../../../lib/server/functions/saveUser';
import { Users } from '../../../models/server';
import { Roles } from '../../../models';
Copy link
Member

Choose a reason for hiding this comment

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

as suggested above

Suggested change
import { Roles } from '../../../models';

import { Meteor } from 'meteor/meteor';

import { saveUser } from '../../../lib/server/functions/saveUser';
import { Users } from '../../../models/server';
Copy link
Member

Choose a reason for hiding this comment

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

You can import Roles model from here

Suggested change
import { Users } from '../../../models/server';
import { Users, Roles } from '../../../models/server';

@claassistantio
Copy link

claassistantio commented Feb 21, 2020

CLA assistant check
All committers have signed the CLA.

@d-gubert
Copy link
Member

We've merged part of this implementation on #15896 along with its Apps-Engine counterpart.

Thank you so much @Cool-fire for starting this, helped us a lot! 😃

@d-gubert d-gubert closed this Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants