soft-delete access_tokens and notification_endpoints

refs #COMMS-867

Change-Id: Ib28258633a539bf44c8d2575877ac182dfe24598
Reviewed-on: https://gerrit.instructure.com/141493
Tested-by: Jenkins
Reviewed-by: Steven Burnett <sburnett@instructure.com>
Product-Review: James Williams  <jamesw@instructure.com>
QA-Review: James Williams  <jamesw@instructure.com>

app/models/access_token.rb

@@ -16,6 +16,13 @@
 # with this program. If not, see <http://www.gnu.org/licenses/>.
 
 class AccessToken < ActiveRecord::Base
+  include Workflow
+
+  workflow do
+    state :active
+    state :deleted
+  end
+
   attr_reader :full_token
   attr_reader :plaintext_refresh_token
   belongs_to :developer_key, counter_cache: :access_token_count
@@ -25,7 +32,7 @@ class AccessToken < ActiveRecord::Base
   serialize :scopes, Array
   validate :must_only_include_valid_scopes
 
-  has_many :notification_endpoints, dependent: :destroy
+  has_many :notification_endpoints, -> { where(:workflow_state => "active") }, dependent: :destroy
 
   before_validation -> { self.developer_key ||= DeveloperKey.default }
 
@@ -34,7 +41,8 @@ class AccessToken < ActiveRecord::Base
   # on the scope defined in the auth process (scope has not
   # yet been implemented)
 
-  scope :active, -> { where("expires_at IS NULL OR expires_at>?", DateTime.now.utc) }
+  scope :active, -> { not_deleted.where("expires_at IS NULL OR expires_at>?", DateTime.now.utc) }
+  scope :not_deleted, -> { where(:workflow_state => "active") }
 
   TOKEN_SIZE = 64
   OAUTH2_SCOPE_NAMESPACE = '/auth/'
@@ -43,11 +51,18 @@ class AccessToken < ActiveRecord::Base
   before_create :generate_token
   before_create :generate_refresh_token
 
+  alias_method :destroy_permanently!, :destroy
+  def destroy
+    return true if deleted?
+    self.workflow_state = 'deleted'
+    run_callbacks(:destroy) { save! }
+  end
+
   def self.authenticate(token_string, token_key = :crypted_token)
     # hash the user supplied token with all of our known keys
     # attempt to find a token that matches one of the hashes
     hashed_tokens = all_hashed_tokens(token_string)
-    token = self.where(token_key => hashed_tokens).first
+    token = self.not_deleted.where(token_key => hashed_tokens).first
     if token && token.send(token_key) != hashed_tokens.first
       # we found the token but, its hashed using an old key. save the updated hash
       token.send("#{token_key}=", hashed_tokens.first)

app/models/developer_key.rb

@@ -26,7 +26,7 @@ class DeveloperKey < ActiveRecord::Base
   belongs_to :account
 
   has_many :page_views
-  has_many :access_tokens
+  has_many :access_tokens, -> { where(:workflow_state => "active") }
 
   has_one :tool_consumer_profile, :class_name => 'Lti::ToolConsumerProfile'
 

app/models/notification_endpoint.rb

@@ -19,6 +19,8 @@
 require 'aws-sdk-sns'
 
 class NotificationEndpoint < ActiveRecord::Base
+  include Canvas::SoftDeletable
+
   belongs_to :access_token
 
   validates_presence_of :token, :access_token

app/models/user.rb

@@ -80,7 +80,7 @@ class User < ActiveRecord::Base
   has_many :associated_accounts, -> { order("user_account_associations.depth") }, source: :account, through: :user_account_associations
   has_many :associated_root_accounts, -> { order("user_account_associations.depth").where(accounts: { parent_account_id: nil }) }, source: :account, through: :user_account_associations
   has_many :developer_keys
-  has_many :access_tokens, -> { preload(:developer_key) }
+  has_many :access_tokens, -> { where(:workflow_state => "active").preload(:developer_key) }
   has_many :notification_endpoints, :through => :access_tokens
   has_many :context_external_tools, -> { order(:name) }, as: :context, inverse_of: :context, dependent: :destroy
   has_many :lti_results, inverse_of: :user, class_name: 'Lti::Result'

db/migrate/20180222152531_add_workflow_states_to_tokens_and_endpoints.rb

@@ -0,0 +1,23 @@
+class AddWorkflowStatesToTokensAndEndpoints < ActiveRecord::Migration[5.0]
+  tag :predeploy
+  disable_ddl_transaction!
+
+  def up
+    add_column :access_tokens, :workflow_state, :string
+    change_column_default(:access_tokens, :workflow_state, 'active')
+    DataFixup::BackfillNulls.run(AccessToken, :workflow_state, default_value: 'active')
+    change_column_null(:access_tokens, :workflow_state, false)
+    add_index :access_tokens, :workflow_state, algorithm: :concurrently
+
+    add_column :notification_endpoints, :workflow_state, :string
+    change_column_default(:notification_endpoints, :workflow_state, 'active')
+    DataFixup::BackfillNulls.run(NotificationEndpoint, :workflow_state, default_value: 'active')
+    change_column_null(:notification_endpoints, :workflow_state, false)
+    add_index :notification_endpoints, :workflow_state, algorithm: :concurrently
+  end
+
+  def down
+    remove_column :access_tokens, :workflow_state
+    remove_column :notification_endpoints, :workflow_state
+  end
+end

spec/controllers/oauth2_provider_controller_spec.rb

@@ -222,7 +222,7 @@ describe Oauth2ProviderController do
       allow(Canvas).to receive_messages(:redis => redis)
       post :token, params: {:client_id => key.id, :client_secret => key.api_key, :code => valid_code, :replace_tokens => '1'}
       expect(response).to be_success
-      expect(AccessToken.exists?(old_token.id)).to be(false)
+      expect(AccessToken.not_deleted.exists?(old_token.id)).to be(false)
     end
 
     it 'does not delete existing tokens without replace_tokens' do
@@ -230,7 +230,7 @@ describe Oauth2ProviderController do
       allow(Canvas).to receive_messages(:redis => redis)
       post :token, params: {:client_id => key.id, :client_secret => key.api_key, :code => valid_code}
       expect(response).to be_success
-      expect(AccessToken.exists?(old_token.id)).to be(true)
+      expect(AccessToken.not_deleted.exists?(old_token.id)).to be(true)
     end
 
     context 'grant_type refresh_token' do

spec/controllers/tokens_controller_spec.rb

@@ -81,7 +81,7 @@ describe TokensController do
         expect(token.user_id).to eq @user.id
         delete 'destroy', params: {:id => token.id}
         expect(response).to be_success
-        expect(assigns[:token]).to be_frozen
+        expect(assigns[:token]).to be_deleted
       end
 
       it "should not allow deleting an access token while masquerading" do

spec/lib/canvas/oauth/token_spec.rb

@@ -139,13 +139,13 @@ module Canvas::Oauth
       it 'deletes existing tokens for the same key when requested' do
         old_token = user.access_tokens.create! :developer_key => key
         token.create_access_token_if_needed(true)
-        expect(AccessToken.exists?(old_token.id)).to be(false)
+        expect(AccessToken.not_deleted.where(:id => old_token.id).exists?).to be(false)
       end
 
       it 'does not delete existing tokens for the same key when not requested' do
         old_token = user.access_tokens.create! :developer_key => key
         token.create_access_token_if_needed
-        expect(AccessToken.exists?(old_token.id)).to be(true)
+        expect(AccessToken.not_deleted.where(:id => old_token.id).exists?).to be(true)
       end
     end
 

spec/models/notification_endpoint_spec.rb

@@ -92,4 +92,18 @@ describe NotificationEndpoint do
       ne.destroy
     end
   end
+
+  it "should be soft-deleteable" do
+    ne = @at.notification_endpoints.build(token: 'token', arn: 'arn')
+    ne.save_without_callbacks
+    allow(ne).to receive(:endpoint_exists?).and_return(false)
+    ne.destroy
+    expect(ne.reload.workflow_state).to eq "deleted"
+    expect(@user.notification_endpoints.count).to eq 0
+
+    ne2 = @at.notification_endpoints.build(token: 'token', arn: 'arn')
+    ne2.save_without_callbacks
+    AccessToken.where(:id => @at).update_all(:workflow_state => 'deleted')
+    expect(@user.notification_endpoints.count).to eq 0
+  end
 end

spec/models/notification_failure_processor_spec.rb

@@ -112,8 +112,9 @@ describe NotificationFailureProcessor do
 
       expect(sns_client).to receive(:delete_endpoint).with(endpoint_arn: bad_arn)
       nfp.process
-      expect(NotificationEndpoint.where(arn: good_arn)).not_to be_empty
-      expect(NotificationEndpoint.where(arn: bad_arn)).to be_empty
+      expect(NotificationEndpoint.active.where(arn: good_arn)).not_to be_empty
+      expect(NotificationEndpoint.active.where(arn: bad_arn)).to be_empty
+      expect(NotificationEndpoint.where(arn: bad_arn).first).to be_deleted
     end
 
     it "fails silently when given an invalid message id" do