code review

[Shannon -jj Behrens]

Hi,

I've sent a couple of days code reviewing authlogic. I have a few
corrections and comments:

==

I'm looking at existence.rb:

before_save
new_session? ? before_create : before_update
new_session? ? after_create : after_update
after_save

save_record

It sure does seem like save_record is in the wrong place.

==

In magic_columns.rb:

# def last_request_update_allowed?
# action_name =! "update_session_time_left"
# end

I think it should be "!=".

return controller.last_request_update_allowed? if
controller.responds_to_last_request_update_allowed?

I'm guessing it should be:

controller.respond_to?(:last_request_update_allowed?)

==

In password.rb:

def add_general_credentials_error
errors.add(:base,
I18n.t('error_messages.general_credentials_error', :default =>
"#{login_field.to_s.humanize}/Password combination is not valid"))
end

"Password" is hardcoded, ignoring the password field configuration.

==

There were a bunch of typos in the API docs, but I figured you wouldn't
care too much about those.

==

In logged_in_status.rb:

raise "Can not determine the records login state because there is
no last_request_at column" if !respond_to?(:last_request_at)

"record's"

==

In login.rb:

# merge options into it. Checkout the convenience function
merge_validates_format_of_login_field_options to merge
...
def validates_uniqueness_of_login_field_options(value = nil)

The comment mentions the wrong function.

==

In password.rb:

# merge options into it. Checkout the convenience function
merge_validates_length_of_password_field_options to merge
...
def validates_confirmation_of_password_field_options(value = nil)

The comment mentions the wrong function.

# merge options into it. Checkout the convenience function
merge_validates_length_of_password_field_options to merge
...
def validates_length_of_password_confirmation_field_options(value = nil)

Same thing.

I'm thinking about transition_from_crypto_providers. Do all the crypto
providers return different length strings? Let's say there is a weak crypto
provider, and it returns the same length string as a strong crypto provider.
If the weak crypto provider is listed in transition_from_crypto_providers,
then the attacker can ignore the stronger crypto provider and try to attack
the weak crypto provider. After all, the code doesn't check which crypto
provider was used to create the hash. It only checks if the hash matches, and
it'll check for each of the crypto providers in
transition_from_crypto_providers. Am I making any sense? I guess what I'm
saying is that it would have been safer to store the name of the crypto
provider rather than checking every possibility in
transition_from_crypto_providers.

==

In single_access_token.rb:

save_without_session_maintenance

Usually you pass false to this function.

==

I hope you find this helpful.

Best Regards,
-jj

--
In this life we cannot do great things. We can only do small things
with great love. -- Mother Teresa
http://jjinux.blogspot.com/

[Shannon -jj Behrens]

By the way, I blogged about my experience code reviewing authlogic:
http://jjinux.blogspot.com/2009/08/rails-authlogic-code-review.html