Skip to content
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

Anonymise customer first and last names #12862

Merged
merged 1 commit into from
Nov 28, 2024

Conversation

dacook
Copy link
Member

@dacook dacook commented Sep 16, 2024

These were added a couple of years ago in #8763 But I guess we never noticed the names weren't getting anonymised.

Now they will be anonymised whenever we run the rake task ofn:data:anonymize. This task is used when refreshing staging servers.

The old 'name' field is still in the DB. It was kept for compatibility during migraiton but never cleaned up. I've added the tech debt task to the welcome new devs board now: #8835

What should we test?

Run the script and check that customer names can not be viewed in the system.

This must be executed on uk_staging because it was recently refreshed from prod and the customer names will still be present. IE:

  1. First, add the pr-staged-uk label and wait for the PR to be deployed.
  2. While waiting, check that customer names are still visible in the admin interface
  3. Once staged, execute the rake task:
    ssh openfoodnetwork@staging.openfoodnetwork.org.uk
    cd apps/openfoodnetwork/current
    RAILS_ENV=staging bundle exec rake ofn:data:anonymize
    
  4. Then the customer names should be masked.

Dependencies

These were added a couple of years ago in openfoodfoundation#8763
But I guess we never noticed the names weren't getting anonymised.

The old 'name' field is still in the DB. It was kept for compatibility during migraiton but never cleaned up. I've added the tech debt task to the welcome new devs board now: openfoodfoundation#8835
@dacook dacook added the technical changes only These pull requests do not contain user facing changes and are grouped in release notes label Sep 16, 2024
@dacook
Copy link
Member Author

dacook commented Sep 16, 2024

Testing on fr_staging

Before: real customer names appeared in Admin > Customers (not shown here for privacy reasons of course).

I ran the updated queries directly and it worked:
Screenshot 2024-09-16 at 11 35 57 AM

irb(main):020:0>       Customer.where(user_id: nil)
irb(main):021:1"         .update_all("email = concat(id, '_ofn_customer@example.com'),
irb(main):022:1"                      name = concat('Customer Number ', id, ' (without connected User)'),
irb(main):023:1"                      first_name = concat('Customer Number ', id),
irb(main):024:1"                      last_name = '(without connected User)'")
=> 12744
irb(main):025:0>       Customer.where.not(user_id: nil)
irb(main):026:1"         .update_all("email = concat(user_id, '_ofn_user@example.com'),
irb(main):027:1"                      name = concat('Customer Number ', id, ' - User ', user_id),
irb(main):028:1"                      first_name = concat('Customer Number ', id),
irb(main):029:1"                      last_name = concat('User ', user_id)")
=> 36829

@filipefurtad0
Copy link
Contributor

Hey @dacook ,

I guess this PR is dependent on completing openfoodfoundation/ofn-install#937, right?
Adding the feedback needed label.

@dacook
Copy link
Member Author

dacook commented Oct 3, 2024

Yes you're right, sorry we can't deploy PRs to the new uk staging server yet.

BTW I just noticed a mistake in the description above, have now fixed that.

@RachL
Copy link
Contributor

RachL commented Oct 15, 2024

@filipefurtad0 @dacook i'm removing the feedback needed label as I think this is unblocked now - correct?

@dacook
Copy link
Member Author

dacook commented Oct 15, 2024

Yes, that's correct thanks.
I've updated the testing notes. It would be a good opportunity for Konrad or Filipe to try this out on the server.

@filipefurtad0 filipefurtad0 self-assigned this Oct 31, 2024
@filipefurtad0 filipefurtad0 added pr-staged-uk staging.openfoodnetwork.org.uk and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Oct 31, 2024
@filipefurtad0
Copy link
Contributor

Not sure this is something with the PR - I've noticed this with other PRs today as well - but staging is failing:
https://openfoodnetwork.slack.com/archives/CEF14NU3V/p1730419014612589

So, I was not able to test this...

@mkllnk
Copy link
Member

mkllnk commented Nov 1, 2024

That's odd. You tried to deploy to uk-staging but it looks like it tried to deploy to all staging servers. And maybe another process was deploying at the same time?

@mkllnk
Copy link
Member

mkllnk commented Nov 1, 2024

The deploy script on uk-staging was wrong. It did deploy to all staging servers at the same time. I fixed it manually. I'll ask in Slack what the cause may be.

@filipefurtad0 filipefurtad0 added the pr-staged-uk staging.openfoodnetwork.org.uk label Nov 22, 2024
@filipefurtad0
Copy link
Contributor

Hey, it seems the staging-UK key has changed so I'm getting:

ffurtado@ff-LAT:~$ ssh openfoodnetwork@staging.openfoodnetwork.org.uk
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
@    WARNING: REMOTE HOST IDENTIFICATION HAS CHANGED!     @
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
IT IS POSSIBLE THAT SOMEONE IS DOING SOMETHING NASTY!
Someone could be eavesdropping on you right now (man-in-the-middle attack)!
It is also possible that a host key has just been changed.
The fingerprint for the ECDSA key sent by the remote host is
SHA256:FFJDp2QrS/plQeQ/cW8wUE0ihYqDdtEPl5hE34Y+cPo.
Please contact your system administrator.
Add correct host key in /home/ffurtado/.ssh/known_hosts to get rid of this message.
Offending ECDSA key in /home/ffurtado/.ssh/known_hosts:3
  remove with:
  ssh-keygen -f "/home/ffurtado/.ssh/known_hosts" -R "staging.openfoodnetwork.org.uk"
Host key for staging.openfoodnetwork.org.uk has changed and you have requested strict checking.
Host key verification failed.

Where can I find the new key? (posted in #devops too, in case someone is online)

@filipefurtad0 filipefurtad0 added feedback-needed and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Nov 22, 2024
@mkllnk
Copy link
Member

mkllnk commented Nov 25, 2024

You don't need to get a new key from somewhere. The host will supply it. You just have to delete the old keys first:

ssh-keygen -f "/home/ffurtado/.ssh/known_hosts" -R "staging.openfoodnetwork.org.uk"

This warning is to prevent you from connecting to a malicious server pretending to be staging-uk. So if we hadn't changed servers then this warning would be worrying. But since we did change servers, we can forget the old key and ssh will automatically use the new key.

@filipefurtad0 filipefurtad0 added pr-staged-uk staging.openfoodnetwork.org.uk and removed feedback-needed labels Nov 28, 2024
@filipefurtad0
Copy link
Contributor

filipefurtad0 commented Nov 28, 2024

Thanks for that hint @mkllnk ,

I've managed to log in to the staging server and run the command.

Before running it:

image

After running the script:

image

I've noticed though, that email accounts were also anonymized, for example, the account I was using was changed as well - I've noticed this on the top right corner:

image

(This means our log in information needs to be updated). I'm not sure this was intended. It's not a major issue, but not mentioned on the description - I was expecting only the customer names to be anonymized. Do you think this is ok to be merged as is @dacook ? Please feel free to, if this is ok.

@filipefurtad0 filipefurtad0 added feedback-needed and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Nov 28, 2024
@mkllnk
Copy link
Member

mkllnk commented Nov 28, 2024

I've noticed though, that email accounts were also anonymized

That's necessary because the email address often contains the person's name as well.

@mkllnk mkllnk merged commit 925ac2e into openfoodfoundation:master Nov 28, 2024
66 of 68 checks passed
@dacook
Copy link
Member Author

dacook commented Dec 1, 2024

Thanks. Perhaps it would be appropriate to skip admin accounts, for convenience. But as we don't frequently refresh staging databases we probably don't need to consider that right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
technical changes only These pull requests do not contain user facing changes and are grouped in release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants