Merge pull request #432 from onetimesecret/404-add-support-for-deleti…

…ng-account

Add support for deleting account aka "you're in" modal danger style

.github/workflows/greetings.yml

@@ -12,5 +12,5 @@ jobs:
     - uses: actions/first-interaction@v1
       with:
         repo-token: ${{ secrets.GITHUB_TOKEN }}
-        issue-message: "Thanks for taking the time to create an issue -- @delano"
-        pr-message: "Thanks so much for contributing to onetimesecret -- @delano"
+        issue-message: "Hey thanks for taking the time to create an issue -- @delano"
+        pr-message: "Sweet, thanks for the PR. Ping me here if you need anything -- @delano"

.github/workflows/housekeeping.yml

@@ -21,9 +21,9 @@ jobs:
     steps:
     - uses: actions/stale@28ca1036281a5e5922ead5184a1bbf96e5fc984e # v9
       with:
-        stale-issue-message: 'Just a heads up -- this issue is now stale (open 90 days with no activity). It will close automatically in a few weeks. You can comment or remove the stale label to restart the clock.'
+        stale-issue-message: 'Just a heads up -- this issue is being marked stale (open 90 days with no activity). It will close automatically in a few weeks. You can comment or remove the stale label to restart the clock.'
         close-issue-message: 'This issue is now closed. 🌻'
-        stale-pr-message: 'Just a heads up -- this PR is now stale (open 45 days with no activity). It will close automatically in a couple weeks. You can comment or remove the stale label to restart the clock.'
+        stale-pr-message: 'Just a heads up -- this PR is being marked stale (open 45 days with no activity). It will close automatically in a couple weeks. You can comment or remove the stale label to restart the clock.'
         close-pr-message: 'This PR is now closed.'
         days-before-issue-stale: 90
         days-before-issue-close: 28
@@ -34,5 +34,3 @@ jobs:
         exempt-draft-pr: true
         stale-issue-label: Stale Boetticher
         stale-pr-label: Stale Boetticher
-
-

.github/workflows/ruby.yml

@@ -41,6 +41,14 @@ on:
 
 jobs:
   build:
+    # Can also be set to `self-hosted` if worker is running. Possible
+    # issue when switching (or trying to run a mix of github-hosted
+    # and self-hosted). All workflows in the repo stopped being queued
+    # up after having a self-hosted worker registered and available
+    # and then manually shutdown and removed. To the extent that open
+    # PRs showed 0 checks at all (as though there were no workflow
+    # yaml files at all). Adding a new workflow in a test branch
+    # seemed to wake up the dragon and all was well in the world.
     runs-on: ubuntu-latest
 
     strategy:

.prettierrc

@@ -1,5 +1,7 @@
 {
+  "parser": "vue",
   "tabWidth": 2,
   "useTabs": false,
+  "bracketSameLine": true,
   "endOfLine": "lf"
 }

config.ru

@@ -33,6 +33,11 @@ Onetime.boot! :app
 
 if Otto.env?(:dev)
 
+  if Onetime.debug
+    require 'pry-byebug'
+    Otto.debug = true
+  end
+
   # DEV: Run web apps with extra logging and reloading
   apps.each_pair do |path, app|
     map(path) do
@@ -45,8 +50,8 @@ if Otto.env?(:dev)
       run app
     end
   end
-else
 
+else
   # PROD: run barebones webapps
   apps.each_pair do |path, app|
     app.option[:public] = PUBLIC_DIR

etc/config.test

@@ -55,3 +55,4 @@
   :show_metadata: 1000
   :show_secret: 1000
   :burn_secret: 1000
+  :destroy_account: 5

lib/onetime.rb

@@ -422,6 +422,10 @@ def initialize(identifier, event, count)
       @event = event
       @count = count
     end
+
+    def message
+      "[limit-exceeded] #{identifier} for #{event} (#{count})"
+    end
   end
 end
 

lib/onetime/app/web/account.rb

@@ -166,14 +166,22 @@ def account
     end
 
     def update_account
-      authenticated do
+      authenticated('/account') do
         logic = OT::Logic::UpdateAccount.new sess, cust, req.params, locale
         logic.raise_concerns
         logic.process
         res.redirect app_path('/account')
       end
     end
 
+    def destroy_account
+      authenticated('/account') do
+        logic = OT::Logic::DestroyAccount.new sess, cust, req.params, locale
+        logic.raise_concerns
+        logic.process
+        res.redirect app_path('/account')
+      end
+    end
     def update_subdomain
       authenticated('/account') do
         logic = OT::Logic::UpdateSubdomain.new sess, cust, req.params, locale

lib/onetime/app/web/base.rb

@@ -48,7 +48,9 @@ def check_subdomain!
       def check_referrer!
         return if @check_referrer_ran
         @check_referrer_ran = true
-        OT.ld  "[check-referrer] #{req.referrer} (#{req.referrer.class}) - #{req.path}"
+        unless req.referrer.nil?
+          OT.ld("[check-referrer] #{req.referrer} (#{req.referrer.class}) - #{req.path}")
+        end
         return if req.referrer.nil? || req.referrer.match(Onetime.conf[:site][:host])
         sess.referrer ||= req.referrer
       end

lib/onetime/app/web/routes

@@ -29,7 +29,6 @@ GET   /info/security                             Onetime::App::Info#security
 GET   /info/terms                                Onetime::App::Info#terms
 
 GET   /pricing                                   Onetime::App#pricing
-#GET   /business                                  Onetime::App#business_pricing
 GET   /signup                                    Onetime::App#signup
 GET   /signup/:planid                            Onetime::App#signup
 POST  /signup                                    Onetime::App#create_account
@@ -46,7 +45,8 @@ GET   /dashboard                                 Onetime::App#dashboard
 POST  /dashboard                                 Onetime::App#create_secret
 
 GET   /account                                   Onetime::App#account
-POST  /account                                   Onetime::App#update_account
+POST  /account/change_password                   Onetime::App#update_account
+POST  /account/destroy                           Onetime::App#destroy_account
 POST  /account/passgen                           Onetime::App#update_account
 POST  /account/apikey                            Onetime::App#generate_apikey
 POST  /account/subdomain                         Onetime::App#update_subdomain

lib/onetime/logic.rb

@@ -78,26 +78,26 @@ class AuthenticateSession < OT::Logic::Base
       attr_reader :custid, :stay
       attr_reader :session_ttl
       def process_params
-        @custid = params[:u].to_s.downcase.strip
+        @potential_custid = params[:u].to_s.downcase.strip
         @passwd = params[:p]
         #@stay = params[:stay].to_s == "true"
         @stay = true # Keep sessions alive by default
         @session_ttl = (stay ? 30.days : 20.minutes).to_i
-        if @custid.to_s.index(':as:')
-          @colonelname, @custid = *@custid.downcase.split(':as:')
+        if @potential_custid.to_s.index(':as:')
+          @colonelname, @potential_custid = *@potential_custid.downcase.split(':as:')
         else
-          @custid = @custid.downcase if @custid
+          @potential_custid = @potential_custid.downcase if @potential_custid
         end
         if @passwd.to_s.empty?
           @cust = nil
-        elsif @colonelname && OT::Customer.exists?(@colonelname) && OT::Customer.exists?(@custid)
-          OT.info "[login-as-attempt] #{@colonelname} as #{@custid} #{@sess.ipaddress}"
+        elsif @colonelname && OT::Customer.exists?(@colonelname) && OT::Customer.exists?(@potential_custid)
+          OT.info "[login-as-attempt] #{@colonelname} as #{@potential_custid} #{@sess.ipaddress}"
           potential = OT::Customer.load @colonelname
           @colonel = potential if potential.passphrase?(@passwd)
-          @cust = OT::Customer.load @custid if @colonel.role?(:colonel)
+          @cust = OT::Customer.load @potential_custid if @colonel.role?(:colonel)
           sess['authenticated_by'] = @colonel.custid
-          OT.info "[login-as-success] #{@colonelname} as #{@custid} #{@sess.ipaddress}"
-        elsif (potential = OT::Customer.load(@custid))
+          OT.info "[login-as-success] #{@colonelname} as #{@potential_custid} #{@sess.ipaddress}"
+        elsif (potential = OT::Customer.load(@potential_custid))
           @cust = potential if potential.passphrase?(@passwd)
         end
       end
@@ -143,6 +143,7 @@ def process_params
       end
       def raise_concerns
         limit_action :destroy_session
+        OT.info "[destroy-session] #{@custid} #{@sess.ipaddress}"
       end
       def process
         sess.destroy!
@@ -275,7 +276,7 @@ def form_fields
     end
 
     class UpdateAccount < OT::Logic::Base
-      attr_reader :modified, :subdomain
+      attr_reader :modified
       def process_params
         @currentp = params[:currentp].to_s.strip.slice(0,60)
         @newp = params[:newp].to_s.strip.slice(0,60)
@@ -286,10 +287,10 @@ def raise_concerns
         @modified ||= []
         limit_action :update_account
         if ! @currentp.empty?
-          raise_form_error "Current password does not match" unless cust.passphrase?(@currentp)
-          raise_form_error "New passwords do not match" unless @newp == @newp2
+          raise_form_error "Current password is incorrect" unless cust.passphrase?(@currentp)
+          raise_form_error "New password cannot be the same as current password" if @newp == @currentp
           raise_form_error "New password is too short" unless @newp.size >= 6
-          raise_form_error "New password cannot match current password" if @newp == @currentp
+          raise_form_error "New passwords do not match" unless @newp == @newp2
         end
         if ! @passgen_token.empty?
           raise_form_error "Token is too short" if @passgen_token.size < 6
@@ -318,6 +319,82 @@ def form_fields
       end
     end
 
+    class DestroyAccount < OT::Logic::Base
+      attr_reader :raised_concerns_was_called
+
+      def process_params
+        unless params.nil?
+          @currentp = params[:currentp].to_s.strip.slice(0,60)
+        end
+      end
+      def raise_concerns
+        @raised_concerns_was_called = true
+
+        # It's vitally important for the limiter to run prior to any
+        # other concerns. This prevents a malicious user from
+        # attempting to brute force the password.
+        #
+        limit_action :destroy_account
+        if @currentp && @currentp.empty?
+          raise_form_error "Password confirmation is required."
+        else
+          OT.info "[destroy-account] Passphrase check attempt cid/#{cust.custid} r/#{cust.role} ipa/#{sess.ipaddress}"
+
+          unless cust.passphrase?(@currentp)
+            sess.set_info_message "Nothing changed"
+            raise_form_error "Password does not match."
+          end
+        end
+      end
+
+      def process
+        # This is very defensive programming. When it comes to
+        # destroying things though, let's pull out all the stops.
+        unless raised_concerns_was_called
+          raise_form_error "We have concerns about that request."
+        end
+
+        if cust.passphrase?(@currentp)
+          # NOTE: we don't use cust.destroy! here.
+          #
+          # Auto-expire customer record out of redis after
+          # a grace period for the system to take care of any
+          # remaining business to do with the account.
+          #
+          cust.ttl = 24.hours  # auto expire custome
+
+          cust.passphrase = ''
+          cust.regenerate_apitoken
+          cust.verified = false
+          cust.role = 'user_deleted_customer'
+
+          cust.save
+
+          # Log the event immediately after saving the change to
+          # to minimize the chance of the event not being logged.
+          OT.info "[destroy-account] Account destroyed. #{cust.custid} #{cust.role} #{sess.ipaddress}"
+
+          sess.replace!
+          sess.set_info_message 'Account deleted'
+
+        else
+
+          # In theory we should never get here since raise_concerns
+          # should have caught an incorrect password.
+          sess.set_error_message 'Nothing changed'
+
+        end
+
+        sess.set_form_fields form_fields  # for tabindex
+      end
+      def modified? guess
+        modified.member? guess
+      end
+      private
+      def form_fields
+        { :tabindex => params[:tabindex] } unless params.nil?
+      end
+    end
     class GenerateAPIkey < OT::Logic::Base
       attr_reader :apikey
       def process_params
@@ -351,7 +428,7 @@ def process_params
         @maxviews = params[:maxviews].to_i
         @maxviews = 1 if @maxviews < 1
         @maxviews = (plan.options[:maxviews] || 100) if @maxviews > (plan.options[:maxviews] || 100)  # TODO
-         if ['share', 'generate'].member?(params[:kind].to_s)
+        if ['share', 'generate'].member?(params[:kind].to_s)
           @kind = params[:kind].to_s.to_sym
         end
         @secret_value = kind == :share ? params[:secret] : Onetime::Utils.strand(12)

lib/onetime/models.rb

@@ -15,7 +15,9 @@ def incr! identifier, event
       count = lmtr.increment
       lmtr.update_expiration
       OT.ld [:limit, event, identifier, count].inspect
-      raise OT::LimitExceeded.new(identifier, event, count) if exceeded?(event, count)
+      if exceeded?(event, count)
+        raise OT::LimitExceeded.new(identifier, event, count)
+      end
       count
     end
     alias_method :increment!, :incr!
@@ -55,12 +57,13 @@ def has_passphrase?
     end
     def passphrase? guess
       begin
-        ret = !has_passphrase? || BCrypt::Password.new(passphrase) == guess
+        ret = BCrypt::Password.new(passphrase) == guess
         @passphrase_temp = guess if ret  # used to decrypt the value
         ret
       rescue BCrypt::Errors::InvalidHash => ex
-        msg = "[old-passphrase]"
-        !has_passphrase? || (!guess.to_s.empty? && passphrase.to_s.downcase.strip == guess.to_s.downcase.strip)
+        prefix = "[old-passphrase]"
+        OT.ld "#{prefix} Invalid passphrase hash: #{ex.message}"
+        (!guess.to_s.empty? && passphrase.to_s.downcase.strip == guess.to_s.downcase.strip)
       end
     end
   end

lib/onetime/models/session.rb

@@ -72,7 +72,8 @@ def identifier
     @sessid  # Don't call the method
   end
   # Used by the limiter to estimate a unique client. We can't use
-  # the session ID b/c they can choose to not send the cookie.
+  # the session ID b/c the request agent can choose to not send
+  # the cookie (which hash the session ID).
   def external_identifier
     elements = []
     elements << ipaddress || 'UNKNOWNIP'
@@ -82,7 +83,7 @@ def external_identifier
     @external_identifier
   end
   def stale?
-    self[:stale].to_s == "true"
+    self[:stale].to_s == 'true'
   end
   def update_fields hsh={}
     hsh[:sessid] ||= sessid
@@ -98,7 +99,7 @@ def replace!
     @sessid = newid
     # This update is important b/c it ensures that the
     # data gets written to redis.
-    update_fields :stale => false
+    update_fields :stale => 'false'
     sessid
   end
   def shrimp? guess

public/web/css/main.css

@@ -85,10 +85,7 @@
 
 
 
-
-
 #footer {
-
   text-align: center;
   line-height: 90%;
   padding: 0px;
@@ -214,6 +211,8 @@ div.private{font-size: 18px; line-height: 18px; margin-top: 20px;}
 .strikethrough {
   text-decoration: line-through;
 }
+.roomier { padding: 5px; }
+.small { font-size: 90%; }
 .smaller { font-size: 85%; }
 .larger { font-size: 120%; }
 .centre { text-align: center }
@@ -225,6 +224,9 @@ div.private{font-size: 18px; line-height: 18px; margin-top: 20px;}
 .hilite {background:#ff0;}
 .hilite2 {background:#6ff;}
 
+.modal-danger {
+  border: 5px solid #d9534f;
+}
 
 .err, .msg { font-weight: bold; padding: 8px; line-height: 32px; width: 100%; }
 .err { background-color: red; color: #fff }