mysociety / alaveteli

Provide a Freedom of Information request system for your jurisdiction
https://alaveteli.org
Other
389 stars 196 forks source link

Tweak censor rules created by the close and anonymise process so they work in more cases #6617

Closed RichardTaylor closed 1 year ago

RichardTaylor commented 2 years ago

Compare WhatDoTheyKnow censor rules 6289 (set manually by an admin) and 6274 (set automatically by the close and anonymise process) and make it so the one set automatically works as well as the manually set one.

From information available to admins it appears the difference might be the manual one is set to "Regexp:False", and perhaps the automatic ones should have that setting too?

See related support thread for screenshots of the issue.

garethrees commented 2 years ago

The automated censor rule is created without a regexp attribute, leaving it set as nil:

Screenshot 2021-10-29 at 13 57 59
CensorRule.find(6274).regexp
# => nil
CensorRule.find(6289).regexp
# => false

The text to redact is identical:

CensorRule.find(6274).text == CensorRule.find(6289).text
# => true

This shouldn't make a difference – nil and false are both falsey in Ruby. I can't immediately see that we're explicitly checking for regexp? to be false anywhere.

DIffing the attributes shows that the only attribute that could make a difference to the redaction behaviour here is the regexp value:

# Via https://www.thetopsites.net/article/53326717.shtml
Hash.class_eval do
  def difference(other)
    reject do |k,v|
      other.has_key?(k) && other[k] == v
    end
  end
end

pp automated.attributes.difference(manual.attributes)
# => {"id"=>6274,
# =>  "replacement"=>"[Name Removed]",
# =>  "last_edit_editor"=>"User#close_and_anonymise",
# =>  "last_edit_comment"=>"User#close_and_anonymise",
# =>  "created_at"=>Tue, 26 Oct 2021 16:41:52 BST +01:00,
# =>  "updated_at"=>Tue, 26 Oct 2021 16:41:52 BST +01:00,
# =>  "regexp"=>nil}

We could explicitly add regexp: false to the params of User#redact_name!, but it would be better to set a default of false in CensorRule and adding a presence validation. In doing so we'd have to add a cleanup task for existing rules:

CensorRule.where(regexp: nil).count
# => 1412

I don't know that this will fix the described issue, but I can't see what else would be causing the problem.

garethrees commented 1 year ago

We haven't had further reports of this so just closing in favour of https://github.com/mysociety/alaveteli/issues/4922.