Remove ability to upload/edit traces from blocked users #4129
Conversation
|
Why should it matter? Because GPS traces do not directly infect our map data. There's no real right or wrong here, it's just a decision we need to make about whether we want/need this power and if we do whether it should be linked to the ability to edit the map or not and who should have the power. Obviously if it's linked to edit blocks then that automatically means that moderators get the power so if, for example, we decide that only administrators should have it then that means it needs to be separate. These are of course all things that should ideally have been discussed before you spent time implementing this... |
|
If you know that it fails rubocop because of the line length rule then increase that limit by the way - we won't accept a change that is failing rubocop. |
AntonKhorev
force-pushed
the
trace-block
branch
from
c0e4a51
to
c22deb6
Compare
|
If we totally don't care about dubious trace uploads, we wouldn't have them blocked through the api. So the power is already there. In general I don't think it should be linked to editing, and the blocks should be customisable. But first trace write blocking should work consistently. I could have removed the api block instead, but this ability has stopped at least some bad trace uploads. |
|
How can an ability that doesn't exist yet have stopped anything? Even if we want the ability to block trace uploads it's not obvious it should be linked to API blocks - maybe it should be separate? |
The API block did.
Eventually it should be. |
|
The API block did what? It didn't block trace uploads did it? |
Presumably it would. An API block applies to all API methods that call the So a user_block will block API access to nodes/ways/relations/changesets and also traces, notes, changeset comments, user preferences, permissions APIs. There's a few other API methods that are not blocked (e.g. map, capabilities, versions, users) I can see the logic in expanding this to cover any API methods that are duplicated in html versions. I view a "User Block" as something to stop a user from doing bad stuff, so if we added e.g. inline tag editing through the website, I would expect it to be blocked too, regardless of implementation details. I suspect we might need to expand UserBlocks themselves at some point, e.g. to have a list of checkboxes for different features, e.g. so that a user can be blocked from uploading traces while still being able to respond to changeset comments, but that's out of scope for now. |
|
I always forget that you can also upload traces via the API so I guess that is a bit inconsistent. |
| can [:account, :go_public], User | ||
|
|
||
| can [:new, :create, :edit, :update, :destroy], Trace if user.blocks.active.empty? | ||
|
|
There was a problem hiding this comment.
I would rearrange these to keep the two Trace lines beside each other.
Also, we need to discuss putting the blocking logic into the ability file - it's not done for the existing blocking logic.
| def deny_html_access_for_current_user(exception) | ||
| user_block = current_user.blocks.active.take | ||
| if exception.action == :new || exception.action == :create | ||
| render_blocked_from_writes user_block, :index unless user_block.nil? |
There was a problem hiding this comment.
It's slightly confusing logic here, since it's detecting if the action is 'new' or 'create' and then calling render_blocked_from_writes with action=index. Two things - it's clearly not the correct action, but also I wouldn't expect any writes to be blocked from an 'index' action in the first place!
I know the intention is to avoid too many translations, but there is hopefully a less convoluted way of achieving the same thing.
There was a problem hiding this comment.
What is the correct action? The user tries to upload a trace, that's not going to work.
action=new shows the upload form, but we want to stop the user before the potentially long upload happens, so we redirect away from the form.
Another approach would have been disabling the 'upload a trace' button and telling the user that they're blocked right away on the index page. But then something else needs to be done if the user manages to make the upload request.
| respond_to do |format| | ||
| format.html do | ||
| flash[:warning] = { :partial => "traces/blocked_flash", :locals => { :action => action, :block_link => user_block_path(user_block) } } | ||
| redirect_to :action => action, :display_name => current_user.display_name |
There was a problem hiding this comment.
Is the redirect here just to get the flash message to appear? If so, use flash.now[:warning] to add the message in the current rendering pass.
There was a problem hiding this comment.
It also redirects away from create/edit forms.
AntonKhorev
force-pushed
the
trace-block
branch
from
c22deb6
to
aca819d
Compare
AntonKhorev
force-pushed
the
trace-block
branch
from
aca819d
to
11efe66
Compare
AntonKhorev
added
the
moderation
AntonKhorev
force-pushed
the
trace-block
branch
from
be482dd
to
5163750
Compare
AntonKhorev
force-pushed
the
trace-block
branch
from
5163750
to
38c8159
Compare
AntonKhorev
force-pushed
the
trace-block
branch
from
38c8159
to
a27cbf4
Compare
Currently blocks on users act as api write access blocks. Users can still do any modifications not involving the api. Some modifications can be done both through the api and through the web interface. Uploading and editing traces is one of them.
As a result it's impossible to stop users from uploading questionable traces. And if we want to stop them, why should it matter if their uploads are coming through the api or not? Maybe sometimes it matters, for example, if they're using a faulty app, but that's secondary to their ability to upload at all. Although maybe we should let them delete their traces even when blocked.
In this PR users will get this flash message if they try to write or edit traces:
The button links to the block page.
P.S. may generate rubocop warnings because class line limit is set exactly to the length of
application_controller.rb