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

password encryption and other virtual attributes #377

Closed
D1plo1d opened this issue May 9, 2013 · 6 comments
Closed

password encryption and other virtual attributes #377

D1plo1d opened this issue May 9, 2013 · 6 comments

Comments

@D1plo1d
Copy link
Contributor

D1plo1d commented May 9, 2013

I'm trying to set up passport-local authentication using the default controller actions wherever possible since it seems that not using them goes very counter to the sails way. But it seems that there is no way to modify attribute values before they go into the database so I'm not sure how to encrypt the user's passwords when they sign up (post /user/create).

In rails we can define methods on the model and use them to do things like encrypting the password when it's set. These things make a lot of sense to me at the model level since their tied to the data. But I'm not seeing how to do that here, am I just missing something obvious? What would be the sails way of doing this?

I worry that because the models definitions do not become the prototypes of the instances this problem (if it is a problem) is going to become larger as time goes on. In a similar vane of thought, how would a npm module inject functionality into a model?

@D1plo1d
Copy link
Contributor Author

D1plo1d commented May 10, 2013

Ok, so I gave this some thought tonight because it's starting to look like a serious problem in my project. What I ended up was a pretty large change to how we define models but I think it could make waterline substantially more powerful and extensible with what (I hope!) are only model definition level changes:

https://gist.github.com/D1plo1d/5551823

What do you guys think? It's a very rough draft of the idea so please ask me anything that needs clarifying.

I'll hold off on implementing this until saturday in case I'm going totally the wrong direction.

@particlebanana
Copy link
Contributor

@D1plo1d I've thought about this as well. I think the prototype way would be the way to go. I had a conversation with Mike in the google group a few days back about what an interface for instance methods would look like: https://groups.google.com/forum/#!msg/sailsjs/oC0YcPrOaAk/QIxTR3QzRu0J

It's not as involved as the example you posted. It looks like you want something more closely resembling ActiveRecord in Rails. I think we could def. do something like this and keep the same adapters if we do it right. I know Associations and Validations are on the roadmap and if we have prototypes it would be easier to implement. It would also give us an interface more closely resembling Rails for associations:

Assuming a User instance that has many preference relations
user.preferences();

I really like the idea of the Mixins too! Thats a whole ecosystem of awesome that could develop. I would be down to help pull Waterline out of Sails and make it a seperate module that can later be pulled into Sails as a dependency. This would give us a start to begin experimenting with different API's for waterline.

I'll wait and see what everyone else thinks but I'm with you!

@dcbartlett
Copy link
Contributor

@particlebanana waterline has been pulled out and put back in before. Not sure why, you'd have to get with mike on that one.

@D1plo1d
Copy link
Contributor Author

D1plo1d commented May 13, 2013

In place or not, it sounds like we should do this.

@particlebanana Yes. Extra mega-full fledged agreement on all the active record/mixins/associations/validations stuff. I ended up swamped with work last weekend / this week but I'm going to be online this sunday to start on this. If you want to get started though, please go for it. I can pick up where you leave off no problem.

@particlebanana
Copy link
Contributor

@D1plo1d I talked with @mikermcneil on friday about all this and we agreed that splitting out Waterline from Sails is the right way to go. We walked through various options for the Model api and came up with something that I think will satisfy most people.

I started hacking on a version this weekend: https://github.com/particlebanana/waterline

It's still very much experimentation and will probably change a lot this week but you can see what I have now. It uses the same Collection/Model/Adapter paradigm as we currently have in waterline but adds a lot of functionality and makes Models much more powerful. I'll try and get something more stable up by this weekend but no promises on that timeline.

The main functionality we want are:

  • Attribute Definitions
  • Instance Methods
  • Class Methods
  • Validations
  • Associations
  • Lifecycle callbacks (beforeSave, beforeValidate, etc.)
  • Multiple Adapters per Model
  • Cache Adapters (think memcached/redis)

As far as the mixins go I would love them but we need to define a scope on them. The way the Collection/Model/Adapter paradigm works currently is Collection is essentially a class that sets the schema and the class methods/queries. This is what allows you to do things like User.find(), User.create(), etc. The Model is a generated prototype that injects the schema attributes/values and instance methods and is what gets returned from a query run on the Collection. This allows you to have access to properties and methods like: user.name and user.doSomething().

For modifying the schema you probably want to inject them on the parent class and set them to be processed in a lifecycle callback like:

beforeValidate: function(cb) {
  this.encryptPassword(cb);
}

This is something we can work on after the base is setup. I think we can use the encrypted password mixin as an example that we use to define how mixins are built. They should ideally be able to hook into the Collection prototype and the lifecycle callbacks so that when you include hasSecurePassword you don't need to worry about setting those steps up.

The API we came up with would look something like below. We are trying to flush out an api in the example folder on the project: https://github.com/particlebanana/waterline/tree/master/example/models

If you have any questions or thoughts you can email me at particlebanana[at]gmail.com or I'm occasionally around the IRC channel. I'll try and get something more stable for you to hack on by Sunday.

/**
 * Example User Model
 */

var Waterline = require('waterline');

var User = Waterline.Model.extend({

  // Adapters are applied from left to right
  // (methods defined in more than one adapter use the rightmost adapter's
  // version, just like _.extend)
  adapter: ['mysql', 'twilio'],

  // Association Support
  hasMany: 'Preference', // another model to link to

  attributes: {
    first_name: {
      type: 'string',
      length: { min: 5 },
      required: true
    },

    last_name: {
      type: 'string',
      length: { min: 5 },
      required: true
    },

    username: {
      type: 'string',
      length: { min: 2, max: 20 },
      unique: true,
      required: true
    },

    email: {
      type: 'email', // possibly use the various type validation in Node-Validate here?
      unique: true,
      required: true
    },

    phone_number: {
      type: 'string',
      defaultsTo: '555-555-555'
    },

    // Instance Method
    full_name: function() {
        return this.first_name + ' ' + this.last_name;
    }
  },


  /**
   * Lifecycle Callbacks
   *
   * Run before and after various stages:
   *
   * beforeValidation
   * afterValidation
   * beforeSave
   * afterSave
   * beforeCreate
   * afterCreate
   * beforeDestroy
   * afterDestroy
   */

  beforeValidate: function(cb) {
    var self = this;

    encrypt(this.password, function(err, password) {
      if(err) return next(err);
      self.password = password;
      cb();
    });
  }
});

@D1plo1d
Copy link
Contributor Author

D1plo1d commented May 20, 2013

Very cool, I'm still wrapping my head around the multiple adapters.. pretty sure that's mega-awesome though!

On my end I'm still getting a handle on the waterline code base. So while I am definitely cheerleading the hell out of this refactoring, I have some serious code walking to do before I can be of any use. Sorry dude :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants