-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[API] Commenting through a token #1513
Changes from 13 commits
7579f8d
829fae9
a095e8d
0ff8697
195bc07
787134c
a5834f1
d2846ac
917c90e
48ecaae
f80878b
61bca00
e513cdd
1a9c61e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,6 @@ | ||
class CommentController < ApplicationController | ||
include CommentHelper | ||
|
||
respond_to :html, :xml, :json | ||
before_filter :require_user, only: %i[create update delete] | ||
|
||
|
@@ -12,9 +14,11 @@ def index | |
# create node comments | ||
def create | ||
@node = Node.find params[:id] | ||
@comment = @node.add_comment(uid: current_user.uid, body: params[:body]) | ||
if current_user && @comment.save | ||
@comment.notify(current_user) | ||
@body = params[:body] | ||
@user = current_user | ||
|
||
begin | ||
@comment = create_comment(@node, @user, @body) | ||
respond_with do |format| | ||
if params[:type] && params[:type] == 'question' | ||
@answer_id = 0 | ||
|
@@ -30,12 +34,36 @@ def create | |
end | ||
end | ||
end | ||
else | ||
rescue CommentError | ||
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'] | ||
|
||
if @user && @user.token == @token | ||
begin | ||
@comment = create_comment(@node, @user, @body) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, will do. |
||
respond_to do |format| | ||
format.all { render :nothing => true, :status => :created } | ||
end | ||
rescue CommentError | ||
respond_to do |format| | ||
format.all { render :nothing => true, :status => :bad_request } | ||
end | ||
end | ||
else | ||
respond_to do |format| | ||
format.all { render :nothing => true, :status => :unauthorized } | ||
end | ||
end | ||
end | ||
|
||
# create answer comments | ||
def answer_create | ||
@answer_id = params[:aid] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
module CommentHelper | ||
class CommentError < ArgumentError | ||
end | ||
|
||
def create_comment(node, user, body) | ||
@comment = node.add_comment(uid: user.uid, body: body) | ||
if user && @comment.save | ||
@comment.notify user | ||
return @comment | ||
else | ||
raise CommentError.new() | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'd prefer this to be more general -- anyone developing an API use -- could we do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely. This was a placeholder anyway, because I realized that there would be a few issues with the |
||
|
||
#Search RESTful endpoints | ||
#constraints(subdomain: 'api') do | ||
mount Srch::API => '/api' | ||
mount GrapeSwaggerRails::Engine => '/api/d1ocs' | ||
#end | ||
|
||
|
||
resources :rusers | ||
resources :user_sessions | ||
resources :images | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the advantages of this over passing the token in a POST req?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
should we use it instead?