1
\$\begingroup\$

I am making a library which can use different auth mechanisms. I have abstracted the code within an interface:

interface Authenticable
{
    public function getName(): string;

    /**
     * Based on auth type, return the right format of credentials to be sent to the server
     *
     * @param Authenticable $credentials
     * @return mixed
     */
    public function encodedCredentials(): string;
}

One of the possible methods to use this is Plain.php:

class Plain implements Authenticable
{
    protected $name = 'PLAIN';
    protected $options;

    public function __construct(Options $options)
    {
        $this->options = $options;
    }

    public function encodedCredentials(): string
    {
        $credentials = "\x00{$this->options->getUsername()}\x00{$this->options->getPassword()}";
        return XML::quote(base64_encode($credentials));
    }

    public function getName(): string
    {
        return $this->name;
    }
}

And when initializing auth methods I am using this part of code

public function getAuthType()
{
    if (!$this->authType)
        $this->authType = new Plain($this->options);

    return $this->authType;
}

which is initializing Plain if no other method was previously given.

What I am wondering here is: should I forward $username and $password to Plain so that I could decouple it from Options class (since those are the only two fields needed in the class)?

On the other hand, that is what Options class is all about. Besides username and pass, it also holds host, port, logger and resource setters and getters, and few helper methods

\$\endgroup\$
  • 1
    \$\begingroup\$ There is an argument to be made for both. For me, it really depends if your Options will always be easily accessible or not. Otherwise it looks good. The only thing I would add is there may be room for an Abstract class ( probably keep the interface too ). For things like public function getName() etc. Which may need to be duplicated in all children etc... It never hurts to have an interface even it you have an Abstract class, sometimes you can do away with it, if you know you won't need to extend something other then your base class etc. \$\endgroup\$ – ArtisticPhoenix Mar 27 '19 at 19:16
  • \$\begingroup\$ The Abstract class solution sounds great! I have implemented it and removed both the constructor and getter from implementing classes. Much cleaner \$\endgroup\$ – Norgul Mar 28 '19 at 8:31
  • \$\begingroup\$ @ArtisticPhoenix Please add an answer instead of a comment. Refer to the section When shouldn't I comment? on Comment everywhere. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jun 18 '19 at 22:02
  • \$\begingroup\$ @SᴀᴍOnᴇᴌᴀ - I was thinking it was to opinion based but, I suppose "code review" is a bit more flexible with that than SO. As I said I can see an argument for either one. \$\endgroup\$ – ArtisticPhoenix Jun 19 '19 at 6:02
1
\$\begingroup\$

I would lay it out something like this:

interface AuthenticableInterface
{
    public function getName(): string;

    /**
     * Based on auth type, return the right format of credentials to be sent to the server
     *
     * @param Authenticable $credentials
     * @return mixed
     */
    public function encodedCredentials(): string;
}

class AbstractAuthenticable implements AuthenticableInterface
{
    protected $name = 'PLAIN';
    protected $options;

    public function __construct(Options $options)
    {
        $this->options = $options;
    }

    abstract public function encodedCredentials(): string;

    public function getName(): string
    {
        return $this->name;
    }
}

class Plain extends AbstractAuthenticable{

    public function encodedCredentials(): string
    {
        $credentials = "\x00{$this->options->getUsername()}\x00{$this->options->getPassword()}";
        return XML::quote(base64_encode($credentials));
    }

}

I kept the interface, a lot of people get confused between using abstract and interface. I should add that the interface is inherited through the extending of the abstract class, so there is no need to declare (implement) it again.

Interfaces basically just outline the classes API (application program interface, or I like to say public interface), Abstract classes do the same thing (basically) as an interface but you can have concrete implementation of common functionality within the class. Interface doesn't allow you to implement any functionality.

By keeping the interface you could in theory create a Authenticable that uses a base other then the Abstract class. This is useful especially when type casting.

class foo{
     public function __construct(AuthenticableInterface $Authenticable){
          //...
     }
}

So instead of hinting the Abstract class or the Concrete class you can type hint the interface which can be satisfied even when using a completely different base class. In this case it may not be strictly necessary to carry the interface but it does give you that little bit of extra flexibility for the hierarchy.

A good example of the above would be to make a token based login for something like REST where you don't have a username and password, and so that class could be quite different then the base class that uses a user and a password.

Another example I can think of is if you have a hierarchy in your user system, such as a Parent and Subusers, or Admins and non-admins. You may want something like a passwordless Login As functionality that relies more on the user's role and relationship to decide if it should authenticate. Something like a button you just click to login as one of your associated subusers etc. We use subusers a lot in our user systems.

Rather or not you should decouple the username and password depends really on how you want to pass that data. Obviously username and password are going to be really important so it may be worth adding them as function arguments. But as I mentioned above you may create authentication that does not use a username and password.

I would say the biggest argument for it is, then the password and username is not "hidden" or buried in some kind of BLOB or array data. But on the same token (pun intended) you may have some classes that don't need them. So the biggest disadvantage is then requiring it as part of the class, which may break the single interface design.

For example consider this interface and class:

interface Authenticable
{
    public function setPassword(string $password);
}

class LoginAs
{
     //public function setPassword(string $password);
}

So you can see setting a password for "LoginAs" doesn't really make sense and this would stretch what we can do with our interface. By having them as options they are not exposed to the interface because they are wrapped in another object.

I'll end by saying if you keep them in the options you will want to do some more robust error checking such as this:

public function encodedCredentials(): string
{
    $credentials = "\x00{$this->options->getUsername()}\x00{$this->options->getPassword()}";
    return XML::quote(base64_encode($credentials));
}

I modified your getUsername function in Options. I like to set them up this way.

class options{

     public function getUsername($default=null){
           if(isset($this->username)) return $this->username;

           if(!is_null($default))  return $default;

           throw new OptionException("Option {$username} is not set");
     }  
}

By doing this we can control what we get back when its not set. By setting a default other than null, when default is not set we throw an error. Typically I do this using the magic __call method etc.. Basically I showed this to show how you can implement some error checking on those values. Obviously you could check this in each encodedCredentials() but you can also bake it into the options class so it happens when we try to get an options etc...

Hope that helps!

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.