Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/abilities/ability.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ def initialize(user)
can [:index, :permalink, :edit, :help, :fixthemap, :offline, :export, :about, :communities, :preview, :copyright, :id], :site
can [:create, :show], :export
can [:create, :read], :search
can [:create, :show], :auth_deletion

if Settings.status != "database_offline"
can [:read, :feed], Changeset
Expand Down
52 changes: 52 additions & 0 deletions app/controllers/accounts/auth_deletions_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
# frozen_string_literal: true

module Accounts
class AuthDeletionsController < ApplicationController
layout :site_layout

before_action :set_locale

authorize_resource :class => :auth_deletion

def show
@auth_provider = params.expect(:provider)
@auth_uid, @time = Rails
.application
.message_verifier(:social_login_deletion)
.verify(params.expect(:confirmation_code))
rescue ActiveSupport::MessageVerifier::InvalidSignature
head :bad_request
end

def create
if params.expect(:provider) == "facebook"
create_facebook
else
head :not_found
end
Comment on lines +22 to +26

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@pablobm pablobm May 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end

private

def create_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"])

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
end
end
5 changes: 5 additions & 0 deletions app/views/accounts/auth_deletions/create.json.jbuilder
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# frozen_string_literal: true

json.url auth_delete_url(:provider => params[:provider],
:confirmation_code => @confirmation_code)
json.confirmation_code @confirmation_code
7 changes: 7 additions & 0 deletions app/views/accounts/auth_deletions/show.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<% content_for :heading do %>
<h1><%= t ".title" %></h1>
<% end %>

<p>
<%= t ".body", :provider => t("auth.providers.#{@auth_provider}"), :id => @auth_uid, :time => l(Time.at(@time).utc) -%>
</p>
4 changes: 4 additions & 0 deletions config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,10 @@ en:
successfully_declared: "You have successfully declared that you consider your edits to be in the Public Domain."
already_declared: "You have already declared that you consider your edits to be in the Public Domain."
did_not_confirm: "You didn't confirm that you consider your edits to be in the Public Domain."
auth_deletions:
show:
title: "Data deletion request"
body: "Data for %{provider} ID %{id} was removed at %{time} and it was disconnected from the associated OpenStreetMap account."
browse:
deleted_ago_by_html: "Deleted %{time_ago} by %{user}"
edited_ago_by_html: "Edited %{time_ago} by %{user}"
Expand Down
13 changes: 10 additions & 3 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -233,9 +233,16 @@
get "/forgot-password.html", :to => redirect(:path => "/user/forgot-password")

# omniauth
get "/auth/failure" => "users#auth_failure"
match "/auth/:provider/callback" => "users#auth_success", :via => [:get, :post], :as => :auth_success
match "/auth/:provider" => "users#auth", :via => [:post, :patch], :as => :auth
scope "/auth", :as => :auth do
get "/failure" => "users#auth_failure"

scope ":provider" do
match "/callback" => "users#auth_success", :via => [:get, :post], :as => :success
match "" => "users#auth", :via => [:post, :patch]

resource :delete, :only => [:show, :create], :module => "accounts", :controller => "auth_deletions"
end
end

# permalink
get "/go/:code" => "site#permalink", :code => /[a-zA-Z0-9_@~]+[=-]*/, :as => :permalink
Expand Down
120 changes: 120 additions & 0 deletions test/controllers/accounts/auth_deletions_controller_test.rb
Comment thread
tomhughes marked this conversation as resolved.
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
# frozen_string_literal: true

require "test_helper"

module Accounts
class AuthDeletionsControllerTest < ActionDispatch::IntegrationTest
##
# test all routes which lead to this controller
def test_routes
assert_routing(
{ :path => "/auth/provider/delete", :method => :get },
{ :controller => "accounts/auth_deletions", :action => "show", :provider => "provider" }
)
assert_routing(
{ :path => "/auth/provider/delete", :method => :post },
{ :controller => "accounts/auth_deletions", :action => "create", :provider => "provider" }
)
end

##
# test showing status of a facebook deletion request
def test_show_facebook
user = create(:user, :auth_provider => "facebook", :auth_uid => "12345")
confirmation_code = Rails.application.message_verifier(:social_login_deletion).generate([user.auth_uid, Time.now.to_i])

get auth_delete_path(:provider => "facebook", :confirmation_code => confirmation_code)
assert_response :success
assert_select "p", /^Data for Facebook ID 12345 was removed at .* and it was disconnected from the associated OpenStreetMap account\.$/
end

##
# test showing status of a facebook deletion request with no code
def test_show_facebook_no_code
get auth_delete_path(:provider => "facebook")
assert_response :bad_request
end

##
# test showing status of a facebook deletion request with an invalid code
def test_show_facebook_invalid_code
get auth_delete_path(:provider => "facebook", :confirmation_code => "invalid")
assert_response :bad_request
end

##
# test creation of a facebook deletion request
def test_create_facebook
user = create(:user, :auth_provider => "facebook", :auth_uid => "12345")

payload = Base64.urlsafe_encode64(
JSON.generate(
:algorithm => "HMAC-SHA256",
:expires => Time.now.to_i + 3600,
:issued_at => Time.now.to_i,
:user_id => "12345"
)
)
signature = OpenSSL::HMAC.digest("SHA256", Settings.facebook_auth_secret, payload)
encoded_signature = Base64.urlsafe_encode64(signature)
signed_request = [encoded_signature, payload].join(".")

post auth_delete_path(:provider => "facebook"), :params => { :signed_request => signed_request }
assert_response :success
js = ActiveSupport::JSON.decode(@response.body)
assert_not_nil js
assert_not_nil js["confirmation_code"]
assert_equal auth_delete_url(:provider => "facebook", :confirmation_code => js["confirmation_code"]), js["url"]

assert_nil user.reload.auth_provider
assert_nil user.reload.auth_uid
end

##
# test creation of a facebook deletion request with an invalid signature
def test_create_facebook_bad_signature
create(:user, :auth_provider => "facebook", :auth_uid => "12345")

payload = Base64.urlsafe_encode64(
JSON.generate(
:algorithm => "HMAC-SHA256",
:expires => Time.now.to_i + 3600,
:issued_at => Time.now.to_i,
:user_id => "12345"
)
)
signature = OpenSSL::HMAC.digest("SHA256", "invalid secret", payload)
encoded_signature = Base64.urlsafe_encode64(signature)
signed_request = [encoded_signature, payload].join(".")

post auth_delete_path(:provider => "facebook"), :params => { :signed_request => signed_request }
assert_response :bad_request
end

##
# test creation of a facebook deletion request for an unassociated ID
def test_create_facebook_not_associated
payload = Base64.urlsafe_encode64(
JSON.generate(
:algorithm => "HMAC-SHA256",
:expires => Time.now.to_i + 3600,
:issued_at => Time.now.to_i,
:user_id => "12345"
)
)
signature = OpenSSL::HMAC.digest("SHA256", Settings.facebook_auth_secret, payload)
encoded_signature = Base64.urlsafe_encode64(signature)
signed_request = [encoded_signature, payload].join(".")

post auth_delete_path(:provider => "facebook"), :params => { :signed_request => signed_request }
assert_response :not_found
end

##
# test creation of a deletion request for an unsupported provider
def test_create_unsupported
post auth_delete_path(:provider => "unsupported")
assert_response :not_found
end
end
end