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

Don't define unnecessary accessors with ActiveRecord Adapter. #196

Conversation

nagachika
Copy link
Contributor

Without DB connection, ActiveRecord doesn't define methods for columns and AttrEncrypted define attr_reader and attr_writer for the encrypted columns.
As a result, AR model with attr_encrypted column return nil for the target column if the DB connection was lost at the load time.

$ bundle exec rails console
> MyModel.create!(my_column: "deadbeef")
> MyModel.last.my_column
=> "deadbeef"
> exit
(← shutdown database server)
$ bundle exec rails console
> MyModel     # to load target model
MyModel (call MyModel.connection' to establish a connection)
(at this timing invoke database server)
> MyModel.last.my_column
nil

To suppress this behavior, don't define accessors if DB connection was active.
Introduce attribute_instance_methods_as_symbols_available? to detect connection failure with AcriveRecord Adapter.

Without DB connection, ActiveRecord doesn't define methods for columns
and AttrEncrypted define attr_reader and attr_writer for the encrypted
columns.
As a result, AR model with attr_encrypted column return nil for the
target column if the DB connection was lost at the load time.

To suppress this behavior, don't define accessors if DB connection was
active.
Introduce attribute_instance_methods_as_symbols_available? to detect
connection failure with AcriveRecord Adapter.
@jturkel
Copy link

jturkel commented Oct 27, 2017

This would fix the swallowing of DB errors described in #217 which can lead to some pretty nasty issues. Let me know if there's anything I can do to help get this landed.

@nagachika
Copy link
Contributor Author

Hi. Are there any chances this pull request to be merged? I have been using my own forked branch for a while. I'd like to refine the patch if needed. Any comments are welcome.

@saghaulor
Copy link
Contributor

@nagachika thank you for proposing this fix. I think it makes sense. In fact, if we change the test suite slightly, it exposes the problem. Namely, by removing this line https://github.com/attr-encrypted/attr_encrypted/blob/master/test/active_record_test.rb#L45, a number of tests fail and expose the behavior that you describe. Ideally your PR would have a test to prove the behavior and fix it.

It should be noted that another test will fail as well and will require refactoring. https://github.com/attr-encrypted/attr_encrypted/blob/master/test/active_record_test.rb#L337
Namely, because your change rightfully fixes this bug, we can no longer use the IV accessor as it is no longer accidentally defined.

I will use your commit and fix the test suite in another PR. Again, thank you for your help.

@saghaulor saghaulor closed this in a185cab Feb 11, 2018
@saghaulor
Copy link
Contributor

Closed by #294

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.

3 participants