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.

Sign up for GitHub

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

Jump to bottom

[FIX] cannot reset password #12903

Merged
merged 2 commits into from
Dec 12, 2018
Merged

[FIX] cannot reset password #12903

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fa70532
Update debug.js
ggazzo Dec 10, 2018
5f39ec9
Temporary hotfix solution for the resetpassword issue
pierre-lehnen-rc Dec 12, 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
12 changes: 11 additions & 1 deletion packages/rocketchat-lib/server/lib/debug.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,17 @@ const wrapMethods = function(name, originalHandler, methodsMap) {
const args = name === 'ufsWrite' ? Array.prototype.slice.call(originalArgs, 1) : originalArgs;
logger.method(name, '-> userId:', Meteor.userId(), ', arguments: ', args);

this.unblock();
Copy link
Member

Choose a reason for hiding this comment

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

Please do not remote this, add a condition to bypass the reset password method.

Copy link
Contributor

Choose a reason for hiding this comment

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

resetPassword is not the only method affected by this:

#12762

Copy link
Member

Choose a reason for hiding this comment

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

@Hudell do we know if it was generated by a meteor update or by converting the packages?

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked the relevant files on the meteor github and there didn't seem to be any recent change, but I'll look on their history to make sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

As I said, the meteor methods block by default, should be a good reason to think about, its a false impression of "performance", if we have a method that takes much time we should unblock or fix them in special...

Copy link
Member

Choose a reason for hiding this comment

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

looks like there many methods breaking because of this.unblock() (at least resetPasswordWithTOTP, verifyEmail, resetPassword that were mentioned on related issues), so it might safer to remove it by now since we don't know how many other might be broken.

Copy link
Member

Choose a reason for hiding this comment

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

but since we need to release a fix for those issues, I'd say we can add a bypass to those methods, cut a release and start planing to remove this unblock() for the next release. wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/meteor/meteor/blob/master/packages/ddp-common/method_invocation.js#L89 looks like this error has existed for over a year. File hasn't been changed. So we must have been getting lucky some how?

Copy link
Contributor

Choose a reason for hiding this comment

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

I debugged our 0.71 release and found out that back then the resetPassword method for some reason was not wrapped by the debug lib.

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to pick some solution, since effecting some of the important operations users do sometimes daily 😬

// Temporary solution for a hotfix while we investigate the underlying issue.
const methodBlackList = [
'resetPassword',
'verifyEmail',
'resetPasswordWithTOTP',
];

if (methodBlackList.indexOf(name) < 0) {
this.unblock();
}

const result = originalHandler.apply(this, originalArgs);
end();
return result;
Expand Down