Skip to content

Commit bbb48bb

Browse files
cihadtekincihadtekin
authored
Merge pull request #7115 from Countly/SER-2754-2-fa-remove-2-fa-secret-from-dashboard
[SER-2754] remove 2FA secret from dashboard response
2 parents 9e76bb4 + 0b014aa commit bbb48bb

File tree

7 files changed

+211
-140
lines changed

7 files changed

+211
-140
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ Fixes:
44
- [core] Add null checking for user permission when opening the dashboard
55
- [core] Preserve URL hash during oauth
66
- [core] Rate limiting for api endpoints
7+
- [2fa] Removed the secret and qr code from the dashboard response
78

89
Enterprise Fixes:
910
- [retention_segments] Adding null check for breakdown filtering

‎plugins/two-factor-auth/api/api.js‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ var plugin = {},
66
plugins = require('../../pluginManager.js'),
77
{ validateUser } = require('../../../api/utils/rights.js');
88

9+
const { generateQRCode } = require('../lib.js');
910
const FEATURE_NAME = 'two_factor_auth';
1011

1112
plugins.setConfigs("two-factor-auth", {
@@ -148,6 +149,19 @@ plugins.register("/i/two-factor-auth", function(ob) {
148149
}
149150
});
150151
break;
152+
case "generate-qr-code":
153+
validateUser(ob.params, function() {
154+
var secret = GA.generateSecret();
155+
generateQRCode(ob.params.member.username, secret, log.w)
156+
.then((qrCode) => {
157+
common.returnOutput(ob.params, { secret, qrCode });
158+
})
159+
.catch((err) => {
160+
log.e("Error generating QR code", err);
161+
common.returnMessage(ob.params, 500, "Error generating QR code");
162+
});
163+
});
164+
break;
151165
default:
152166
common.returnMessage(ob.params, 400, "Invalid method");
153167
}

‎plugins/two-factor-auth/frontend/app.js‎

Lines changed: 52 additions & 95 deletions
Original file line numberDiff line numberDiff line change
@@ -1,40 +1,13 @@
11
var pluginObject = {},
22
{authenticator: GA} = require("otplib"),
3-
URL = require('url').URL,
4-
qrcode = require("qrcode"),
53
countlyConfig = require('../../../frontend/express/config'),
64
plugins = require('../../pluginManager.js'),
75
apiUtils = require("../../../api/utils/utils.js"),
86
members = require("../../../frontend/express/libs/members.js"),
9-
versionInfo = require("../../../frontend/express/version.info"),
107
languages = require('../../../frontend/express/locale.conf'),
118
preventBruteforce = require('../../../frontend/express/libs/preventBruteforce.js');
129

13-
/**
14-
@param {string} username - user identifier
15-
@param {string} secret - base32 encoded 2FA secret
16-
@param {function} callback - function to call with an error, if any, and a SVG string
17-
*/
18-
function generateQRCode(username, secret, callback) {
19-
var domain = versionInfo.company || versionInfo.title || "Countly";
20-
21-
if (domain === "Countly") {
22-
try {
23-
const apiURL = plugins.getConfig("api").domain;
24-
if (apiURL) {
25-
let parsedURL = new URL(apiURL);
26-
domain = parsedURL.hostname;
27-
}
28-
}
29-
catch (err) {
30-
console.log(`Error parsing api URL: ${err}`);
31-
}
32-
}
33-
34-
qrcode.toString(GA.keyuri(username, domain, secret),
35-
{type: "svg", color: {light: "#FFF0"}, errorCorrectionLevel: "L"},
36-
callback);
37-
}
10+
const { generateQRCode } = require('../lib.js');
3811

3912
(function(plugin) {
4013
plugin.init = function(app, countlyDb) {
@@ -81,7 +54,7 @@ function generateQRCode(username, secret, callback) {
8154
// everything is ok, let the user reset their password
8255
countlyDb.collection('password_reset').updateOne({prid: req.params.prid}, {$set: {two_factor_auth_passed: true}}, {}, function(passwordResetUpdateErr) {
8356
if (passwordResetUpdateErr) {
84-
console.log(`Error setting 2FA pass for password reset: ${passwordResetUpdateErr}`);
57+
console.error(`Error setting 2FA pass for password reset: ${passwordResetUpdateErr}`);
8558
}
8659
next();
8760
});
@@ -90,7 +63,7 @@ function generateQRCode(username, secret, callback) {
9063
// 2FA auth code was wrong, delete the password reset token
9164
countlyDb.collection('password_reset').deleteOne({prid: req.params.prid}, function(passwordResetDelErr) {
9265
if (passwordResetDelErr) {
93-
console.log(`Error deleting password reset: ${passwordResetDelErr}`);
66+
console.error(`Error deleting password reset: ${passwordResetDelErr}`);
9467
}
9568
});
9669
res.redirect(countlyConfig.path + '/forgot');
@@ -139,38 +112,36 @@ function generateQRCode(username, secret, callback) {
139112

140113
// modify login flow
141114
app.post(countlyConfig.path + '/login', function(req, res, next) {
142-
members.verifyCredentials(req.body.username, req.body.password, function(member) {
115+
members.verifyCredentials(req.body.username, req.body.password, async function(member) {
143116
// if member exists and 2fa is enabled globally or for the user
144117
if (member && (member.two_factor_auth && member.two_factor_auth.enabled || plugins.getConfig("two-factor-auth").globally_enabled)) {
145118
// if 2fa is not set up for the user
146119
if ((member.two_factor_auth === undefined || member.two_factor_auth.secret_token === undefined) &&
147120
(req.body.auth_code === undefined || req.body.secret_token === undefined)) {
148121
const secretToken = GA.generateSecret();
149-
150-
generateQRCode(member.username, secretToken, function(err, svg) {
151-
if (err) {
152-
console.log(`Error generating QR code: ${err}`);
153-
res.redirect(countlyConfig.path + "/login?message=two-factor-auth.login.error");
154-
}
155-
else {
156-
res.render("../../../plugins/two-factor-auth/frontend/public/templates/setup2fa", {
157-
cdn: countlyConfig.cdn || "",
158-
countlyFavicon: req.countly.favicon,
159-
countlyPage: req.countly.page,
160-
countlyTitle: req.countly.title,
161-
csrf: req.csrfToken(),
162-
inject_template: req.template,
163-
languages: languages,
164-
message: req.flash('info'),
165-
path: countlyConfig.path || "",
166-
themeFiles: req.themeFiles,
167-
username: req.body.username || "",
168-
password: req.body.password || "",
169-
secret_token: secretToken,
170-
qrcode_html: svg
171-
});
172-
}
173-
});
122+
try {
123+
const svg = await generateQRCode(member.username, secretToken, console.warn);
124+
res.render("../../../plugins/two-factor-auth/frontend/public/templates/setup2fa", {
125+
cdn: countlyConfig.cdn || "",
126+
countlyFavicon: req.countly.favicon,
127+
countlyPage: req.countly.page,
128+
countlyTitle: req.countly.title,
129+
csrf: req.csrfToken(),
130+
inject_template: req.template,
131+
languages: languages,
132+
message: req.flash('info'),
133+
path: countlyConfig.path || "",
134+
themeFiles: req.themeFiles,
135+
username: req.body.username || "",
136+
password: req.body.password || "",
137+
secret_token: secretToken,
138+
qrcode_html: svg
139+
});
140+
}
141+
catch (err) {
142+
console.error("Error generating QR code", err);
143+
res.redirect(countlyConfig.path + "/login?message=two-factor-auth.login.error");
144+
}
174145
}
175146
// else if user did not provide 2fa code (login flow first phase)
176147
else if (!req.body.auth_code) {
@@ -208,7 +179,7 @@ function generateQRCode(username, secret, callback) {
208179
},
209180
function(enableErr) {
210181
if (enableErr) {
211-
console.log(`Error enabling 2FA for ${member.username}: ${enableErr.message}`);
182+
console.error(`Error enabling 2FA for ${member.username}: ${enableErr.message}`);
212183
}
213184
else {
214185
plugins.callMethod("logAction", {req: req, res: res, user: member, action: "two_factor_auth_enabled", data: {}});
@@ -221,30 +192,29 @@ function generateQRCode(username, secret, callback) {
221192
else {
222193
// 2fa is being set up
223194
if (req.body.secret_token) {
224-
generateQRCode(member.username, req.body.secret_token, function(err, svg) {
225-
if (err) {
226-
console.log(`Error generating QR code: ${err}`);
227-
res.redirect(countlyConfig.path + "/login?message=two-factor-auth.login.error");
228-
}
229-
else {
230-
res.render("../../../plugins/two-factor-auth/frontend/public/templates/setup2fa", {
231-
cdn: countlyConfig.cdn || "",
232-
countlyFavicon: req.countly.favicon,
233-
countlyPage: req.countly.page,
234-
countlyTitle: req.countly.title,
235-
csrf: req.csrfToken(),
236-
inject_template: req.template,
237-
languages: languages,
238-
message: req.flash('info'),
239-
path: countlyConfig.path || "",
240-
themeFiles: req.themeFiles,
241-
username: req.body.username || "",
242-
password: req.body.password || "",
243-
secret_token: req.body.secret_token,
244-
qrcode_html: svg
245-
});
246-
}
247-
});
195+
try {
196+
const svg = await generateQRCode(member.username, req.body.secret_token, console.warn);
197+
res.render("../../../plugins/two-factor-auth/frontend/public/templates/setup2fa", {
198+
cdn: countlyConfig.cdn || "",
199+
countlyFavicon: req.countly.favicon,
200+
countlyPage: req.countly.page,
201+
countlyTitle: req.countly.title,
202+
csrf: req.csrfToken(),
203+
inject_template: req.template,
204+
languages: languages,
205+
message: req.flash('info'),
206+
path: countlyConfig.path || "",
207+
themeFiles: req.themeFiles,
208+
username: req.body.username || "",
209+
password: req.body.password || "",
210+
secret_token: req.body.secret_token,
211+
qrcode_html: svg
212+
});
213+
}
214+
catch (err) {
215+
console.error("Error generating QR code", err);
216+
res.redirect(countlyConfig.path + "/login?message=two-factor-auth.login.error");
217+
}
248218
}
249219
// 2fa is already set up
250220
else {
@@ -268,7 +238,7 @@ function generateQRCode(username, secret, callback) {
268238
}
269239
}
270240
catch (verifyErr) {
271-
console.log(`Error verifying 2FA for ${member.username}: ${verifyErr.message}`);
241+
console.error(`Error verifying 2FA for ${member.username}: ${verifyErr.message}`);
272242
res.redirect(countlyConfig.path + "/login?message=two-factor-auth.login.code");
273243
}
274244
}
@@ -284,19 +254,6 @@ function generateQRCode(username, secret, callback) {
284254
// these variables are passed to countly.views.js
285255
// therefore the view / vars are loaded in the existing countly window
286256
plugin.renderDashboard = function(params) {
287-
var secretToken = GA.generateSecret();
288-
289-
params.data.countlyGlobal["2fa_secret_token"] = secretToken;
290-
291-
generateQRCode(params.data.countlyGlobal.member.username, secretToken, function(err, svg) {
292-
if (err) {
293-
console.log(`Error generating QR code: ${err}`);
294-
params.data.countlyGlobal["2fa_qrcode_html"] = "<span>:(</span>";
295-
}
296-
else {
297-
params.data.countlyGlobal["2fa_qrcode_html"] = svg;
298-
}
299-
});
300257
params.data.countlyGlobal["2fa_globally_enabled"] = !!plugins.getConfig("two-factor-auth").globally_enabled;
301258
};
302259
}(pluginObject));

‎plugins/two-factor-auth/frontend/public/javascripts/countly.views.js‎

Lines changed: 43 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
/* global app, countlyVue, CV, countlyGlobal, CountlyHelpers, $ */
1+
/* global app, countlyVue, CV, countlyGlobal, CountlyHelpers, $, countlyCommon */
22

33
// if configuration view exists
44
if (app.configurationsView) {
@@ -20,15 +20,16 @@ var TwoFAUser = countlyVue.views.create({
2020
return {
2121
TFAsettings: countlyGlobal.member.two_factor_auth,
2222
dataModal: {showDialog: false},
23-
qrcode_html: $('<div>').html(countlyGlobal["2fa_qrcode_html"]).text(),
24-
secret_token: countlyGlobal["2fa_secret_token"],
25-
secret_code: ""
23+
qrcode_html: null,
24+
secret_token: null,
25+
secret_code: null
2626
};
2727
},
2828
methods: {
2929
onChange: function(value) {
3030
if (value) {
3131
this.dataModal.showDialog = true;
32+
this.getQRCode();
3233
}
3334
else {
3435
CountlyHelpers.confirm(
@@ -40,7 +41,7 @@ var TwoFAUser = countlyVue.views.create({
4041
}
4142
$.ajax({
4243
type: "GET",
43-
url: countlyGlobal.path + "/i/two-factor-auth",
44+
url: countlyCommon.API_PARTS.data.w + "/two-factor-auth",
4445
data: {
4546
method: "disable"
4647
},
@@ -86,10 +87,10 @@ var TwoFAUser = countlyVue.views.create({
8687
var self = this;
8788
$.ajax({
8889
type: "GET",
89-
url: countlyGlobal.path + "/i/two-factor-auth",
90+
url: countlyCommon.API_PARTS.data.w + "/two-factor-auth",
9091
data: {
9192
method: "enable",
92-
secret_token: countlyGlobal["2fa_secret_token"],
93+
secret_token: self.secret_token,
9394
auth_code: self.secret_code
9495
},
9596
success: function() {
@@ -121,6 +122,41 @@ var TwoFAUser = countlyVue.views.create({
121122
self.dataModal.showDialog = false;
122123
}
123124
});
125+
},
126+
getQRCode() {
127+
this.qrcode_html = null;
128+
this.secret_token = null;
129+
this.secret_code = null;
130+
$.ajax({
131+
type: "GET",
132+
url: countlyCommon.API_PARTS.data.w + '/two-factor-auth',
133+
data: {
134+
method: "generate-qr-code",
135+
"countly-token": countlyGlobal.auth_token,
136+
"Content-Type": "application/json; charset=utf-8",
137+
},
138+
success: (data) => {
139+
this.qrcode_html = countlyCommon.unescapeHtml(data.qrCode);
140+
this.secret_token = data.secret;
141+
},
142+
error: (xhr) => {
143+
var errMessage = "";
144+
145+
try {
146+
var response = JSON.parse(xhr.responseText);
147+
errMessage = response.result || xhr.statusText;
148+
}
149+
catch (err) {
150+
errMessage = xhr.statusText;
151+
}
152+
153+
CountlyHelpers.notify({
154+
title: $.i18n.map["two-factor-auth.failsetup_title"],
155+
message: $.i18n.prop("two-factor-auth.failsetup_message", errMessage),
156+
type: "error"
157+
});
158+
}
159+
});
124160
}
125161
}
126162
});

‎plugins/two-factor-auth/frontend/public/stylesheets/main.css‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,4 +99,12 @@
9999
margin: auto 6px auto 0;
100100
color: #a8a8a8;
101101
font-size: 12px;
102+
}
103+
104+
#two-factor-auth-setup-modal ol {
105+
font-size: 18px;
106+
}
107+
108+
#two-factor-auth-setup-modal ol a {
109+
color: #0166D6;
102110
}

0 commit comments

Comments
 (0)