Authorization #13
Authorization #13
Conversation
| @@ -7,23 +7,23 @@ class AnswersController < ApplicationController | |||
|
|
|||
| def create | |||
| @answer = question.answers.create(answer_params.merge(user: current_user)) | |||
| authorize! :create, Answer | |||
There was a problem hiding this comment.
авторизовыывать после того, как объект создан смысла-то уже нет. Авторизация всегда должна идти ДО основнонго действия, которое мы авторизуем. Сейчас, допустим, окажется, что прав на создание нет и что тогда? ответ ведь уже был создан и сохранен в базе, какой смысл проверять после того, как все уже сделано? как говорится, "после драки калаками не машут" ))
| @@ -2,8 +2,16 @@ class ApplicationController < ActionController::Base | |||
| before_action :authenticate_user! | |||
| before_action :gon_params | |||
|
|
|||
| check_authorization unless: :devise_controller? | |||
|
|
|||
| rescue_from CanCan::AccessDenied do |exception| | |||
There was a problem hiding this comment.
тут надо обработку сделать еще и для js и json форматов, т.к. редирект возможен только в html-формате. а в js и json лучше возвращать 403 ошибку и обрабатывать ее уже на клиенте
| path = current_user ? root_path : new_user_session_path | ||
| redirect_to path, alert: exception.message | ||
| end | ||
|
|
||
| def delete_file_attached |
There was a problem hiding this comment.
так, я это, видимо, пропустил. что делает этот экшн в AppicationController? Нужно создать AttachamentsController с экшеном destroy и через него удалять аттачменты.
app/controllers/badges_controller.rb
Outdated
| @@ -3,5 +3,6 @@ class BadgesController < ApplicationController | |||
|
|
|||
| def index | |||
| @badges = current_user.badges | |||
| authorize! :read, @badges | |||
There was a problem hiding this comment.
то же самое: авторизация должна выполняться ДО действия
| @@ -6,6 +6,7 @@ class CommentsController < ApplicationController | |||
|
|
|||
| def create | |||
| @comment = @commentable.comments.new(comment_params.merge(user: current_user)) | |||
| authorize! :create, @comment | |||
There was a problem hiding this comment.
вот здесь нормально, потому что тут new, т.е. объект хоть в паямти и инстанциирован, но еще не сохранен. а в ответах у тебя create, там авторизация должна быть ДО действия. Но на самом деле, для создания, обычно в правила авторизации нужно только класс передавать, поэтому тут тоже можно авторизацию сделать первой
app/controllers/concerns/voted.rb
Outdated
| @@ -2,6 +2,8 @@ module Voted | |||
| extend ActiveSupport::Concern | |||
| included do | |||
| before_action :set_ratable, only: %i[upvote downvote] | |||
|
|
|||
| load_and_authorize_resource | |||
There was a problem hiding this comment.
лучше использовать authorize! в самих экшенах, иначе может быть двойная загрузка
| @@ -1,29 +1,31 @@ | |||
| class QuestionsController < ApplicationController | |||
There was a problem hiding this comment.
тут та же проблема: сначала авторизация, потом действия, никак не наоборот
| @@ -0,0 +1,10 @@ | |||
| class ActiveStorage::AttachmentsController < ApplicationController | |||
There was a problem hiding this comment.
Не обязательно даже неймспейс вводить здесь (тем более в системном неймспейсе создавать свой контроллер). Контроллер - это просто класс с методами (экшенами), в каждом экшене ты можешь написать любой код, который может обращаться к любым моделям, имя контроллера и моделей никак между собой не связаны. Мы называем их похоже чисто для понятности и логичности, но это необязательно.
Здесь достаточно было бы просто AttachmentsController
| def destroy | ||
| @file = ActiveStorage::Attachment.find(params[:id]) | ||
| authorize! :destroy, @file | ||
| if current_user&.author_of?(@file.record) |
There was a problem hiding this comment.
у тебя предыдущая строка должна выполнить эту проверку, в этом и суть авторизации. ранее мы вручную это проверяли, поэтому нужен был вызов метода author_of?, теперь везде эти вызовы надо менять на вызов авторизации.
| path = current_user ? root_path : new_user_session_path | ||
| redirect_to path, alert: exception.message | ||
| end | ||
| format.json { render json: {error: exception.message}, status: :unauthorized } |
There was a problem hiding this comment.
почему в json один статус, а в js другой? здесь в обоих случаях должен быть forbidden (403), т.к. unauthorized (401), несмотря на название, на самом деле означает, что юзер не залогинен.
app/models/ability.rb
Outdated
| can :downvote, [Question, Answer] | ||
| cannot :downvote, [Question, Answer], user_id: user.id | ||
| can :best, Answer, question_id: user.question_ids | ||
| can :destroy, ActiveStorage::Attachment, record_id: user.question_ids |
There was a problem hiding this comment.
вот такая запись очень неоптимальная, ты каждый раз коллекцию передаешь. да и просто по id в полиморфных ассоциациях нельзя четко определить объект, нужен еще и тип. более того, у тебя следующее правило перезапишет это (т.е. они не объединятся, а одно заменит другое) и у тебя сможет юзер удалять аттачменты у объектов, id которых совпадает с id ответов юзера. А ведь это может быть и ответ и вопрос, причем, вопрос может быть чужой.
Тут нужно использовать правило с блоком. В нем можно написать любой код и если блок вернет true, значит, юзер сможет выполнить действие. Внутри блока можно вызвать метод author_of?, например
Реализованы все проверки в проекте на наличие прав (например, такие, что только автор может редактировать вопрос/ответ и другие правила авторизации) у пользователя через CanCan, покрыт тестами класс Ability.