319 - Restrict Submitters From Accessing Things They Don't Own #323
Conversation
| resource = FactoryBot.create(model_name_underscored.to_sym) | ||
| configure_user_session(user_role, resource) | ||
| params = params_for(action, resource) | ||
| public_send(method, action, params:) | ||
|
|
There was a problem hiding this comment.
This change allows us to specify the individual publication or submitter profile that we are trying to log in as owners of. It's calling helper functions located in spec/support/helpers/access_authorization.rb to correctly set up the session variables and request parameters
There was a problem hiding this comment.
helper function used to set the session[:submitter_id] to the id of the resource sent.
There was a problem hiding this comment.
changes allow us to set the params for a specific resource rather than just a generically created one. This is needed because of the newly created restrictions where submitters can only change their own work.
Janell-Huyck
changed the title
|
@Janell-Huyck Just to make sure I understand this change correctly, AAEC currently allows any submitter to visit something like http://localhost:3000/artworks/3 and view/edit a publication another submitter added. The same is true for viewing/editing another person's submitter record. Your change blocks access if the current submitter is not the person who created those records, correct? I've verified the bug and verified your change fixes it, but is there anything else this change does that I should verify? |
Oh I see you're also restricting submitter access for certain actions. This all seems to work correctly, but I'd like to talk through some of the code with you on Monday. Easier to chat about than having a conversation in comments. |
Janell-Huyck
force-pushed
the
security-319-restrict-submitter-to-own-items
branch
from
3f0cd81
to
9dec236
Compare
| @resource && (session[:submitter_id].to_s == @resource.submitter_id.to_s) | ||
| end | ||
|
|
||
| def unauthorized_access |
There was a problem hiding this comment.
Suggest making the name an action such as raise_unauthorized_access
| controller_name.classify.constantize | ||
| end | ||
|
|
||
| def authorized_submitter? |
There was a problem hiding this comment.
Suggest a more descriptive name at will help devs know why a submitter is authorized. E.g. current_submitter_is_creator?
| return | ||
| end | ||
|
|
||
| set_resource |
There was a problem hiding this comment.
Perhaps we can call set_resource from within authorized_submitter? since that's the only place making use of @resource. Might not even need a separate method for it.
| %w[errors pages].include? controller_name | ||
| end | ||
|
|
||
| def handle_submitter_special_case |
There was a problem hiding this comment.
"special_case" is vague. Seems like this method should just return true if submitter_logged_in?. Move the call to unauthorized access outside of this method.
| unauthorized_access unless authorized_submitter? | ||
| end | ||
|
|
||
| def non_submitter_controller? |
There was a problem hiding this comment.
Avoid "non" because negatives are hard to follow mentally.
Janell-Huyck
force-pushed
the
security-319-restrict-submitter-to-own-items
branch
from
9dec236
to
55eb408
Compare
Janell-Huyck
changed the title
Janell-Huyck
changed the title
There was a problem hiding this comment.
@Janell-Huyck You call user_is_admin? but that method isn't defined.
|
Janell-Huyck
force-pushed
the
security-319-restrict-submitter-to-own-items
branch
from
5d14e45
to
5d04947
Compare
…ailing restrict_submitter_access tests previously not being tested because of incorrect file naming
Janell-Huyck
force-pushed
the
security-319-restrict-submitter-to-own-items
branch
from
5d04947
to
c2790b1
Compare
|
We had a flaky feature test. I've added in a line that makes the test wait for a page to load, and I expect this to clear up the problem. |
There was a problem hiding this comment.
@Janell-Huyck Looks great. Thanks for all the work you put into this. merging
Fixes #319
Restricts submitters from viewing or accessing publications or submitter profiles that they did not create.
Core Changes
RestrictSubmitterAccessthat checks if a submitter is the creator of a publication (or the submitter profile), and returns a 404 error if they are not.Additional Changes:
Correct HTTP status of failed creation and update of colleges and submitters to be
status: :unprocessable_entity