@pablobm commented on this pull request.
- 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`
> +
+ ##
+ # 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(
+ :algortihm => "HMAC-SHA256",
Misspelled in all three instances:
```suggestion
:algorithm => "HMAC-SHA256",
```
> + 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
For readability, how about avoiding the multiple nesting?
```suggestion
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.
On test/controllers/accounts/auth_deletions_controller_test.rb:
Should we have a test for `provider != "facebook"`? Just for completeness.
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/7093#pullrequestreview-4328940574
You are receiving this because you are subscribed to this thread.
Message ID:
<openstreetmap/openstreetmap-website/pull/7093/review/[email protected]>_______________________________________________
rails-dev mailing list
[email protected]
https://lists.openstreetmap.org/listinfo/rails-dev
