deny null #1020

itizawa · 2019-06-06T07:56:03Z

related to #944

yuki-takei · 2019-06-06T11:34:18Z

src/server/routes/page.js

@@ -549,7 +549,7 @@ module.exports = function(crowi, app) {
  api.create = async function(req, res) {
    const body = req.body.body || null;
    const pagePath = req.body.path || null;
-    const grant = req.body.grant || null;


初期化コードは変えたくないので、null で初期化されても上手く動くように、後続のコードを変えてほしい。

yuki-takei · 2019-06-10T07:31:35Z

src/server/routes/page.js

@@ -561,6 +561,10 @@ module.exports = function(crowi, app) {
      return res.json(ApiResponse.error('Parameters body and path are required.'));
    }

+    if (!grant) {


とりあえず grant が設定されているかどうかは grant != null という式にする

yuki-takei · 2019-06-10T07:41:51Z

src/server/routes/page.js

@@ -561,6 +561,10 @@ module.exports = function(crowi, app) {
      return res.json(ApiResponse.error('Parameters body and path are required.'));
    }

+    if (!grant) {
+      return res.json(ApiResponse.error('Grant is not selected'));


解はこれじゃないなあ。

まず仕様理解

grant が設定されていない場合は、最終的なデータとしては grant を 1(published)、grantUserGroupId を null に設定しないといけない(両方書き換えないといけない)。
なぜかは説明できる？

手段の話

そして最初のFBの「後続の処理」というのは、このファイルだけにとどまらない。
models/page.js の applyScope もチェックして。routes/page.js で直すべき箇所もあるし、models/page.js で直した方がいい箇所もある。

捕捉

因みに 1 というマジックナンバーは使わないように。models/page.js に定数がある。

yuki-takei · 2019-06-17T06:16:47Z

6/10にFBしたように、「grant が設定されていない場合(クライアントからパラメータが指定されなかった場合」も正常に動くように routes/page.js を修正しないといけない。create と update 両方でエラーなく動くことを確認している？

itizawa · 2019-06-18T03:35:34Z

src/server/models/page.js

@@ -1012,7 +1016,7 @@ module.exports = function(crowi) {
    validateCrowi();

    const Revision = crowi.model('Revision');
-    const grant = options.grant || null;
+    const grant = options.grant || GRANT_PUBLIC;


updateでnullをチェックして、nullだったらgrant:1にする。
createでは
let grant = options.grant || GRANT_PUBLIC;
となっている

itizawa · 2019-06-18T03:36:11Z

src/server/models/page.js

@@ -481,6 +481,10 @@ module.exports = function(crowi) {
    if (grant === GRANT_USER_GROUP) {
      this.grantedGroup = grantUserGroupId;
    }
+
+    if (grant === GRANT_PUBLIC && grantUserGroupId !== null) {
+      this.grantedGroup = null;


grantが1でgroupidが存在すると異常なのでnullに書き換える

yuki-takei · 2019-06-18T09:48:06Z

src/server/models/page.js

    // reset
    this.grantedUsers = [];
    this.grantedGroup = null;

+    this.grant = grant || GRANT_PUBLIC;


null, undefined の場合に public にするのは最後の砦であるこのメソッドで行う

yuki-takei · 2019-06-18T09:48:22Z

src/server/models/page.js

@@ -934,7 +934,7 @@ module.exports = function(crowi) {
      .cursor();
  };

-  async function pushRevision(pageData, newRevision, user, grant, grantUserGroupId) {
+  async function pushRevision(pageData, newRevision, user) {


これはただのゴミ掃除

yuki-takei · 2019-06-18T09:49:34Z

src/server/models/page.js

@@ -973,7 +973,7 @@ module.exports = function(crowi) {
    // sanitize path
    path = crowi.xss.process(path); // eslint-disable-line no-param-reassign

-    let grant = options.grant || GRANT_PUBLIC;
+    let grant = options.grant;


Page.create に渡ってきたものはそのまま使う(portal の場合だけ public にする)

yuki-takei · 2019-06-18T09:49:42Z

src/server/models/page.js

@@ -997,7 +997,7 @@ module.exports = function(crowi) {

    let savedPage = await page.save();
    const newRevision = Revision.prepareRevision(savedPage, body, null, user, { format });
-    const revision = await pushRevision(savedPage, newRevision, user, grant, grantUserGroupId);
+    const revision = await pushRevision(savedPage, newRevision, user);


これはただのゴミ掃除

yuki-takei · 2019-06-18T09:50:02Z

src/server/models/page.js

-    const grant = options.grant || null;
-    const grantUserGroupId = options.grantUserGroupId || null;
+    const grant = options.grant || pageData.grant; //                                  use the previous data if absence
+    const grantUserGroupId = options.grantUserGroupId || pageData.grantUserGroupId; // use the previous data if absence


update 時は指定がなければ元の値を使う

yuki-takei · 2019-06-18T09:50:09Z

src/server/models/page.js

@@ -1023,7 +1023,7 @@ module.exports = function(crowi) {
    // update existing page
    let savedPage = await pageData.save();
    const newRevision = await Revision.prepareRevision(pageData, body, previousBody, user);
-    const revision = await pushRevision(savedPage, newRevision, user, grant, grantUserGroupId);
+    const revision = await pushRevision(savedPage, newRevision, user);


これはただのゴミ掃除

yuki-takei · 2019-06-18T09:50:40Z

src/server/routes/page.js

+    if (grant != null) {
+      options.grant = grant;
+      options.grantUserGroupId = grantUserGroupId;
+    }


router の create では、API body で指定がなければ options にも入れない

router の update での仕様に揃えた

undefied, null 値でも model の applyScope がよしなにやってくれるので router のスコープ外という考え方

yuki-takei · 2019-06-18T09:54:20Z

src/server/routes/page.js

@@ -1116,6 +1119,8 @@ module.exports = function(crowi, app) {
    req.body.path = newPagePath;
    req.body.body = page.revision.body;
    req.body.grant = page.grant;
+    req.body.grantedUsers = page.grantedUsers;
+    req.body.grantedGroup = page.grantedGroup;


duplicate 時に元々バグがあった部分。コピー元データを使うように修正。

yuki-takei · 2019-06-18T09:56:25Z

src/server/routes/page.js

@@ -648,8 +651,6 @@ module.exports = function(crowi, app) {
    const options = { isSyncRevisionToHackmd, socketClientId };
    if (grant != null) {
      options.grant = grant;
-    }
-    if (grantUserGroupId != null) {


applyScope が賢くなったので、ここは grant が指定されているかどうかだけ見て grantUserGroupId も options に入れた。仮に grant: 1 で grantUserGroupId に値が入っている不正な状態でも、applyScode が是正してくれる。

deny null

52a4c97

yuki-takei requested changes Jun 6, 2019

View reviewed changes

itizawa added 2 commits June 10, 2019 16:03

FB

dd63545

FB

a68e085

yuki-takei requested changes Jun 10, 2019

View reviewed changes

check Groupid

d53a6b1

itizawa added 3 commits June 17, 2019 15:51

Merge branch 'master' into deny-null-of-page-grant

3e5f610

delete grant check

8f17d34

null check

cea6d9a

itizawa commented Jun 18, 2019

View reviewed changes

yuki-takei added 2 commits June 18, 2019 18:40

refactor models/page.js, routes/page.js

2fb44c7

clean code

cf34a2d

yuki-takei reviewed Jun 18, 2019

View reviewed changes

yuki-takei approved these changes Jun 18, 2019

View reviewed changes

yuki-takei merged commit 6475ca9 into master Jun 19, 2019

yuki-takei deleted the deny-null-of-page-grant branch June 19, 2019 05:36

yuki-takei mentioned this pull request Jun 28, 2019

Improvement: ページを削除できるユーザーを限定したい #714

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deny null #1020

deny null #1020

itizawa commented Jun 6, 2019 •

edited by yuki-takei

yuki-takei Jun 6, 2019

yuki-takei Jun 10, 2019

yuki-takei Jun 10, 2019 •

edited

yuki-takei commented Jun 17, 2019 •

edited

itizawa Jun 18, 2019

itizawa Jun 18, 2019

yuki-takei Jun 18, 2019

yuki-takei Jun 18, 2019

yuki-takei Jun 18, 2019

yuki-takei Jun 18, 2019

yuki-takei Jun 18, 2019

yuki-takei Jun 18, 2019

yuki-takei Jun 18, 2019

yuki-takei Jun 18, 2019

yuki-takei Jun 18, 2019

deny null #1020

deny null #1020

Conversation

itizawa commented Jun 6, 2019 • edited by yuki-takei

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuki-takei Jun 10, 2019 • edited

Choose a reason for hiding this comment

まず仕様理解

手段の話

捕捉

yuki-takei commented Jun 17, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itizawa commented Jun 6, 2019 •

edited by yuki-takei

yuki-takei Jun 10, 2019 •

edited

yuki-takei commented Jun 17, 2019 •

edited