Skip to content

Commit e899c44

Browse files
MartiiMartii
committed
Partial refactor of auth
* Fix attach strategy broken from OpenUserJS#1867 * Fix missing redirect QSP ... long time foo. * Prep for request callback verify... split out what can be in auth. Indention is a placeholder for next phase of this and intentional. * Remove and add some duplicate *(but needed duplicate for next phase)* code. * More comments! :) Post OpenUserJS#944 NOTE(s): * This is one of the most complicated routines in the code and took quite a while to match logic *(with a few fixes)*. :P" * Checked for accidental privilege escalation with alter "User" account... none detected on dev.
1 parent 0ad21d8 commit e899c44

File tree

1 file changed

+138
-119
lines changed

1 file changed

+138
-119
lines changed

‎controllers/auth.js

+138-119
Original file line numberDiff line numberDiff line change
@@ -77,45 +77,59 @@ function getRedirect(aReq) {
7777

7878
exports.preauth = function (aReq, aRes, aNext) {
7979
var authedUser = aReq.session.user;
80-
var username = aReq.body.username || aReq.session.username ||
81-
(authedUser ? authedUser.name : null);
80+
81+
var username = aReq.body.username;
8282
var SECRET = process.env.HCAPTCHA_SECRET_KEY;
8383
var SITEKEY = process.env.HCAPTCHA_SITE_KEY;
8484

85-
if (!username) {
86-
aRes.redirect('/login?noname');
87-
return;
88-
}
89-
// Clean the username of leading and trailing whitespace,
90-
// and other stuff that is unsafe in a url
91-
username = cleanFilename(username.replace(/^\s+|\s+$/g, ''));
85+
if (!authedUser) {
86+
if (!username) {
87+
aRes.redirect('/login?noname');
88+
return;
89+
}
90+
// Clean the username of leading and trailing whitespace,
91+
// and other stuff that is unsafe in a url
92+
username = cleanFilename(username.replace(/^\s+|\s+$/g, ''));
9293

93-
// The username could be empty after the replacements
94-
if (!username) {
95-
aRes.redirect('/login?noname');
96-
return;
97-
}
94+
// The username could be empty after the replacements
95+
if (!username) {
96+
aRes.redirect('/login?noname');
97+
return;
98+
}
9899

99-
if (username.length > 64) {
100-
aRes.redirect('/login?toolong');
101-
return;
102-
}
100+
if (username.length > 64) {
101+
aRes.redirect('/login?toolong');
102+
return;
103+
}
103104

104-
User.findOne({ name: { $regex: new RegExp('^' + username + '$', 'i') } },
105-
function (aErr, aUser) {
106-
if (aErr) {
107-
console.error('Authfail with no User found of', username, aErr);
108-
aRes.redirect('/login?usernamefail');
109-
return;
110-
}
105+
User.findOne({ name: { $regex: new RegExp('^' + username + '$', 'i') } },
106+
function (aErr, aUser) {
107+
if (aErr) {
108+
console.error('Authfail with no User found of', username, aErr);
109+
aRes.redirect('/login?usernamefail');
110+
return;
111+
}
112+
113+
if (aUser || !SECRET) {
114+
// Ensure that casing is identical so we still have it, correctly, when they
115+
// get back from authentication
116+
aReq.body.username = aUser.name;
117+
118+
// Skip captcha for known individual and not implemented
119+
exports.auth(aReq, aRes, aNext);
120+
} else {
121+
// Match cleansed name and this is the casing they have chosen
122+
aReq.body.username = username;
123+
124+
// Validate captcha for unknown individual
125+
aNext();
126+
}
127+
});
128+
} else {
129+
// Skip captcha for already logged in
130+
exports.auth(aReq, aRes, aNext);
131+
}
111132

112-
if (aUser || !SECRET) {
113-
// Skip captcha for known individuals and not implemented
114-
exports.auth(aReq, aRes, aNext);
115-
} else {
116-
aNext();
117-
}
118-
});
119133
};
120134

121135
exports.auth = function (aReq, aRes, aNext) {
@@ -148,110 +162,115 @@ exports.auth = function (aReq, aRes, aNext) {
148162
}
149163
}
150164

165+
function anteauth() {
166+
// Store the useragent always so we still have it when they
167+
// get back from authentication and/or attaching
168+
aReq.session.useragent = aReq.get('user-agent');
169+
170+
User.findOne({ name: username },
171+
function (aErr, aUser) {
172+
var strategies = null;
173+
var strat = null;
174+
175+
if (aErr) { // NOTE: Possible DB error
176+
console.error('Authfail with no User found of', username, aErr);
177+
aRes.redirect('/login?usernamefail');
178+
return;
179+
}
180+
181+
if (aUser) {
182+
strategies = aUser.strategies;
183+
strat = strategies.pop();
184+
185+
if (aReq.session.newstrategy) { // authenticate with a new strategy
186+
strategy = aReq.session.newstrategy;
187+
} else if (!strategy) { // use an existing strategy
188+
strategy = strat;
189+
} else if (strategies.indexOf(strategy) === -1) {
190+
// add a new strategy but first authenticate with existing strategy
191+
aReq.session.newstrategy = strategy;
192+
strategy = strat;
193+
} // else {
194+
// use the strategy that was given in the POST
195+
// }
196+
}
197+
198+
if (!strategy) {
199+
aRes.redirect('/login?stratfail');
200+
return;
201+
} else {
202+
auth();
203+
return;
204+
}
205+
}
206+
);
207+
}
208+
151209
var authedUser = aReq.session.user;
152210
var consent = aReq.body.consent;
153211
var strategy = aReq.body.auth || aReq.params.strategy;
154-
var username = aReq.body.username || aReq.session.username ||
155-
(authedUser ? authedUser.name : null);
212+
var username = null;
156213
var authOpts = { failureRedirect: '/login?stratfail' };
157214
var passportKey = aReq._passport.instance._key;
158215

159216
if (!authedUser) {
160-
// TODO: Send out token and sitekey back to https://hcaptcha.com/siteverify
161-
// Maybe postauth routine with req hcaptcha?
162-
}
217+
// Already validated username
218+
username = aReq.body.username;
163219

164-
// Yet another passport hack.
165-
// Initialize the passport session data only when we need it.
166-
if (!aReq.session[passportKey] && aReq._passport.session) {
167-
aReq.session[passportKey] = {};
168-
aReq._passport.session = aReq.session[passportKey];
169-
}
220+
if (consent !== 'true') {
221+
aRes.redirect('/login?noconsent');
222+
return;
223+
}
170224

171-
// Save redirect url from the form submission on the session
172-
aReq.session.redirectTo = aReq.body.redirectTo || getRedirect(aReq);
173-
174-
// Allow a logged in user to add a new strategy
175-
if (strategy && authedUser) {
176-
aReq.session.passport.oujsOptions.authAttach = true;
177-
aReq.session.newstrategy = strategy;
178-
aReq.session.username = authedUser.name;
179-
} else if (authedUser) {
180-
aRes.redirect(aReq.session.redirectTo || '/');
181-
delete aReq.session.redirectTo;
182-
return;
183-
} else if (consent !== 'true') {
184-
aRes.redirect('/login?noconsent');
185-
return;
186-
}
187225

188-
if (!username) {
189-
aRes.redirect('/login?noname');
190-
return;
191-
}
192-
// Clean the username of leading and trailing whitespace,
193-
// and other stuff that is unsafe in a url
194-
username = cleanFilename(username.replace(/^\s+|\s+$/g, ''));
226+
// TODO: Send out token and sitekey back to https://hcaptcha.com/siteverify
227+
// ... routine with req hcaptcha?
228+
// If successful then do below and call anteauth otherwise redirect
229+
230+
// Yet another passport hack.
231+
// Initialize the passport session data only when we need it. i.e. late binding
232+
if (!aReq.session[passportKey] && aReq._passport.session) {
233+
aReq.session[passportKey] = {};
234+
aReq._passport.session = aReq.session[passportKey];
235+
}
195236

196-
// The username could be empty after the replacements
197-
if (!username) {
198-
aRes.redirect('/login?noname');
199-
return;
200-
}
237+
// Save redirect url from the form submission on the session
238+
aReq.session.redirectTo = aReq.body.redirectTo || getRedirect(aReq);
201239

202-
if (username.length > 64) {
203-
aRes.redirect('/login?toolong');
204-
return;
205-
}
240+
// Store the username always so we still have it when they
241+
// get back from authentication
242+
aReq.session.username = username;
206243

207-
// Store the username in the session so we still have it when they
208-
// get back from authentication
209-
if (!aReq.session.username) {
210-
aReq.session.username = username;
211-
}
212-
// Store the useragent always so we still have it when they
213-
// get back from authentication and attaching
214-
aReq.session.useragent = aReq.get('user-agent');
244+
anteauth();
215245

216-
User.findOne({ name: { $regex: new RegExp('^' + username + '$', 'i') } },
217-
function (aErr, aUser) {
218-
var strategies = null;
219-
var strat = null;
220246

221-
if (aErr) {
222-
console.error('Authfail with no User found of', username, aErr);
223-
aRes.redirect('/login?usernamefail');
224-
return;
225-
}
247+
} else {
248+
// Already validated username
249+
username = aReq.session.username || (authedUser ? authedUser.name : null);
250+
251+
// Yet another passport hack.
252+
// Initialize the passport session data only when we need it. i.e. late binding
253+
if (!aReq.session[passportKey] && aReq._passport.session) {
254+
aReq.session[passportKey] = {};
255+
aReq._passport.session = aReq.session[passportKey];
256+
}
226257

227-
if (aUser) {
228-
// Ensure that casing is identical so we still have it, correctly, when they
229-
// get back from authentication
230-
if (aUser.name !== username) {
231-
aReq.session.username = aUser.name;
232-
}
233-
strategies = aUser.strategies;
234-
strat = strategies.pop();
235-
236-
if (aReq.session.newstrategy) { // authenticate with a new strategy
237-
strategy = aReq.session.newstrategy;
238-
} else if (!strategy) { // use an existing strategy
239-
strategy = strat;
240-
} else if (strategies.indexOf(strategy) === -1) {
241-
// add a new strategy but first authenticate with existing strategy
242-
aReq.session.newstrategy = strategy;
243-
strategy = strat;
244-
} // else use the strategy that was given in the POST
245-
}
258+
// Save redirect url from the form submission on the session
259+
aReq.session.redirectTo = aReq.body.redirectTo || getRedirect(aReq);
246260

247-
if (!strategy) {
248-
aRes.redirect('/login');
249-
return;
250-
} else {
251-
auth();
252-
return;
253-
}
254-
});
261+
// Allow a logged in user to add a new strategy
262+
if (strategy) {
263+
aReq.session.passport.oujsOptions.authAttach = true;
264+
aReq.session.newstrategy = strategy;
265+
aReq.session.username = authedUser.name;
266+
} else {
267+
aRes.redirect(aReq.session.redirectTo || '/');
268+
delete aReq.session.redirectTo;
269+
return;
270+
}
271+
272+
anteauth();
273+
}
255274
};
256275

257276
exports.callback = function (aReq, aRes, aNext) {

0 commit comments

Comments
 (0)