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

190 id number storage #1269

Merged
merged 6 commits into from
Dec 29, 2017
Merged

190 id number storage #1269

merged 6 commits into from
Dec 29, 2017

Conversation

entantoencuanto
Copy link
Member

@entantoencuanto entantoencuanto commented Dec 19, 2017

Closes #190

What does this PR do?

Allows to store an encrypted ID number as a new user verification type. The model makes use of a key identifier which value is expected to be defined in application secrets.

How should this be manually tested?

Define in config/secrets.yml a new secret using RSA public key algorithm for the value. To create a valid key use, for example:

OpenSSL::PKey::RSA.new(2048).public_key.to_pem

You can create a new id number verification calling:

verification = User::Verification::IdNumber.new(encryption_key: key_secret_identifier, id_number: id_number)

Once saved, verification will only store the id_number encrypted with the previously defined key. This value is accessible calling verification.encrypted_id_number

Does this PR changes any configuration file?

Not directly, but to create valid User::Verification::IdNumber instances at least a public key using RSA public key algorithm should be defined in config/secrets.yml and used in encryption_key attribute.

  • new environment variable in .env.example?
  • new entry in config/application.yml?
  • new entry in config/secrets.yml?

(Changes in these files might need to update the role in Ansible)

@entantoencuanto entantoencuanto requested review from danguita and removed request for danguita December 19, 2017 17:37
@codecov-io
Copy link

codecov-io commented Dec 19, 2017

Codecov Report

Merging #1269 into master will increase coverage by 0.06%.
The diff coverage is 97.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1269      +/-   ##
==========================================
+ Coverage   75.79%   75.86%   +0.06%     
==========================================
  Files         455      457       +2     
  Lines       12205    12238      +33     
==========================================
+ Hits         9251     9284      +33     
  Misses       2954     2954
Impacted Files Coverage Δ
app/models/user.rb 100% <100%> (ø) ⬆️
app/models/user/verification.rb 92.3% <100%> (ø) ⬆️
lib/asymmetric_encryptor.rb 100% <100%> (ø)
app/models/user/verification/id_number.rb 95.45% <95.45%> (ø)
...pp/models/user/verification/census_verification.rb 100% <0%> (+4.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8024f24...f861dc3. Read the comment docs.

Copy link
Member

@ferblape ferblape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@entantoencuanto secrets.yml is commited and part of the repository. Sensitive information is stored in the environments file.

So could you update the PR with the following changes?

  • update and commit secrets.yml referencing an environment variable
  • update .env.example with the new variable
  • update the environment variables document in docs/

@ferblape ferblape merged commit f81acb8 into master Dec 29, 2017
@ferblape ferblape deleted the 190-id_number_storage branch December 29, 2017 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants