Skip to content
This repository has been archived by the owner on Jun 5, 2024. It is now read-only.

Replace custom authentication with devise #35

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

vfonic
Copy link
Contributor

@vfonic vfonic commented Oct 4, 2020

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
Copy link
Contributor Author

vfonic commented Oct 4, 2020

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

@vfonic vfonic closed this Oct 4, 2020
@vfonic vfonic reopened this Oct 4, 2020
@vfonic
Copy link
Contributor Author

vfonic commented Oct 4, 2020

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

@inoda
Copy link
Owner

inoda commented Oct 5, 2020

@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.

@vfonic
Copy link
Contributor Author

vfonic commented Oct 5, 2020

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? :)

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

Successfully merging this pull request may close these issues.

2 participants