Feature/field level authentication #46

Danosaurus · 2017-04-18T22:59:19Z

Field level authentication.

Added in the schema like so:

get schema() {
  return { 
    Field: { 
      ...
      canRead: (bundle) => {
        return Promise.resolve(!!bundle.isSudo);
      },
      canWriteOnCreate: (bundle) => {
        return Promise.resolve(!!bundle.isSudo);
      },
      canWriteOnUpdate: (bundle) => {
        return Promise.resolve(!!bundle.isSudo)
      }
    }
  }
}

coveralls · 2017-04-18T23:03:21Z

Coverage increased (+0.2%) to 91.39% when pulling 451d549 on feature/field-level-authentication into 09881a3 on master.

coveralls · 2017-04-18T23:03:21Z

Coverage increased (+0.2%) to 91.39% when pulling 451d549 on feature/field-level-authentication into 09881a3 on master.

SylvainEstevez · 2017-04-19T00:02:37Z

src/fields/lib/abstract-field.js

+      }
+      return permissionPromise;
+    })).then(authorizations => {
+      return Promise.resolve(_.reduce(authorizations, (acc, perm) => {


You don't need _, JS natively supports reduce

SylvainEstevez · 2017-04-19T00:02:59Z

src/fields/lib/abstract-field.js

+          permissionPromise = this.canWriteOnUpdate(bundle);
+          break;
+        default:
+          permissionPromise = Promise.resolve(true);


Hmm, shouldn't you throw instead ?

I think I would be better if we don't block access (by throwing) for permissions we don't support

Hmm, that's dangerous. Let's I mistype read to reed, I will never know my mistake

Good point, will change

SylvainEstevez · 2017-04-19T00:03:50Z

src/fields/lib/abstract-field.js

+      }
+      return permissionPromise;
+    })).then(authorizations => {
+      return Promise.resolve(_.reduce(authorizations, (acc, perm) => {


And actually I'm not totally sure why you want to return a single value, how to know what error to throw ?

Right now I display the permissions requested and the field accessed in the error. But I will make the error more granular.

SylvainEstevez · 2017-04-19T00:04:52Z

src/resources/lib/abstract-resource.js

        }
+        FIELD_AUTH_METHODS.forEach(permCheck => {
+          if (schema[keyItem].hasOwnProperty(permCheck)) {


This should only check for canRead, defaultSelect has nothing to do with writing

SylvainEstevez · 2017-04-19T00:06:07Z

src/resources/lib/abstract-resource.js

    }

-    let headers = bundle.req.headers['content-type'];
-    return Promise.reject(new Restypie.TemplateErrors.UnsupportedFormat({ expected: supported, value: headers }));
+    return Promise.resolve(parsed)


Hmm ?? How can you get the bundle here ?

authorize returns the bundle that is passed in

But Promise.resolve(parsed) doesn't ! :)

To be clearer, what you get in then is just parsed, not the bundle

Oh ! parsed is a promise ? Maybe use a more descriptive naming, I got confused thinking it was an object. Also you can do parserPromise.then...

SylvainEstevez · 2017-04-19T00:06:35Z

src/resources/lib/abstract-resource.js

@@ -954,6 +976,49 @@ module.exports = class AbstractResource extends Restypie.Resources.AbstractCoreR
  }

  /**
+   * Authenticates the requested fields


authorizes ?

SylvainEstevez · 2017-04-19T00:08:10Z

src/resources/lib/abstract-resource.js

+   * @param fields
+   * @returns {Promise.<TResult>}
+   */
+  authorize(bundle, fields) {


So this function only deals with selected fields ? The name might not be a good one then

I feel like you've tried to factorize, but it's making things a bit too unclear

This deals with get/patch/post fields and the permission required is obtained through getPermissions

SylvainEstevez · 2017-04-19T00:10:46Z

src/template-errors.js

+   * @extends Restypie.TemplateErrors.AbstractError
+   * @constructor
+   */
+  UnAuthorizedRequest: class UnAuthorizedRequestError extends AbstractError {


I'd rather have FieldNotReadable, FieldNotWritable, FieldNotUpdatable

SylvainEstevez · 2017-04-19T00:10:58Z

src/utils.js

  makeArray(value) {
    if (Array.isArray(value)) return value;
    if (_.isUndefined(value)) return [];
    return [value];
+  },
+
+  addIfNotInclude(array, item) {


pushUnique ?

SylvainEstevez · 2017-04-19T00:11:19Z

src/utils.js

+  },
+
+  addIfNotInclude(array, item) {
+    if (!_.includes(array, item)) {


Arrays natively support contains

Keeping lodash because native contains/includes breaks in different node versions

…ultSelect

coveralls · 2017-04-19T02:00:38Z

Coverage increased (+0.4%) to 91.585% when pulling d290fd3 on feature/field-level-authentication into 09881a3 on master.

coveralls · 2017-04-19T02:00:39Z

Coverage increased (+0.4%) to 91.585% when pulling d290fd3 on feature/field-level-authentication into 09881a3 on master.

Danosaurus · 2017-04-19T23:32:26Z

src/fields/lib/abstract-field.js

+      return Promise.resolve(this._canWriteOnUpdate.call(null, bundle))
+        .then(result => {
+          if (!result) {
+            return Promise.reject(new Restypie.TemplateErrors.FieldNotUpdatable({


Rejecting here forces the override the throw the error as well. A way to fix this is add another private method of each of these that throws the error.

coveralls · 2017-04-19T23:34:43Z

Coverage increased (+0.4%) to 91.647% when pulling 9d92a5c on feature/field-level-authentication into 09881a3 on master.

coveralls · 2017-04-19T23:48:29Z

Coverage increased (+0.4%) to 91.647% when pulling ff85239 on feature/field-level-authentication into 09881a3 on master.

coveralls · 2017-04-20T00:22:17Z

Coverage increased (+0.4%) to 91.647% when pulling 606700b on feature/field-level-authentication into 09881a3 on master.

…inEstevez/Restypie into feature/field-level-authentication

coveralls · 2017-04-20T01:42:46Z

Coverage increased (+0.4%) to 91.671% when pulling 54bdeb7 on feature/field-level-authentication into 09881a3 on master.

SylvainEstevez · 2017-04-20T17:34:23Z

src/fields/lib/abstract-field.js

+   * @private
+   */
+  _canWriteOnUpdate(bundle) {
+    return Promise.resolve(this.canWriteOnUpdate(bundle))


Why do you need to wrap this into Promise.resolve ? Doesn't canWriteOnUpdate return a Promise already ?

Returning a promise in case canWriteOnUpdate is overidden to return a value

Good idea !

SylvainEstevez · 2017-04-20T17:35:03Z

src/fields/lib/abstract-field.js

+    return Promise.resolve(this.canRead(bundle))
+      .then(result => {
+        if (!result) {
+          return Promise.reject(new Restypie.TemplateErrors.FieldNotReadable({


You can just throw

SylvainEstevez · 2017-04-20T17:35:16Z

src/fields/lib/abstract-field.js

+            key: this.key
+          }));
+        }
+        return Promise.resolve(result);


You can just return

SylvainEstevez · 2017-04-20T17:35:44Z

src/fields/lib/abstract-field.js

-          }
-        });
+    if (this._canWriteOnCreateField) {
+      return Promise.resolve(this._canWriteOnCreateField.call(null, bundle));


Why do you bind the context to null ?

Passed in because the context wasn't necessary, but I will add it in case it because useful later

coveralls · 2017-04-20T18:06:32Z

Coverage increased (+0.4%) to 91.671% when pulling bc35a79 on feature/field-level-authentication into 09881a3 on master.

coveralls · 2017-04-20T18:06:32Z

Coverage increased (+0.4%) to 91.671% when pulling bc35a79 on feature/field-level-authentication into 09881a3 on master.

coveralls · 2017-04-20T18:06:32Z

Coverage increased (+0.4%) to 91.671% when pulling bc35a79 on feature/field-level-authentication into 09881a3 on master.

coveralls · 2017-04-21T00:59:51Z

Coverage increased (+0.4%) to 91.651% when pulling e4a2b35 on feature/field-level-authentication into 09881a3 on master.

SylvainEstevez · 2017-04-21T00:27:09Z

src/basic-routes/lib/get-many.js

-      .add((bundle) => {
-        const includeScore = bundle.hasOption(Restypie.QueryOptions.INCLUDE_SCORE);
-        const scoreOnly = bundle.hasOption(Restypie.QueryOptions.SCORE_ONLY);
+    return Promise.resolve(resource.authorize(bundle)).then(() => {


Why not to just add resource.authorize to the pipeline ?

SylvainEstevez · 2017-04-21T00:27:36Z

src/basic-routes/lib/get-single.js

-            .setStatusCode(Restypie.Codes.OK)
-            .next();
-        });
+    return Promise.resolve(resource.authorize(bundle))


SylvainEstevez · 2017-04-21T00:28:18Z

src/fields/lib/abstract-field.js

+   * @private
+   */
+  _canWriteOnUpdate(bundle) {
+    return Promise.resolve(this.canWriteOnUpdate(bundle))


Good idea !

SylvainEstevez · 2017-04-21T00:36:01Z

src/resources/lib/abstract-resource.js

+      return Promise.reject(new Restypie.TemplateErrors.UnsupportedFormat({ expected: supported, value: headers }));
+    }
+
+    return Promise.resolve(parserPromise)


return parserPromise.then... will do the same

SylvainEstevez · 2017-04-21T00:37:15Z

src/resources/lib/abstract-resource.js

+    return Promise.resolve(parserPromise)
+      .then(bundle => {
+        const data = Restypie.Utils.makeArray(bundle.body);
+        const fields = Object.getOwnPropertyNames(_.reduce(data, (acc, object) => {


What format are you trying to obtain ? There might be an easier way to get it

SylvainEstevez · 2017-04-21T01:50:08Z

src/template-errors.js

@@ -45,6 +45,14 @@ class AbstractForbiddenError extends AbstractError {
  }
 }

+class AbstractUnauthorizedError extends AbstractError {
+  get statusCode() { return Restypie.Codes.Unauthorized; }


Confusion : Unauthorized actually means "Requires authentication". If the caller doesn't have the authorization to perform an operation, then the status code to send back is Forbidden. (I know...)

SylvainEstevez · 2017-04-21T01:51:21Z

src/index.js

+      return null;
+    }
+    index = index || 0;
+    separator = separator || this.POPULATE_PATH_SEPARATOR;


. is also used for deep filtering too. So you probably want this constants to be something like DEEP_PATH_SEPARATOR

coveralls · 2017-04-21T22:01:42Z

Coverage increased (+0.4%) to 91.632% when pulling c51a7aa on feature/field-level-authentication into 09881a3 on master.

Danosaurus added 3 commits April 10, 2017 18:28

✨ Added auth check on abstract field

8cedfc9

🔒 Field level WRITE access

5414cd0

🔧 Update sequelize

451d549

Danosaurus added the enhancement label Apr 18, 2017

Danosaurus requested a review from SylvainEstevez April 18, 2017 22:59

SylvainEstevez requested changes Apr 19, 2017

View reviewed changes

🔒 Field level READ access control, fields cannot have canRead an defa…

d290fd3

…ultSelect

Danosaurus added 3 commits April 19, 2017 11:09

💄 Renames for clarity

5b3035e

🔒 Added support for populate

efda035

🔧 Error handling refactor

9d92a5c

Danosaurus commented Apr 19, 2017

View reviewed changes

🐛 Using lodash includes for stability of all node versions

ff85239

Merge branch 'master' into feature/field-level-authentication

606700b

Danosaurus added 2 commits April 19, 2017 18:33

🔧 Added private methods to simplify abstract field overrides

c8ff22f

Merge branch 'feature/field-level-authentication' of github.com:Sylva…

54bdeb7

…inEstevez/Restypie into feature/field-level-authentication

SylvainEstevez reviewed Apr 20, 2017

View reviewed changes

src/fields/lib/abstract-field.js Outdated

key: this.key

}));

}

return Promise.resolve(result);

Copy link

Collaborator

SylvainEstevez Apr 20, 2017

Choose a reason for hiding this comment

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

You can just return

SylvainEstevez reviewed Apr 20, 2017

View reviewed changes

💄 return/throw over promise

bc35a79

🔧 Removed unecessary then, 401->403

e4a2b35

SylvainEstevez requested changes Apr 21, 2017

View reviewed changes

💄 Cleanup

c51a7aa

Feature/field level authentication #46

Are you sure you want to change the base?

Feature/field level authentication #46

Conversation

Danosaurus commented Apr 18, 2017

Field level authentication.

coveralls commented Apr 18, 2017

coveralls commented Apr 18, 2017 • 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

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

coveralls commented Apr 19, 2017

coveralls commented Apr 19, 2017 • edited

Choose a reason for hiding this comment

coveralls commented Apr 19, 2017

coveralls commented Apr 19, 2017 • edited

coveralls commented Apr 20, 2017 • edited

coveralls commented Apr 20, 2017 • 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

coveralls commented Apr 20, 2017

coveralls commented Apr 20, 2017

coveralls commented Apr 20, 2017

coveralls commented Apr 21, 2017 • 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

coveralls commented Apr 21, 2017 • edited

coveralls commented Apr 18, 2017 •

edited

coveralls commented Apr 19, 2017 •

edited

coveralls commented Apr 19, 2017 •

edited

coveralls commented Apr 20, 2017 •

edited

coveralls commented Apr 20, 2017 •

edited

coveralls commented Apr 21, 2017 •

edited

coveralls commented Apr 21, 2017 •

edited