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

imprv: Exception handling for user authentication #6019

Merged
merged 3 commits into from
Jun 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions packages/app/resource/locales/en_US/translation.json
Original file line number Diff line number Diff line change
Expand Up @@ -984,6 +984,7 @@
"application_already_installed": "Application already installed.",
"email_address_could_not_be_used": "This email address could not be used. (Make sure the allowed email address)",
"user_id_is_not_available":"This User ID is not available.",
"username_should_not_be_null":"Username should not be null. Please check Authentication Mechanism Settings on admin page",
"email_address_is_already_registered":"This email address is already registered.",
"can_not_register_maximum_number_of_users":"Can not register more than the maximum number of users.",
"failed_to_register":"Failed to register.",
Expand Down
1 change: 1 addition & 0 deletions packages/app/resource/locales/ja_JP/translation.json
Original file line number Diff line number Diff line change
Expand Up @@ -977,6 +977,7 @@
"application_already_installed": "アプリケーションのインストールが完了しました。",
"email_address_could_not_be_used":"このメールアドレスは使用できません。(許可されたメールアドレスを確認してください。)",
"user_id_is_not_available":"このユーザーIDは使用できません。",
"username_should_not_be_null":"Username が null になっています 管理画面の認証機構設定にて設定の確認をしてください",
"email_address_is_already_registered":"このメールアドレスは既に登録されています。",
"can_not_register_maximum_number_of_users":"ユーザー数が上限を超えたため登録できません。",
"failed_to_register":"登録に失敗しました。",
Expand Down
3 changes: 2 additions & 1 deletion packages/app/resource/locales/zh_CN/translation.json
Original file line number Diff line number Diff line change
Expand Up @@ -986,7 +986,8 @@
"aws_sttings_required": "使用此功能所需的AWS设置。请询问管理员。",
"application_already_installed": "应用程序已安装。",
"email_address_could_not_be_used": "无法使用此电子邮件地址。(确保允许的电子邮件地址)",
"user_id_is_not_available": "此用户ID不可用。",
"user_id_is_not_available": "此用户ID不可用。",
"username_should_not_be_null":"用户名不应为空。请检查管理页面上的身份验证机制设置",
"email_address_is_already_registered": "此电子邮件地址已注册。",
"can_not_register_maximum_number_of_users": "注册的用户数不能超过最大值。",
"failed_to_register": "注册失败。",
Expand Down
6 changes: 6 additions & 0 deletions packages/app/src/server/models/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,9 @@ export class PathAlreadyExistsError extends ExtensibleCustomError {
}

}


/*
* User Authentication
*/
export class NullUsernameToBeRegisteredError extends ExtensibleCustomError {}
Comment on lines +15 to +18
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Usernameがnullの場合のカスタムエラーを NullUsernameToBeRegisteredErrorとして定義しました。

5 changes: 5 additions & 0 deletions packages/app/src/server/models/external-account.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// disable no-return-await for model functions
/* eslint-disable no-return-await */
import { NullUsernameToBeRegisteredError } from '~/server/models/errors';

const debug = require('debug')('growi:models:external-account');
const mongoose = require('mongoose');
Expand Down Expand Up @@ -104,6 +105,10 @@ class ExternalAccount {
return account;
}

if (usernameToBeRegistered == null) {
throw new NullUsernameToBeRegisteredError('username_should_not_be_null');
}

const User = ExternalAccount.crowi.model('User');

let promise = User.findOne({ username: usernameToBeRegistered });
Expand Down
1 change: 0 additions & 1 deletion packages/app/src/server/routes/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ module.exports = function(crowi, app) {
app.get('/login/invited' , applicationInstalled, login.invited);
app.post('/login/activateInvited' , apiLimiter , applicationInstalled, loginFormValidator.inviteRules(), loginFormValidator.inviteValidation, csrf, login.invited);
app.post('/login' , apiLimiter , applicationInstalled, loginFormValidator.loginRules(), loginFormValidator.loginValidation, csrf, loginPassport.loginWithLocal, loginPassport.loginWithLdap, loginPassport.loginFailure);
app.post('/login' , apiLimiter , applicationInstalled, loginFormValidator.loginRules(), loginFormValidator.loginValidation, csrf, loginPassport.loginWithLocal, loginPassport.loginWithLdap, loginPassport.loginFailure);
yuki-takei marked this conversation as resolved.
Show resolved Hide resolved

app.post('/register' , apiLimiter , applicationInstalled, registerFormValidator.registerRules(), registerFormValidator.registerValidation, csrf, login.register);
app.get('/register' , applicationInstalled, login.preLogin, login.register);
Expand Down
8 changes: 7 additions & 1 deletion packages/app/src/server/routes/login-passport.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { NullUsernameToBeRegisteredError } from '~/server/models/errors';
import loggerFactory from '~/utils/logger';

/* eslint-disable no-use-before-define */
Expand Down Expand Up @@ -112,6 +113,7 @@ module.exports = function(crowi, app) {
const usernameToBeRegistered = ldapAccountInfo[attrMapUsername];
const nameToBeRegistered = ldapAccountInfo[attrMapName];
const mailToBeRegistered = ldapAccountInfo[attrMapMail];

const userInfo = {
id: ldapAccountId,
username: usernameToBeRegistered,
Expand Down Expand Up @@ -578,7 +580,11 @@ module.exports = function(crowi, app) {
}
catch (err) {
/* eslint-disable no-else-return */
if (err.name === 'DuplicatedUsernameException') {
if (err instanceof NullUsernameToBeRegisteredError) {
req.flash('warningMessage', req.t(`message.${err.message}`));
return;
}
else if (err.name === 'DuplicatedUsernameException') {
if (isSameEmailTreatedAsIdenticalUser || isSameUsernameTreatedAsIdenticalUser) {
// associate to existing user
debug(`ExternalAccount '${userInfo.username}' will be created and bound to the exisiting User account`);
Expand Down