ontrack icon indicating copy to clipboard operation
ontrack copied to clipboard

Replace custom authentication with devise

Open vfonic opened this issue 3 years ago • 4 comments

Good job on writing password encrypting algorithm that's compatible with default devise's encrypting algorithm!

I've added devise because I'm always afraid to write my own authentication. Devise is battle tested and works.

Plus, I was constantly being logged out on my phone so that was annoying, since I use quite long password that's difficult to type on the phone.

Plus, the login was case-sensitive and on my phone, the first letter was capitalized so I'd have to downcase it while typing.

This fixes all of the above issues while keeping the backwards compatibility with the existing users/databases. :)

vfonic avatar Oct 04 '20 17:10 vfonic

Unfortunately, this is not ready to be merged in yet.

vfonic avatar Oct 04 '20 18:10 vfonic

There were couple of bugs that have now been fixed:

  1. Devise wasn't recognizing new usernames created with User.create!(username: '...'). Existing usernames worked, but no one could create new (their first) user and login successfully)
  2. When user login failed, for whatever reason, the page would reload and username input field would be populated with BCrypted username

To fix both of these issues and to simplify the code, I've converted the username to plain text. This requires users to run a rake task to set their username again. I've explained this in the README: https://github.com/inoda/ontrack/blob/18752a41ee2ee1fb9ba859e63a99b536a3b43404/README.md#upgrading

vfonic avatar Oct 04 '20 18:10 vfonic

@vfonic Some questions:

  1. "Plus, I was constantly being logged out on my phone so that was annoying, since I use quite long password that's difficult to type on the phone." - I'm not quite sure why this would happen. Currently the session is just cookie based. It doesn't automatically expire unless you log out or clear your cookies.
  2. "Plus, the login was case-sensitive and on my phone, the first letter was capitalized so I'd have to downcase it while typing." - Shouldn't the password be case sensitive? Not sure why you'd want a case insensitive password.

I'm hesitant to merge this, because it's pretty heavy weight for the needs of this app. Devise also makes it seem like a multi-user app which this is not. I'm pretty sure devise also uses bcrypt under the hood.

inoda avatar Oct 05 '20 02:10 inoda

I'm hesitant to merge this, because it's pretty heavy weight for the needs of this app.

You're right, the app, at the moment, is rather simple. I wouldn't mind expanding it further and adding more features to it. With more features being added, there will be added complexity of course. If, for example, we'd like to build a mobile app at some point, it would be easier if we'd have a good foundation to build on top of.

Devise also makes it seem like a multi-user app which this is not. I'm pretty sure devise also uses bcrypt under the hood.

Having a User model, with a whole database table, already makes it seem like a multi-user app. If we make this multi-user app, what are the drawbacks? I don't see any downsides to that, apart from making it easier to have it deployed somewhere, domain purchased and being able to share it even with non-developers, making it a real app that anyone can use just by registering. Maybe we could add subscriptions for usage as well at some point. Why not? :)

vfonic avatar Oct 05 '20 07:10 vfonic