[API] Commenting through a token #1513

ryzokuken · 2017-07-08T17:58:34Z

@jywarren take a look. The function does not do anything yet, but a laid out a possible flow about how it could work. Maybe you had something else in mind? I'd be happy to incorporate your suggestions.

def create_by_token
    # Parse the user's token from the headers

    # Use logic similar to the above create controller but with the user that
    # the token belongs to instead of the `current_user`

    # In order to avoid looking up the entire database for a user having a
    # matching token, we could instead ask the user to pass username/id in the
    # params along with the node id and comment body.
    # This would also mean that if someone tries to bruteforce a token, they
    # would to do so for each user individually.

    # If the username/id and token do not match, send an error in the same
    # format as the request (JSON/XML)
  end

PublicLabBot · 2017-07-08T18:09:02Z

	2 Messages
📖	@ryzokuken Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution!
📖	This pull request doesn’t link to a issue number. Please refer to the issue it fixes (if any) in the body of your PR, in the format: `Fixes #123`.

Generated by 🚫 Danger

jywarren · 2017-07-08T18:13:14Z

Sounds good -- plus a test; i added a token to the bob user fixture for testing. We'd want to handle errors and rejections as well, so that's a second and third test.

If you're creating a whole new method instead of shoehorning it into the existing one, maybe this is best to put into a new API file after all? Your call.

I like the idea of the username, but lookup by token isn't that bad, esp. if we only search users that have status != 0. Your call, i do think it's nice to have few requirements so writing api calls is not a big deal, but i'm agnostic.

ryzokuken · 2017-07-08T18:23:15Z

@jywarren I'm more concerned by the security problem here.

SecureRandom.uuid gives us 16 bytes of entropy, which means that if there's only one "right" token (as in the case of a single user, we have 1/2^16 (~0.00001525878) chance of it being the right one (which in itself is awfully low).

However, if the API accepts any token and looks up the user, the possibility will increase to n/2^16 where n is the number of users with status != 0. Considering a scenario (not very unlikely) where we have 10,000 users, this goes up to 0.1525878 or 15%. That's too bad, don't you think?

EDIT: I made a calculation mistake. I counted 16 bits instead of bytes, which is only 2 bytes of entropy and an awfully bad idea. Instead, on an individual basis, there a 1/2^128 chance instead which translates to approximately 2.9387359e-39, and for 10,000 users, it is 2.9387359e-35 instead which is more like 2.9387359e-33%, so on a second look that might not me THAT bad an idea. However, I see no harm with being a little more cautious.

ryzokuken · 2017-07-08T18:25:08Z

P.S. I added a new function so that we could point the comment_by_token function to a more appropriate URL inside config/routes.rb. (Possibly something like /api/comment. It would be a good idea IMO to group all the API routes together with an /api/ prefix. What do you say?

jywarren · 2017-07-08T18:28:13Z

Ah ok, good point. Sounds good!

…

On Jul 8, 2017 2:25 PM, "Ujjwal Sharma" ***@***.***> wrote: P.S. I added a new function so that we could point the comment_by_token function to a more appropriate URL inside config/routes.rb. (Possibly something like /api/comment. It would be a good idea IMO to group all the API routes together with an /api/ prefix. What do you say? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1513 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ4AAqXl6aplCxroE-StzfphaM9uFks5sL8mEgaJpZM4OR0j4> .

jywarren · 2017-07-08T18:28:43Z

On both counts. On Jul 8, 2017 2:28 PM, "Jeffrey Warren" <[email protected]> wrote: Ah ok, good point. Sounds good!

…

On Jul 8, 2017 2:25 PM, "Ujjwal Sharma" ***@***.***> wrote: P.S. I added a new function so that we could point the comment_by_token function to a more appropriate URL inside config/routes.rb. (Possibly something like /api/comment. It would be a good idea IMO to group all the API routes together with an /api/ prefix. What do you say? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1513 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ4AAqXl6aplCxroE-StzfphaM9uFks5sL8mEgaJpZM4OR0j4> .

ryzokuken · 2017-07-08T19:16:45Z

@jywarren if I have reference to user DrupalUser, how do I retrieve their token? Do I have to add a method for this in app/models/drupal_users.rb?

ryzokuken · 2017-07-08T19:18:17Z

Oh, wait. I could do @user.user.token. Thanks.

jywarren · 2017-07-08T19:24:20Z

But please try to use user instead, if possible. We want to deprecate DrupalUsers soon. Either try to switch over entirely or you can add an adapter method to DrupalUsers aliasing token, so you can do drupal_user.token, see? Thanks!

…

On Jul 8, 2017 3:18 PM, "Ujjwal Sharma" ***@***.***> wrote: Oh, wait. I could do @user.user.token. Thanks. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1513 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ3q8Lux1ykSDwJxxtRXs6ITm5Dhjks5sL9X5gaJpZM4OR0j4> .

ryzokuken · 2017-07-08T19:27:33Z

@jywarren I will. Was using DrupalUsers for the find_by_name function.

jywarren · 2017-07-08T19:29:19Z

Ah, so, User.find_by_username should work!

…

On Jul 8, 2017 3:27 PM, "Ujjwal Sharma" ***@***.***> wrote: @jywarren <https://github.com/jywarren> I will. Was using DrupalUsers for the find_by_name function. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1513 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJx3w77YXvGYbLpjn0FTECQvtG4xLks5sL9glgaJpZM4OR0j4> .

ryzokuken · 2017-07-08T19:29:50Z

Awesome. Will use that instead.

ryzokuken · 2017-07-08T19:41:28Z

@jywarren how does this look? Should I wrap the commenting logic inside the create function into a private function and then call it from both of the functions?

ryzokuken · 2017-07-08T19:52:13Z

Reference: https://web.archive.org/web/20130909062027/http://peepcode.com/blog/2011/rip-ruby-hash-rocket-syntax

ryzokuken · 2017-07-08T20:33:19Z

@jywarren It finally works now, I just forgot the comma inside the hash literal. Could you please guide me regarding the last question I asked so that I could continue working on this?

jywarren · 2017-07-09T00:32:31Z

Sure, a shared private function sounds great.

ryzokuken · 2017-07-09T10:54:13Z

@jywarren started to work on the private function, it is mostly on the lines of the regular function. Hopefully, if its well tested, it will be super easy for me to make just the right function. The problem is, I had trouble understanding this part:

      respond_with do |format|
        if params[:type] && params[:type] == 'question'
          @answer_id = 0
          format.js
        else
          format.html do
            if request.xhr?
              render partial: 'notes/comment', locals: { comment: @comment }
            else
              flash[:notice] = 'Comment posted.'
              redirect_to @node.path + '#last' # to last comment
            end
          end
        end

I would start digging into it, but if you get the time, could you explain what exactly is achieves? I will then add some comments, I guess. That would be great for later contributors.

ryzokuken · 2017-07-09T17:14:36Z

@jywarren Take a look now. If you like it, I would start writing unit tests for it. Hopefully, we could get it merged by today.

ananyo2012 · 2017-07-09T17:39:42Z

app/controllers/comment_controller.rb

+class CommentError < ArgumentError
+end
+
+def create_comment(node, user, body)


This type of logic should go in helpers. Keep the controller code as thin as possible. Only keep action related methods

Okay, If @jywarren approves the function, I would move it to the corresponding helper class.

Maybe look at how we've done error responses in other parts of the code, and compare also with best practices from modern Rails guides?

jywarren · 2017-07-09T18:23:45Z

format.js lets you create a response to a request with format = js, so ending in ".js" in the uRL. Chris is also a way to specify that it's an asynchronous xhr request, most likely from a JavaScript ajax function and not an html form request. So that code is helping respond to different types of requests but giving essentially the same information.

jywarren · 2017-07-09T20:40:33Z

app/controllers/comment_controller.rb

+    if @user && @user.token == @token
+      begin
+        @comment = create_comment(@node, @user, @body)
+        msg = {


I guess this just has a /lot/ of repeated code here -- the only unique line in 52-59 vs 61-68 vs 71-78 is :created and "Created", you know? If you're going through the trouble to a whole new class for CommentError, i just feel that it's not justified given how little this code is reducing repetition. Also, isn't it a bit odd to need to begin/rescue in here?

I'm trying to find a guide to best-practices in Rails controller error handling, to help DRY out this code. But I'm not finding much that's not based on model validation error handling...

I get your point. I used the raise -> rescue workflow here because I call the @comment.save inside the function but wanted the calling function to know that something went wrong and do something sensible, and decided that throwing an error would be the best possible way to achieve this. If you could suggest a better way, I would be more than happy to use it instead, though.

ryzokuken · 2017-07-09T20:42:13Z

@jywarren wrote a basic test. I have no idea why, but the test fails because the response is not :success but 406. Will be going to bed for today, but if you can, please look into this and post a solution if possible. I will try to figure something out myself first thing in the morning, and as soon as I do, I will add the next two tests and hopefully this will be ready to merge.

jywarren · 2017-07-09T20:44:32Z

Re the /api/ endpoint... if you're doing such a thorough rewrite, do you think it's worth using the same URL before we actually try to integrate this with teh Grape/swagger stuff?

If so, i think you can set a more specific (narrower) path higher in the routes.rb file and it'll catch ones that fit the pattern /api/comment/____, for example. The rest should miss that, and default to Swagger.

ryzokuken · 2017-07-09T20:45:38Z

@jywarren got it, will definitely add it higher up.

jywarren · 2017-07-09T20:51:23Z

Maybe you could pass back the output of `.save` and then you'd have the built-in ActiveRecord exceptions to work with?

…

On Sun, Jul 9, 2017 at 4:45 PM, Ujjwal Sharma ***@***.***> wrote: @jywarren <https://github.com/jywarren> got it, will definitely add it higher up. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1513 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ_dsu_TwzGJvxwqxPQr7aL8lSCYdks5sMTvzgaJpZM4OR0j4> .

ryzokuken · 2017-07-10T13:08:15Z

@jywarren I failed to figure out the reason I was getting an HTTP 406 status. I have made a stack overflow question. Here's the link: https://stackoverflow.com/questions/45005178/rails-responds-with-status-code-406

ryzokuken · 2017-07-10T13:55:31Z

@jywarren figured out a solution 🎉 https://stackoverflow.com/questions/45005178/rails-responds-with-status-code-406/45013651#45013651

Will push my changes now and will accept the answer after 2 days.

jywarren · 2017-07-10T14:57:34Z

Ah one more thing -- did you want to display tokens to logged-in users on their profiles, and to admins for all profiles? You could make it show only during a prompt('Copy your token', token) sort of thing if you wanted.

ryzokuken · 2017-07-10T15:00:42Z

@jywarren Yes. Showing the token only when the user explicitly asks for it sounds perfect.

ryzokuken · 2017-07-10T15:02:37Z

@jywarren I was curious. When can @comment.save fail?

ryzokuken · 2017-07-10T15:07:06Z

@jywarren figured it out. Check out the three tests I added. Do we need more?

jywarren

Looking good -- almost there! I think you could include the profile token display in this PR if you like.

jywarren · 2017-07-10T16:30:46Z

app/controllers/comment_controller.rb

+    @node = Node.find params[:id]
+    @user = User.find_by_username params[:username]
+    @body = params[:body]
+    @token = request.env['rack.session']['token']


What are the advantages of this over passing the token in a POST req?

I will look around for the best practices regarding tokens and come back to you. AFAIK, there's an important security benefit of passing the auth token inside the header, but I totally forgot what it was. 😅

https://www.codeschool.com/blog/2014/02/03/token-based-authentication-rails/ says that there's an inbuilt Rails function for dealing Authorization tokens "the right way".

It goes like:

authenticate_or_request_with_http_token do |token, options| # authenticate user... end

should we use it instead?

jywarren · 2017-07-10T16:31:50Z

app/controllers/comment_controller.rb

+
+    if @user && @user.token == @token
+      begin
+        @comment = create_comment(@node, @user, @body)


Maybe add a comment here telling where to find this method? I think for newcomers the Rails organization can be a bit arcane and although it adds a line, this could be really helpful for some.

Sure, will do.

jywarren · 2017-07-10T16:33:31Z

config/routes.rb

@@ -3,13 +3,15 @@
  mount JasmineRails::Engine => '/specs' if defined?(JasmineRails)
  mount JasmineFixtureServer => '/spec/javascripts/fixtures' if defined?(Jasmine::Jquery::Rails::Engine)

+  # Manually written API functions
+  post '/bot/comment/id.:format', to: 'comment#create_by_token'


i'd prefer this to be more general -- anyone developing an API use -- could we do /comment/create/token/:id.:format maybe?

Definitely. This was a placeholder anyway, because I realized that there would be a few issues with the /api endpoint because it's already being used.

jywarren · 2017-07-10T18:07:15Z

let's not - your implementation is clean and readable enough that we needn't worry. I hadn't seen that before and some Rails conventions are more widely adopted than others. Sometimes they're obscure enough to actually make using them harder :-P

…

On Mon, Jul 10, 2017 at 1:16 PM, Ujjwal Sharma ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In app/controllers/comment_controller.rb <#1513 (comment)>: > flash[:error] = 'The comment could not be saved.' render text: 'failure' end end + def create_by_token + @node = Node.find params[:id] + @user = User.find_by_username params[:username] + @Body = params[:body] + @token = request.env['rack.session']['token'] https://www.codeschool.com/blog/2014/02/03/token-based- authentication-rails/ says that there's an inbuilt Rails function for dealing Authorization tokens "the right way". It goes like: authenticate_or_request_with_http_token do |token, options| # authenticate user...end should we use it instead? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1513 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ5VIeOybJRdhWNkaI6aGeOb6DrW0ks5sMlx9gaJpZM4OR0j4> .

ryzokuken · 2017-07-10T18:08:43Z

@jywarren Agreed. I will push a commit for the other two things you pointed out, and hopefully, we'd probably get this show on the road today.

ryzokuken · 2017-07-10T19:55:46Z

@jywarren made the suggested changes. Take a look.

ryzokuken · 2017-07-10T20:56:31Z

@jywarren merge this whenever you're ready, I see you have approved the changes but I do not have access to the repo so I cannot do it myself. 😅

ananyo2012 · 2017-07-11T16:24:02Z

Great work guys!

ryzokuken · 2017-07-11T16:26:08Z

Thanks, @ananyo2012. Hopefully, this will clear the way for a few basic publiclab-related behaviors, especially ones that involve simple commenting.

ananyo2012 · 2017-07-11T16:32:22Z

Yes you can move on with the publiclab/plotsbot#13 after this is merged.

jywarren · 2017-07-11T16:55:05Z

Merging! Thanks -- can you add to /doc/API.md with a sample query?

Start working on a function to add commenting feature through a token

7579f8d

ryzokuken self-assigned this Jul 8, 2017

ryzokuken requested review from jywarren and ananyo2012 July 8, 2017 17:58

Add authorization

829fae9

Avoid ruby hash rocket syntax

a095e8d

Facepalm

0ff8697

Start working on a private function for adding comments

195bc07

ryzokuken added 2 commits July 9, 2017 18:15

Finalize the original function using the private helper

787134c

Make comment_by_token method using the private function

a5834f1

ananyo2012 reviewed Jul 9, 2017

View reviewed changes

jywarren reviewed Jul 9, 2017

View reviewed changes

ryzokuken added 3 commits July 10, 2017 19:31

Fix 406 issue

48ecaae

Add second test and improve responses

f80878b

Fix test names

61bca00

Add test for invalid comment

e513cdd

jywarren requested changes Jul 10, 2017

View reviewed changes

Add recommendations

1a9c61e

jywarren approved these changes Jul 10, 2017

View reviewed changes

ananyo2012 approved these changes Jul 11, 2017

View reviewed changes

jywarren merged commit c74f950 into publiclab:master Jul 11, 2017

ryzokuken added the plotsbot label Aug 23, 2017

jywarren mentioned this pull request Jan 16, 2018

Planning Issue: Expand, Coordinate, and Test API #1449

Closed

21 tasks

[API] Commenting through a token #1513

[API] Commenting through a token #1513

Conversation

ryzokuken commented Jul 8, 2017

PublicLabBot commented Jul 8, 2017 • edited Loading

jywarren commented Jul 8, 2017

ryzokuken commented Jul 8, 2017 • edited Loading

ryzokuken commented Jul 8, 2017

jywarren commented Jul 8, 2017 via email

jywarren commented Jul 8, 2017 via email

ryzokuken commented Jul 8, 2017

ryzokuken commented Jul 8, 2017

jywarren commented Jul 8, 2017 via email

ryzokuken commented Jul 8, 2017

jywarren commented Jul 8, 2017 via email

ryzokuken commented Jul 8, 2017

ryzokuken commented Jul 8, 2017

ryzokuken commented Jul 8, 2017

ryzokuken commented Jul 8, 2017

jywarren commented Jul 9, 2017

ryzokuken commented Jul 9, 2017 • edited Loading

ryzokuken commented Jul 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jywarren commented Jul 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzokuken commented Jul 9, 2017

jywarren commented Jul 9, 2017

ryzokuken commented Jul 9, 2017

jywarren commented Jul 9, 2017 via email

ryzokuken commented Jul 10, 2017

ryzokuken commented Jul 10, 2017

jywarren commented Jul 10, 2017

ryzokuken commented Jul 10, 2017

ryzokuken commented Jul 10, 2017

ryzokuken commented Jul 10, 2017

jywarren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jywarren commented Jul 10, 2017 via email

ryzokuken commented Jul 10, 2017

ryzokuken commented Jul 10, 2017

ryzokuken commented Jul 10, 2017

ananyo2012 commented Jul 11, 2017

ryzokuken commented Jul 11, 2017

ananyo2012 commented Jul 11, 2017

jywarren commented Jul 11, 2017 • edited Loading

PublicLabBot commented Jul 8, 2017 •

edited

Loading

ryzokuken commented Jul 8, 2017 •

edited

Loading

ryzokuken commented Jul 9, 2017 •

edited

Loading

jywarren commented Jul 11, 2017 •

edited

Loading