Facebook deletion callback

[tomhughes]

This is an attempt to implement a facebook data deletion callback.

Currently every so often (it used to be every few months but has now reduced to once a week or so) we will get an email from Facebook asking us to download a list of IDs for users that have asked to delete their data and apply those deletions and doing that manually is obviously rather tedious.

The alternative is to implement an HTTP endpoint as described in that document, which is what this tries to do.

There are a number of obvious questions, starting with what data exactly do they expect us to delete? As we only use the name and email to populate defaults on the signup form which are then confirmed directly to us by the user I've take it to mean just the facebook ID that links the accounts.

The second complication is that the whole things seems to assume that you'll have to do the deletion as a background job so you need to provide a URL by return that the user can use to monitor the process. I've chose to just return a URL that says done but with a signed copy of the ID and deletion time as a parameter so we can say what we did and when.

I'm sure there's probably improvements that could be made so I'm open to all suggestions!

[pablobm]

• If this doesn't work correctly, will we know? Will you keep receiving the emails, etc? Making sure that we don't leave accounts in limbo.
• Just deleting the Facebook id to sever the connection sounds reasonable to me. However: should the LWG be consulted on this?
• I like how this keeps the burden of storage on Facebook's side. I didn't know about message_verifier

[pablobm]

For readability, how about avoiding the multiple nesting?

Suggested change

	if params.expect(:provider) == "facebook"
	encoded_signature, payload = params.expect(:signed_request).split(".", 2)
	signature = Base64.urlsafe_decode64(encoded_signature)
	if signature == OpenSSL::HMAC.digest("SHA256", Settings.facebook_auth_secret, payload)
	data = JSON.parse(Base64.urlsafe_decode64(payload))
	user = User.find_by(:auth_provider => "facebook", :auth_uid => data["user_id"])
	if user
	user.auth_provider = nil
	user.auth_uid = nil
	user.save!
	@confirmation_code = Rails
	.application
	.message_verifier(:social_login_deletion)
	.generate([data["user_id"], Time.now.to_i])
	render :formats => [:json]
	else
	head :not_found
	end
	else
	head :bad_request
	end
	else
	head :not_found
	end
	if params.expect(:provider) != "facebook"
	head :not_found
	return
	end
	encoded_signature, payload = params.expect(:signed_request).split(".", 2)
	signature = Base64.urlsafe_decode64(encoded_signature)
	if signature != OpenSSL::HMAC.digest("SHA256", Settings.facebook_auth_secret, payload)
	head :bad_request
	return
	end
	data = JSON.parse(Base64.urlsafe_decode64(payload))
	user = User.find_by(:auth_provider => "facebook", :auth_uid => data["user_id"])
	unless user
	head :not_found
	return
	end
	user.auth_provider = nil
	user.auth_uid = nil
	user.save!
	@confirmation_code = Rails
	.application
	.message_verifier(:social_login_deletion)
	.generate([data["user_id"], Time.now.to_i])
	render :formats => [:json]
	end

The multiple returns are not great, but I think there's more gained than lost here.

[tomhughes]

I'll have to think about that one... I'm not really a fan of early/multiple returns...

[pablobm]

Alternatively could be done with exceptions:

Suggested change

	if params.expect(:provider) == "facebook"
	encoded_signature, payload = params.expect(:signed_request).split(".", 2)
	signature = Base64.urlsafe_decode64(encoded_signature)
	if signature == OpenSSL::HMAC.digest("SHA256", Settings.facebook_auth_secret, payload)
	data = JSON.parse(Base64.urlsafe_decode64(payload))
	user = User.find_by(:auth_provider => "facebook", :auth_uid => data["user_id"])
	if user
	user.auth_provider = nil
	user.auth_uid = nil
	user.save!
	@confirmation_code = Rails
	.application
	.message_verifier(:social_login_deletion)
	.generate([data["user_id"], Time.now.to_i])
	render :formats => [:json]
	else
	head :not_found
	end
	else
	head :bad_request
	end
	else
	head :not_found
	end
	raise ActionController::RoutingError, "Unknown provider" unless params.expect(:provider) == "facebook"
	encoded_signature, payload = params.expect(:signed_request).split(".", 2)
	signature = Base64.urlsafe_decode64(encoded_signature)
	raise ActionController::BadRequest unless signature == OpenSSL::HMAC.digest("SHA256", Settings.facebook_auth_secret, payload)
	data = JSON.parse(Base64.urlsafe_decode64(payload))
	user = User.find_by(:auth_provider => "facebook", :auth_uid => data["user_id"])
	raise ActionController::RoutingError, "No matching user" unless user
	user.auth_provider = nil
	user.auth_uid = nil
	user.save!
	@confirmation_code = Rails
	.application
	.message_verifier(:social_login_deletion)
	.generate([data["user_id"], Time.now.to_i])
	render :formats => [:json]

Which brings another reason to test the "no facebook" case: turns out that ActionController::RoutingError requires a message, and I only noticed because the second instance was being tested (but not the first one).

[pablobm]

Full list of supported exceptions in actionpack:/lib/action_dispatch/middleware/exception_wrapper.rb. Custom ones probably can be added (I don't remember from the top of my head).

[tomhughes]

I've reworked it to be more exception based though ActionController::BadRequest is the only one I'm raising explicitly, and I'm a bit nervous about that to be honest as I'm not sure those exceptions are intended for external use as they're not documented at all.

Possibly we should define our own exception instead?

[pablobm]

I like this 👍 I should have thought of using find_by!.

Yeah, those exceptions sit in a weird place where they are not well documented, but I think the community tends to know about them. (My perception; might be wrong). Having said that... looks like this is a bit better documented now, I just found this in the Rails guides: https://guides.rubyonrails.org/configuring.html#config-action-dispatch-rescue-responses

So I think this is ok, but if you think it'd be clearer to be more explicit with a custom exception (either adding it to the list or having a custom rescue_from), then that works for me too.

[tomhughes]

If this doesn't work correctly, will we know? Will you keep receiving the emails, etc? Making sure that we don't leave accounts in limbo.

I assume they'll yell at us and/or keep telling me to download the CSV and process them manually.