Imprv/use properly user public fields #2667
Conversation
| @@ -55,7 +54,6 @@ export default class PersonalContainer extends Container { | |||
| isEmailPublished: currentUser.isEmailPublished, | |||
| lang: currentUser.lang, | |||
| isGravatarEnabled: currentUser.isGravatarEnabled, | |||
| isPasswordSet: (currentUser.password != null), | |||
There was a problem hiding this comment.
client には password を渡さない
| router.get('/is-password-set', accessTokenParser, loginRequiredStrictly, async(req, res) => { | ||
| const isPasswordSet = await User.isPasswordSetById(req.user._id); | ||
| return res.apiv3({ isPasswordSet }); | ||
| }); |
There was a problem hiding this comment.
password の有無を使用するのは再設定フォームだけだった
There was a problem hiding this comment.
isPasswordSetById を廃止してこっちに持ってきたとしても、まだいろ足りない異常系処理がある。
- User.findOne に対する try-catch
|
before after |
src/server/models/user.js
Outdated
| } | ||
| const user = await this.findOne({ _id: userId }); | ||
| return (user.password != null); | ||
| }; |
There was a problem hiding this comment.
このメソッドは消そう。
- この程度の処理(
user.password != null)は route でやって差し支えない - loginRequiredStrictly をくぐり抜けたという前提があれば
findOneの結果が null であることは考慮しなくて良いが、model のメソッドとして用意するならその辺も必要になってくる
| router.get('/is-password-set', accessTokenParser, loginRequiredStrictly, async(req, res) => { | ||
| const isPasswordSet = await User.isPasswordSetById(req.user._id); | ||
| return res.apiv3({ isPasswordSet }); | ||
| }); |
There was a problem hiding this comment.
isPasswordSetById を廃止してこっちに持ってきたとしても、まだいろ足りない異常系処理がある。
- User.findOne に対する try-catch
src/server/models/user.js
Outdated
| }; | ||
|
|
||
| userSchema.statics.findUserByApiToken = function(apiToken) { | ||
| if (apiToken == null) { | ||
| return Promise.resolve(null); | ||
| } | ||
| return this.findOne({ apiToken }); | ||
| return this.findOne({ apiToken }).select(this.USER_PUBLIC_FIELDS); |
There was a problem hiding this comment.
ここで select するという戦略、もうちょっと吟味した方がいい。
悩ましい部分ではある。
現状、どのプロパティを取得するのか、API のレスポンスとして何を消すかは、model を使う側が責任を持つべきであるという考え方から、models/user.js には toObject に transform を設定してあって、様々な service や route で使っている。
こういう stackoverflow もある。
https://stackoverflow.com/questions/17795219/transform-an-array-of-documents-using-mongoose
一方で、user モデルを populate する際に select: User.USER_PUBLIC_FIELDS しているコードがあるが、これらは古いコードだと見なしてよく、なるべく toObject を使うコードにしたい。もし populate するにしても、その処理自体 route or service 側で行うべきである。例外として、populateDataTo* 系メソッドは名が体を表しているので許容する。
There was a problem hiding this comment.
補足
model の findByHoge メソッドで select や populate を入れてしまうと、以下のようなデメリットがある
- もし route, service 層で ORM の組み込みメソッド(Mongoose の findOne など)の利用を許す場合、そちらはそちらで select や populate、transform を備えた toObject の呼び出しを忘れないようにしないといけない
- にもかかわらず、findByHoge を利用するコードは別途存在し、そちらは model で対策を行っているため、route ,service 層では対策が必要なくなってしまう
- つまり、同じ route ファイルを眺めたときに、対策を行っているアクションと行っていない(model を信頼して対策を任せている)アクションが並行して存在してしまう
- これは、route, service 層のアクションのメンテ時の落とし穴になりやすい
以上の理由により、「route, service 層でのみ populate を行う」「route, service 層では、フィールド管理が必須」というルールに一本化した方がメンテナンス性が高くなる。
src/server/models/user.js
Outdated
| @@ -418,7 +418,7 @@ module.exports = function(crowi) { | |||
| const User = this; | |||
|
|
|||
| return new Promise((resolve, reject) => { | |||
| const query = User.find({ email: emailPartRegExp }, USER_PUBLIC_FIELDS); | |||
| const query = User.find({ email: emailPartRegExp }); | |||
There was a problem hiding this comment.
admin.js の usersSearch が消えたって事は、findUsersByPartOfEmail 自体消してもいいよね？
No description provided.