Forum: Ruby How can I improve and write more OOP way my username/password validation code

249c7fd851c5c5ac5a1abdb756472ae1?d=identicon&s=25 Arup Rakshit (my-ruby)
on 2014-03-16 13:00
require 'openssl'

class EncryptDecrypt
  attr_reader :secreat_key
  def initialize(usr, pwd)
    @username = usr
    @password = pwd
    @secreat_key ||= {}
  end
  def decrypt(usr, pwd)
    bool = false
    secreat_key.each do |k,v|
      decipher = OpenSSL::Cipher::AES.new(128, :CBC)
      decipher.decrypt
      decipher.key = k
      decipher.iv = v.first
      plain = decipher.update(v.last) + decipher.final
      break bool = true if plain == [usr,pwd].join(',')
    end
    bool
  end
  def encrypt
    data = [@username,@password].join(",")
    cipher = OpenSSL::Cipher::AES.new(128, :CBC)
    cipher.encrypt
    key = cipher.random_key
    iv = cipher.random_iv
    encrypted = cipher.update(data) + cipher.final
    @secreat_key[key] = [iv,encrypted]
  end
  def user_exist?(usrnam,pswd)
    decrypt(usrnam,pswd)
  end
  def user_add
    encrypt
  end

  private :encrypt, :decrypt
end

user1 = EncryptDecrypt.new('arup','xxy')
user1.user_add
user1.user_exist?('arup','xxy') # => true
user1.user_exist?('arup','xx') # => false
4828d528e2e46f7c8160c336eb332836?d=identicon&s=25 Robert Heiler (shevegen)
on 2014-03-16 20:56
Looks ok.

You should add more comments though.

Also perhaps add the << method to add a user.

user << 'Joe'
1ea245dc79ad44fcaa9cbf821a354369?d=identicon&s=25 Michael Uplawski (frog_boche)
on 2014-03-17 08:31
Good morning.

This question is probably easy to answer, as each of us has her/his idea
of 'more'. Some may even have convictions. I just add 'comments',
where I deem them valid. Whether my suggestions are 'improvements'
depends much on what you try to achieve and who will see your
source-code in the future.

> require 'openssl'
>
> class EncryptDecrypt
>   attr_reader :secreat_key
>   def initialize(usr, pwd)
>     @username = usr
>     @password = pwd

This 'assign or not' is useless. You can straight away assign an empty
array as long as no external actor will access the object's *private*
properties. And because the call to 'initialize()' will precede most
interaction with such an object.., I really doubt that possibility. But
as my metaprogramming-skills are rudimentary, there can be objections to
all that I write (or anybody else). The way that some people understand
OOP, it can make sense to keep the 'or' anyway but I doubt that you have
a use for that kind of code, now.

>     @secreat_key ||= {}
>   end
>   def decrypt(usr, pwd)

Name your variables in a way that helps to guess their utility. For
a variable which is local to the code-block of a method, this 'rule' is
certainly of minor importance. However. You should not be obliged to
read all of your code, in a year from now, just because you do not
remember any more...
'bool' does not mean a lot.

>     bool = false
>     secreat_key.each do |k,v|
>       decipher = OpenSSL::Cipher::AES.new(128, :CBC)
>       decipher.decrypt
>       decipher.key = k
>       decipher.iv = v.first
>       plain = decipher.update(v.last) + decipher.final
>       break bool = true if plain == [usr,pwd].join(',')
>     end
>     bool
>   end
>   def encrypt
>     data = [@username,@password].join(",")
>     cipher = OpenSSL::Cipher::AES.new(128, :CBC)
>     cipher.encrypt
>     key = cipher.random_key
>     iv = cipher.random_iv
>     encrypted = cipher.update(data) + cipher.final
>     @secreat_key[key] = [iv,encrypted]
>   end

user_exist? is an alias for decrypt, so:
alias :user_exist? :encrypt

>   def user_exist?(usrnam,pswd)
>     decrypt(usrnam,pswd)
>   end
>   def user_add
>     encrypt
>   end
>
>   private :encrypt, :decrypt

For the alias 'user_exist?, add
    public :user_exist?

> end
>
> user1 = EncryptDecrypt.new('arup','xxy')
> user1.user_add
> user1.user_exist?('arup','xxy') # => true
> user1.user_exist?('arup','xx') # => false
1ea245dc79ad44fcaa9cbf821a354369?d=identicon&s=25 Michael Uplawski (frog_boche)
on 2014-03-17 11:19
SORRY for the typo. The alias-declaration must read:
alias :user_exist? :decrypt
You knew that. Anyway.
Bee69cfed999cd13e3bff73d472a39ee?d=identicon&s=25 Hassan Schroeder (Guest)
on 2014-03-17 17:13
(Received via mailing list)
On Sun, Mar 16, 2014 at 5:00 AM, Arup Rakshit <lists@ruby-forum.com>
wrote:

> class EncryptDecrypt

The class name itself says "violation of the Single Responsibility
Principle". And why would you expect something with that name to
return a "user" ???

> user1 = EncryptDecrypt.new('arup','xxy')

Leaving aside the obvious refactorings of the insides (methods too
long, etc.), I would call this something like a Credential using two
separate classes (Encrypt and Decrypt, though I would probably
also name them as "things" rather than actions, e.g. Encryptor).

credential = Credential.new('arup','xxy')
credential.match?('arup','xxy') # => true

and so on.

Googling 'ruby and oop' will find you lots of material to ponder :-)

HTH,
Please log in before posting. Registration is free and takes only a minute.
Existing account

NEW: Do you have a Google/GoogleMail, Yahoo or Facebook account? No registration required!
Log in with Google account | Log in with Yahoo account | Log in with Facebook account
No account? Register here.