mikker / passwordless

🗝 Authentication for your Rails app without the icky-ness of passwords
MIT License
1.26k stars 87 forks source link

Getting to 1.0 #89

Closed mikker closed 1 year ago

mikker commented 3 years ago

This PR marks the beginnings of working towards a 1.0 of Passwordless.

There will be breaking changes.

Plans

  1. [x] Remove isolate_namespace
    • This prevents the weird behaviour of having to use users.sign_in_path and even weirder main_app.whatever_path.
  2. [x] Store hashed tokens.
    • Work has already gone into this but now that we are already introducing breaking changes, it will be somewhat easier.
  3. [x] Fix multiple resources
    • Due to how the default views are made, multiple resource (eg. passwordless_for :users and :admins) is broken
  4. [ ] Add new input token manually screen when signing in.
    • Currently Passwordless doesn't give a clue whether a resource (User) record was found when logging in. This is so malicious users cannot check whether a given email exists in the system. However, it is very likely that user records have uniqueness constraints on the email field so the malicious can just go try signing up with the email instead of signing in and get the answer. This leaves us with a worse user experience and without any real "security" benefits.
    • Also, if users are logging in on one device but checking their email on another, they have to transfer the link somehow.
  5. [ ] Make tokens shorter in length and in timeouts.
    • Shorter tokens are easier to type but also "easier" to brute force. However, a malicious user would have to know and be ready to brute force within the shorter token timeout. Adding the delay of BCrypt hashing tokens before checking, this is as good as impossible.
  6. [x] Remove deprecated and old upgrade code
  7. [ ] More…
    • I'd love to hear any ideas or suggestions. Now is the time for the sweeping, possibly breaking changes.
xdmx commented 3 years ago

Thank you Mikkel for your continuous work on this gem, it's one of my favorite.

I love and really look forward having tokens hashed

I've always felt weird that the sign in would accept any email and say that if you're there you may receive an email. I understand the reasons behind it, but from a UX point of view it's not that great

Some food for thoughts:

mikker commented 3 years ago

Thank you @xdmx!

  1. have you thought about having a token like what used nowadays […]:

    • Yes, this was what I was trying to describe in point number 4 ( Add new input token manually screen when signing in.). This is the approach that I'm using in an actual app, gerating 6 chars, only upper case and numbers:

      class AuthToken
      CHARS = ('A'..'Z').to_a + (0..9).to_a.map(&:to_s)
      
      def self.generate
      Array.new(6).map { CHARS.sample }.join
      end
      end
  2. Rate limiting code requests is a very good idea 👍

  3. I don't think we should add more complexity to the views or the controller. It's not very hard to add your completely own, custom controller and view code if you have special needs, like AJAX responses and such. I'd rather provide examples of custom code than include actual implementations that we'd just have to then support going forward.

  4. Yes, the render on create is sort of weird and messes with reloads. I'm not sure how big a problem it is outside development but it is sure a bit annoying when testing the create action.

rickychilcott commented 3 years ago

Having official support for system and controller/request specs (started in #71) would be really nice for 1.0

rickychilcott commented 3 years ago

I can’t think of major changes that would be needed. But 1.0 should be turbo compatible. I think that would only mean form errors may need to be handled differently.

nickbayley commented 3 years ago

I can’t think of major changes that would be needed. But 1.0 should be turbo compatible. I think that would only mean form errors may need to be handled differently.

I was trying out Turbo with my app the other day. I didn't spend a whole lot of time on it, but I did get the following error when trying to request a passwordless sign on:

Error: Form responses must redirect to another location

image

I do use custom templates for my session/new and session/create, but they don't fundamentally change anything. I can get round this by adding data: { turbo: false } to my form_with, but obviously then I don't make use of Turbo.

duarme commented 2 years ago

Protect from open redirect vulnerability

How can the open redirect vulnerability be avoided?

Probably, by making sure that passwordless_query_redirect_path cannot redirect to other websites.

In the rare instance of a multi-domain application, a passlist can be issued via a config param.

It seems to me that destination_path should accept only a path to the same domain, but here: https://github.com/mikker/passwordless/blob/484e05df011c3f588db880a85b070bb3808e7c43/app/controllers/passwordless/sessions_controller.rb#L77

Are we sure that host can only be the application host?

duarme commented 2 years ago

Allow the user to update their email address

It's currently not possible for users to update their email addresses. For any long-lived application, this is a required feature. With @rickychilcott we're thinking of the best approach here: #106

hibachrach commented 2 years ago

Regarding the Turbo issue: I believe responding with a 422 (Unprocessable Entity) should address it (based on this comment https://github.com/hotwired/turbo-rails/issues/12#issuecomment-754629885)

EDIT: ah sorry, this only applies for error scenarios

xdmx commented 2 years ago

@mikker what is missing to bring this to the finish line and release a v1 with these changes?

Would you consider releasing a v0.11 with these changes and then separately add the remaining ones (shorter tokens and sign in message) and then release a v1?

Following the semver a 0.x it would be acceptable to change something that could break (like the routes and hashed tokens) as it's still in development:

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.