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

[NEW] Alert admins when user requires approval & alert users when the account is approved/activated/deactivated #7098

Merged
merged 57 commits into from
Feb 20, 2018
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
d4495b3
Changes for issue #976. Still need some work
luisfn May 25, 2017
0255a9e
Moved email strings to propper indexes on i18n
luisfn May 25, 2017
b90b3b7
Created templates for subject and mail body
luisfn May 25, 2017
7a93379
Fix typo and code standards
luisfn May 25, 2017
a877603
Modified to change email to all admins at once
luisfn May 26, 2017
21dcb15
Added reason field into register screen
May 26, 2017
8701eb0
- Added error message for invalid reason
luisfn May 27, 2017
7f5e498
- Changed to use the correct setting
luisfn May 27, 2017
c92d2b0
- Modified admin message to contain reason and user name
luisfn May 27, 2017
eae8aa3
- Added public key to use Accounts_ManuallyApproveNewUsers on frontend
luisfn May 27, 2017
16a58b5
- Displaying reason on user info
luisfn May 28, 2017
3925e89
Added logic to remove user reason once user gets activated
luisfn May 28, 2017
f001a4c
Added reason field into register screen
luisfn May 26, 2017
21c6c73
- Added error message for invalid reason
luisfn May 27, 2017
fad9497
- Changed to use the correct setting
luisfn May 27, 2017
17ad773
- Modified admin message to contain reason and user name
luisfn May 27, 2017
db7d777
- Added public key to use Accounts_ManuallyApproveNewUsers on frontend
luisfn May 27, 2017
0f99622
- Displaying reason on user info
luisfn May 28, 2017
e6fff42
Added logic to remove user reason once user gets activated
luisfn May 28, 2017
12988c4
Removing unneeded comma
luisfn May 28, 2017
3cf721f
Fix merge
luisfn May 28, 2017
7538f00
Added conditionals to check and use reason field
luisfn May 28, 2017
e7cd5eb
Trying to fix user creation tests
luisfn May 29, 2017
777441e
Trying to fix user creation tests(2)
luisfn May 29, 2017
c0edb09
Applying last suggestions
luisfn May 30, 2017
c041848
Moving reason tests to right file
luisfn May 31, 2017
206c868
Moving reason tests to right file (2)
luisfn May 31, 2017
72d8335
Fixing test where field was not found
luisfn May 31, 2017
1482536
Changes for issue #976. Still need some work
luisfn May 25, 2017
1ebd371
Moved email strings to propper indexes on i18n
luisfn May 25, 2017
073dd0b
Created templates for subject and mail body
luisfn May 25, 2017
42201cf
Fix typo and code standards
luisfn May 25, 2017
4952eb1
Modified to change email to all admins at once
luisfn May 26, 2017
b9f983a
Added reason field into register screen
May 26, 2017
dbcba56
- Added error message for invalid reason
luisfn May 27, 2017
b10b887
- Changed to use the correct setting
luisfn May 27, 2017
4248273
- Modified admin message to contain reason and user name
luisfn May 27, 2017
87b51d0
- Added public key to use Accounts_ManuallyApproveNewUsers on frontend
luisfn May 27, 2017
195d56b
- Displaying reason on user info
luisfn May 28, 2017
24f0618
Added logic to remove user reason once user gets activated
luisfn May 28, 2017
f9d7363
- Added error message for invalid reason
luisfn May 27, 2017
a54af9b
- Changed to use the correct setting
luisfn May 27, 2017
e04387a
Removing unneeded comma
luisfn May 28, 2017
920f928
Added conditionals to check and use reason field
luisfn May 28, 2017
6069e02
Trying to fix user creation tests
luisfn May 29, 2017
26c255e
Trying to fix user creation tests(2)
luisfn May 29, 2017
23219cd
Applying last suggestions
luisfn May 30, 2017
dfff89e
Moving reason tests to right file
luisfn May 31, 2017
128116d
Moving reason tests to right file (2)
luisfn May 31, 2017
645943a
Fixing test where field was not found
luisfn May 31, 2017
d5f141b
Rebasing with develop and applied code suggestion
luisfn Jun 1, 2017
c639292
Merge remote-tracking branch 'origin/issue_976' into issue_976
luisfn Jun 1, 2017
1b564f4
Fix the tests
MartinSchoeler Jun 2, 2017
244cb4c
fix error
MartinSchoeler Jun 2, 2017
2a8d479
Merge remote-tracking branch 'origin/develop' into issue_976
rodrigok Feb 17, 2018
a9b004c
Code improvements
rodrigok Feb 17, 2018
b2d99d1
Alert by email when an account is approved/activated/deactivated
rodrigok Feb 17, 2018
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
6 changes: 6 additions & 0 deletions client/methods/unsetUserReason.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
Meteor.methods({
unsetUserReason(userId) {
Meteor.users.update(userId, { $unset: { 'reason' : 1 } });
return true;
}
});
Copy link
Member

Choose a reason for hiding this comment

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

This method shouldn't exist. First, it is not checking for user permission to unset reason, and second you could unset reason in the same method setUserActiveStatus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

6 changes: 5 additions & 1 deletion packages/rocketchat-i18n/i18n/en.i18n.json
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@
"Accounts_Enrollment_Email_Default": "<h2>Welcome to <h1>[Site_Name]</h1></h2><p>Go to [Site_URL] and try the best open source chat solution available today!</p>",
"Accounts_Enrollment_Email_Description": "You may use the following placeholders: <br /><ul><li>[name], [fname], [lname] for the user's full name, first name or last name, respectively.</li><li>[email] for the user's email.</li><li>[Site_Name] and [Site_URL] for the Application Name and URL respectively.</li></ul>",
"Accounts_Enrollment_Email_Subject_Default": "Welcome to [Site_Name]",
"Accounts_Admin_Email_Approval_Needed_Default": "<p>The user <b>[name] ([email])</b> has been registered.</p><p>Reason: <b>[reason]</b></p><p>Please check Administration -> Users to activate or delete it.</p>",
"Accounts_Admin_Email_Approval_Needed_Subject_Default": "A new user registered and needs approval",
"Accounts_ForgetUserSessionOnWindowClose": "Forget user session on window close",
"Accounts_Iframe_api_method": "Api Method",
"Accounts_Iframe_api_url": "API URL",
Expand Down Expand Up @@ -762,6 +764,7 @@
"Invalid_name": "The name must not be empty",
"Invalid_notification_setting_s": "Invalid notification setting: %s",
"Invalid_pass": "The password must not be empty",
"Invalid_reason": "The reason to join must not be empty",
"Invalid_room_name": "<strong>%s</strong> is not a valid room name,<br/> use only letters, numbers, hyphens and underscores",
"Invalid_secret_URL_message": "The URL provided is invalid.",
"Invalid_setting_s": "Invalid setting: %s",
Expand Down Expand Up @@ -1225,6 +1228,7 @@
"Read_only_changed_successfully": "Read only changed successfully",
"Read_only_channel": "Read Only Channel",
"Read_only_group": "Read Only Group",
"Reason_To_Join": "Reason to Join",
"Record": "Record",
"Redirect_URI": "Redirect URI",
"Refresh_oauth_services": "Refresh OAuth Services",
Expand Down Expand Up @@ -1724,4 +1728,4 @@
"your_message_optional": "your message (optional)",
"Your_password_is_wrong": "Your password is wrong!",
"Your_push_was_sent_to_s_devices": "Your push was sent to %s devices"
}
}
1 change: 1 addition & 0 deletions packages/rocketchat-lib/lib/placeholders.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ RocketChat.placeholders.replace = function(str, data) {
str = str.replace(/\[lname\]/g, _.strRightBack(data.name, ' ') || '');
str = str.replace(/\[email\]/g, data.email || '');
str = str.replace(/\[password\]/g, data.password || '');
str = str.replace(/\[reason\]/g, data.reason || '');

if (data.unsubscribe) {
str = str.replace(/\[unsubscribe\]/g, data.unsubscribe);
Expand Down
3 changes: 2 additions & 1 deletion packages/rocketchat-lib/server/functions/getFullUserData.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ RocketChat.getFullUserData = function({userId, filter, limit}) {
status: 1,
utcOffset: 1,
type: 1,
active: 1
active: 1,
reason: 1
};

if (RocketChat.authz.hasPermission(userId, 'view-full-other-user-info')) {
Expand Down
21 changes: 21 additions & 0 deletions packages/rocketchat-lib/server/models/Users.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ class ModelUsers extends RocketChat.models._Base {
this.tryEnsureIndex({ 'active': 1 }, { sparse: 1 });
this.tryEnsureIndex({ 'statusConnection': 1 }, { sparse: 1 });
this.tryEnsureIndex({ 'type': 1 });
this.tryEnsureIndex({ 'reason': 1 });
Copy link
Member

Choose a reason for hiding this comment

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

Do you really need this index? I don't think we'll be doing any query/sorting using reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, just forgot it there. Removing


this.cache.ensureIndex('username', 'unique');
}
Expand Down Expand Up @@ -493,6 +494,26 @@ class ModelUsers extends RocketChat.models._Base {
return this.update({ _id }, update);
}

setReason(_id, reason) {
const update = {
$set: {
reason
}
};

return this.update(_id, update);
}

unsetReason(_id) {
const update = {
$unset: {
'reason' : true
}
};

return this.update(_id, update);
}

// INSERT
create(data) {
const user = {
Expand Down
1 change: 1 addition & 0 deletions packages/rocketchat-lib/server/startup/settings.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ RocketChat.settings.addGroup('Accounts', function() {
}
});
this.add('Accounts_ManuallyApproveNewUsers', false, {
'public': true,
type: 'boolean'
});
this.add('Accounts_AllowedDomainsList', '', {
Expand Down
7 changes: 6 additions & 1 deletion packages/rocketchat-ui-flextab/client/tabs/userInfo.html
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ <h3 title="{{name}}"><i class="status-{{status}}"></i> {{name}}</h3>
{{#if hasPhone}}
{{#each phone}} <p class="secondary-font-color"><i class="icon-phone"></i> {{phoneNumber}}</p> {{/each}}
{{/if}}
{{#if lastLogin}} <p class="secondary-font-color"><i class="icon-calendar"></i> {{_ "Created_at"}}: {{createdAt}}</p> {{/if}}
{{#if createdAt}} <p class="secondary-font-color"><i class="icon-calendar"></i> {{_ "Created_at"}}: {{createdAt}}</p> {{/if}}
{{#if lastLogin}} <p class="secondary-font-color"><i class="icon-calendar"></i> {{_ "Last_login"}}: {{lastLogin}}</p> {{/if}}
{{#if services.facebook.id}} <p class="secondary-font-color"><i class="icon-facebook"></i><a href="{{services.facebook.link}}" target="_blank">{{services.facebook.name}}</a></p> {{/if}}
{{#if services.github.id}} <p class="secondary-font-color"><i class="icon-github-circled"></i><a href="https://github.com/{{services.github.username}}" target="_blank">{{services.github.username}}</a></p> {{/if}}
Expand All @@ -37,6 +37,11 @@ <h3 title="{{name}}"><i class="status-{{status}}"></i> {{name}}</h3>
{{#if services.twitter.id}} <p class="secondary-font-color"><i class="icon-twitter"></i><a href="https://twitter.com/{{services.twitter.screenName}}" target="_blank">{{services.twitter.screenName}}</a></p> {{/if}}
{{#if services.wordpress.id}} <p class="secondary-font-color"><i class="icon-wordpress"></i>{{services.wordpress.user_login}}</p> {{/if}}
{{/if}}
{{#if shouldDisplayReason}}
<p class="secondary-font-color">
{{_ "Reason_To_Join"}}: {{user.reason}}
</p>
{{/if}}
</div>
</div>
{{/with}}
Expand Down
8 changes: 8 additions & 0 deletions packages/rocketchat-ui-flextab/client/tabs/userInfo.js
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,11 @@ Template.userInfo.helpers({
isBlocker() {
const subscription = ChatSubscription.findOne({rid:Session.get('openedRoom'), 'u._id': Meteor.userId()}, { fields: { blocker: 1 } });
return subscription.blocker;
},

shouldDisplayReason() {
const user = Template.instance().user.get();
return RocketChat.settings.get('Accounts_ManuallyApproveNewUsers') && user.active === false && user.reason;
}
});

Expand Down Expand Up @@ -390,6 +395,9 @@ Template.userInfo.events({
if (user) {
return Meteor.call('setUserActiveStatus', user._id, true, function(error, result) {
if (result) {

Meteor.call('unsetUserReason', user._id);

toastr.success(t('User_has_been_activated'));
}
if (error) {
Expand Down
9 changes: 9 additions & 0 deletions packages/rocketchat-ui-login/client/login/form.html
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,15 @@ <h2 data-i18n="Registration_Succeeded">{{{_ "Registration_Succeeded"}}}</h2>
</div>
</div>
{{/if}}
{{#if manuallyApproveNewUsers}}
<div class="input-line">
<label for="reason">{{_ "Reason_To_Join"}}</label>
<div>
<input type="text" name='reason' id="reason" />
<div class="input-error"></div>
</div>
</div>
{{/if}}
{{/if}}
{{#if state 'forgot-password' 'email-verification'}}
<div class="input-line">
Expand Down
6 changes: 6 additions & 0 deletions packages/rocketchat-ui-login/client/login/form.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ Template.loginForm.helpers({
},
hasOnePassword() {
return typeof OnePassword !== 'undefined' && OnePassword.findLoginForUrl && typeof device !== 'undefined' && device.platform && device.platform.toLocaleLowerCase() === 'ios';
},
manuallyApproveNewUsers() {
return RocketChat.settings.get('Accounts_ManuallyApproveNewUsers');
}
});

Expand Down Expand Up @@ -250,6 +253,9 @@ Template.loginForm.onCreated(function() {
if (RocketChat.settings.get('Accounts_RequirePasswordConfirmation') && formObj['confirm-pass'] !== formObj['pass']) {
validationObj['confirm-pass'] = t('Invalid_confirm_pass');
}
if (RocketChat.settings.get('Accounts_ManuallyApproveNewUsers') && !formObj['reason']) {
validationObj['reason'] = t('Invalid_reason');
}
validateCustomFields(formObj, validationObj);
}
$('#login-card h2').removeClass('error');
Expand Down
48 changes: 48 additions & 0 deletions server/lib/accounts.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ Accounts.emailTemplates.siteName = RocketChat.settings.get('Site_Name');

Accounts.emailTemplates.from = `${ RocketChat.settings.get('Site_Name') } <${ RocketChat.settings.get('From_Email') }>`;

Accounts.emailTemplates.notifyAdmin = {};

const verifyEmailHtml = Accounts.emailTemplates.verifyEmail.text;

Accounts.emailTemplates.verifyEmail.html = function(user, url) {
Expand Down Expand Up @@ -56,6 +58,31 @@ Accounts.emailTemplates.enrollAccount.html = function(user = {}/*, url*/) {
return header + html + footer;
};

Accounts.emailTemplates.notifyAdmin.subject = function() {
const subject = TAPi18n.__('Accounts_Admin_Email_Approval_Needed_Subject_Default');
const siteName = RocketChat.settings.get('Site_Name');

return `[${ siteName }] ${ subject }`;
};

Accounts.emailTemplates.notifyAdmin.html = function(options = {}) {

let html;

html = TAPi18n.__('Accounts_Admin_Email_Approval_Needed_Default');

const header = RocketChat.placeholders.replace(RocketChat.settings.get('Email_Header') || '');
const footer = RocketChat.placeholders.replace(RocketChat.settings.get('Email_Footer') || '');

html = RocketChat.placeholders.replace(html, {
name: options.name,
email: options.email,
reason: options.reason
});

return header + html + footer;
};

Accounts.onCreateUser(function(options, user = {}) {
RocketChat.callbacks.run('beforeCreateUser', options, user);

Expand Down Expand Up @@ -91,6 +118,27 @@ Accounts.onCreateUser(function(options, user = {}) {
}
}

if (!user.active) {
user.emails.some((email) => {
Copy link
Member

Choose a reason for hiding this comment

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

Why are you using .some((email) => { if you are not using the email variable and you are also not returning a true value to stop the some? what's the expected result?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea, was just folowing an example from another file sending email. Fixing

const destinations = [];

RocketChat.models.Roles.findUsersInRole('admin').forEach(function(adminUser) {
destinations.push(`${ adminUser.name }<${ adminUser.emails[0].address }>`);
Copy link
Member

Choose a reason for hiding this comment

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

It's possible that an adminUser doesn't have e-mail on adminUser.emails[0].address (for example if the admin only ever logged in using OAuth). You should check if property exists.

});

email = {
to: destinations,
from: RocketChat.settings.get('From_Email'),
subject: Accounts.emailTemplates.notifyAdmin.subject(),
html: Accounts.emailTemplates.notifyAdmin.html(options)
};

Meteor.defer(() => {
Email.send(email);
});
});
}

return user;
});

Expand Down
18 changes: 17 additions & 1 deletion server/methods/registerUser.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ Meteor.methods({
registerUser(formData) {
const AllowAnonymousRead = RocketChat.settings.get('Accounts_AllowAnonymousRead');
const AllowAnonymousWrite = RocketChat.settings.get('Accounts_AllowAnonymousWrite');
const manuallyApproveNewUsers = RocketChat.settings.get('Accounts_ManuallyApproveNewUsers');
if (AllowAnonymousRead === true && AllowAnonymousWrite === true && formData.email == null) {
const userId = Accounts.insertUserDoc({}, {
globalRoles: [
Expand All @@ -19,6 +20,12 @@ Meteor.methods({
name: String,
secretURL: Match.Optional(String)
}));

if (manuallyApproveNewUsers) {
check(formData, Match.ObjectIncluding({
reason: String
}));
}
}

if (RocketChat.settings.get('Accounts_RegistrationForm') === 'Disabled') {
Expand All @@ -31,9 +38,14 @@ Meteor.methods({

const userData = {
email: s.trim(formData.email.toLowerCase()),
password: formData.pass
password: formData.pass,
name: formData.name
};

if (manuallyApproveNewUsers) {
userData.reason = formData.reason;
}

// Check if user has already been imported and never logged in. If so, set password and let it through
const importedUser = RocketChat.models.Users.findOneByEmailAddress(s.trim(formData.email.toLowerCase()));
let userId;
Expand All @@ -46,6 +58,10 @@ Meteor.methods({

RocketChat.models.Users.setName(userId, s.trim(formData.name));

if (manuallyApproveNewUsers) {
RocketChat.models.Users.setReason(userId, s.trim(formData.reason));
}

RocketChat.saveCustomFields(userId, formData);

try {
Expand Down
27 changes: 27 additions & 0 deletions server/methods/unsetUserReason.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
Meteor.methods({
unsetUserReason(userId) {
check(userId, String);

if (!Meteor.userId()) {
throw new Meteor.Error('error-invalid-user', 'Invalid user', {
method: 'unsetUserReason'
});
}

if (RocketChat.authz.hasPermission(Meteor.userId(), 'edit-other-user-active-status') !== true) {
throw new Meteor.Error('error-not-allowed', 'Not allowed', {
method: 'unsetUserReason'
});
}

const user = RocketChat.models.Users.findOneById(userId);

if (user) {
RocketChat.models.Users.unsetReason(userId);

return true;
}

return false;
}
});
Copy link
Member

Choose a reason for hiding this comment

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

Likewise on client side, this method should not exist and its content should be executed inside setUserActiveStatus

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

1 change: 1 addition & 0 deletions tests/data/user.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
export const username = `user.test.${ Date.now() }`;
export const email = `${ username }@rocket.chat`;
export const password = 'rocket.chat';
export const reason = 'rocket.chat.reason';

export const adminUsername = 'rocketchat.internal.admin.test';
export const adminEmail = `${ adminUsername }@rocket.chat`;
Expand Down
4 changes: 2 additions & 2 deletions tests/end-to-end/ui/03-user-creation.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import loginPage from '../../pageobjects/login.page';
import mainContent from '../../pageobjects/main-content.page';

//test data imports
import {username, email, password} from '../../data/user.js';
import {username, email, password, reason} from '../../data/user.js';



Expand All @@ -22,7 +22,7 @@ describe('[User Creation]', function() {
it('it should create user', () => {
loginPage.gotToRegister();

loginPage.registerNewUser({username, email, password});
loginPage.registerNewUser({username, email, password, reason});
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the reason only shows up when the approve users manually setting is on this should be tested here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @MartinSchoeler. I was looking for this


loginPage.inputUsername.waitForExist(5000);

Expand Down
4 changes: 3 additions & 1 deletion tests/pageobjects/login.page.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,15 @@ class LoginPage extends Page {
get emailField() { return browser.element('[name=email]'); }
get passwordField() { return browser.element('[name=pass]'); }
get confirmPasswordField() { return browser.element('[name=confirm-pass]'); }
get reasonField() { return browser.element('[reason]'); }
get inputUsername() { return browser.element('form#login-card input#username'); }

get emailOrUsernameInvalidText() { return browser.element('[name=emailOrUsername]~.input-error'); }
get nameInvalidText() { return browser.element('[name=name]~.input-error'); }
get emailInvalidText() { return browser.element('[name=email]~.input-error'); }
get passwordInvalidText() { return browser.element('[name=pass]~.input-error'); }
get confirmPasswordInvalidText() { return browser.element('[name=confirm-pass]~.input-error'); }
get reasonInvalidText() { return browser.element('[name=reason]~.input-error'); }
get registrationSucceededCard() { return browser.element('#login-card h2'); }

open() {
Expand All @@ -38,7 +40,7 @@ class LoginPage extends Page {
this.emailField.waitForVisible(15000);
}

registerNewUser({username, email, password}) {
registerNewUser({username, email, password, reason}) {
this.nameField.waitForVisible(5000);
this.nameField.setValue(username);
this.emailField.setValue(email);
Expand Down