# Kwicero LTD launches KwiBill Community Edition



## KwiceroLTD (Apr 2, 2015)

Kwicero LTD has launched KwiBill Community Edition.
The community edition comes with everything the paid version does, existing customers are upgraded to priority support for free.

What's under the hood of KwiBill?
As of version 1.2, we use CodeIgniter (https://codeigniter.com) for the base framework, and have written the libraries for KwiBill software to use. 

Version 1.2.1 brings along some new features, like the ability to encrypt and destroy your customer information, with the click of a button.
This features is good in scenarios like you're done at the business you purchased from, now you don't want to risk your account information being stolen - or just don't want automated emails, you simply click the erase button, and poof, your data is encrypted, then deleted from the customer database. The only information this feature does not delete is invoice history, and support ticket history.

Links:
Home
Client Area
Community Forums


----------



## fixidixi (Apr 2, 2015)

My only question is what is the bonus of using this instead of boxbilling for example?


----------



## KwiceroLTD (Apr 2, 2015)

fixidixi said:


> My only question is what is the bonus of using this instead of boxbilling for example?


Well, BoxBilling iirc isn't completely opensourced, last time I checked they used ioncube encoding. What's the bonus? The freedom to expand, modify, etc. to your hearts desire. Additionally, we have some features other panels don't, like a erase feature for customers with no active services, to help protect their information.


----------



## joepie91 (Apr 2, 2015)

So, some of my feedback. Please note that all is meant constructively - I'm not looking to just bash, but most of these are pretty serious/big points that really do need to be addressed.

Also note that I've only grabbed some excerpts from the code - the same issues may exist in other places that I haven't looked. There are more issues, but I have a limited amount of time for writing this post, and these issues are a good place to start. I'll look at the code more once these points are dealt with.

Also, an important bit of advice: *Changing your code is okay.* Code isn't write-once - if something gets messy, it's perfectly okay to rewrite it until it's clean and easy to understand. Even large changes in the internals of your codebase are completely okay - for example, switching to an ORM (this will be explained later in this post) is absolutely worth it in the long run, even if it means you will need to change significant parts of your code.

*License*



KwiceroLTD said:


> Well, BoxBilling iirc isn't completely opensourced,


Like mentioned before, KwiBill - according to the included license.txt, anyway - is not open-source, it just doesn't obfuscate the code (ie. the code is visible). Whether to distribute the 'Community Edition' as open-source or as freeware is, of course, completely up to you, but please make sure that you only call it "open-source" if it actually uses an OSI-compliant license - preferably a pre-existing one rather than a custom one (more info about that here). Otherwise, "freeware" is the correct term.

*Database interaction*

You're using PDO with parameterized queries - that's good. However, consider using an ORM such as DataMapper or any of the other options for CodeIgniter. It will make it much easier to write your database code, and make it much less error-prone.

There are a few snippets like this:


$this->ci->db->query('INSERT INTO `support_tickets`(`id`, `customerid`, `fullname`, `message`, `status`) VALUES (NULL,?,?,?,"1")',array($token,$fullname,$message));
$query = $this->ci->db->query('SELECT * FROM `support_tickets` WHERE `customerid` = ? AND `fullname` = ? AND `message` = ?',array($token,$fullname,$message));
foreach($query->result() as $row)
{
mail($email, 'Ticket Created', 'Ticket Created. To view it please visit: '.base_url().'main/support/viewticket/'.$row->id.'/'.$token);
redirect(base_url().'main/support/viewticket/'.$row->id.'/'.$token);
}

You seem to be trying to get the ID of the just-inserted row, but this is not the correct way of doing that (and it is extremely error-prone). Instead, use lastInsertId.

*Random generation*


$token = rand(10,200);
$this->ci->db->query('INSERT INTO `support_tickets`(`id`, `customerid`, `fullname`, `message`, `status`) VALUES (NULL,?,?,?,"1")',array($token,$fullname,$message));

You're using `rand` for generating a 'secret' token - this is a very bad idea. `rand` and `mt_rand` are not truly random (ie. they're not "cryptographically secure random number generators"), and are relatively easy to guess (which is not something you want for a secret token). Unfortunately, CodeIgniter's random_string method is _also broken_, so that's not a good choice either.

Instead, use the RandomLib library. Make sure to read the README well - you need a medium strength generator.

*Unnecessary code*


if(!is_numeric($id))
{
redirect(base_url().'main/support/'); exit(); // just incase redirect fails
}

You don't need that `exit` statement there - the `redirect` method already does this for you. You should keep your code to the minimum required to make it work - unnecessary code makes it harder to spot real bugs, and harder to understand your code (even for yourself - which can lead to more bugs when you modify your code).

*Poor comments*


// process response
// Show ticket
// Show replies

Most of the comments are along these lines, but they're really not very useful like that.

The golden role of comments is this: *A comment shouldn't describe what you're doing, but why you're doing it.*

What the purpose is of a block of code, should be obvious from the code itself - the function names, the arguments, the structure, and so on. If it isn't obvious from the code what it does, then that just means you need to refactor the code until it is. Dividing code into smaller, reusable chunks is usually a good idea.

*Variable names*


$this->ci->db->query($sql,array($fn,$ln,$ad,$st,$cn,$zp,$ph,$em,$pw));

These kind of variable names are very, very bad. They do not _at all_ describe what they contain, and it's completely unclear what they do. A variable name should always clearly describe what it contains.

Most importantly: *There is never a good reason to abbreviate variable names.* Longer variable names do not make your code faster, or meaningfully smaller. Abbreviating variable names just makes your code harder to read. Don't go Java-style a la AbstractObjectFactoryMethodGenerator, but a name like $email_address is a much better name than $em or $emt.

*Procedural code*

Your code is very procedural - even if it _technically_ uses classes, it doesn't use them correctly, and you end up having a 'monster class' with all the functionality in it. This makes it hard to write understandable code. Again, an ORM should make this a lot easier.

*Client data removal*


if(password_verify($password, $row->password))
{
// valid, user information will be encrypted
$id = $row->id;
$firstname = sha1($row->firstname);
$lastname = sha1($row->lastname);
$address = sha1($row->address);
$city = sha1($row->city);
$country = sha1($row->country);
$zip = sha1($row->zip);
$phone = sha1($row->phone);
$email = sha1($row->email);
$sql = "UPDATE `customerdata` SET `firstname`=?,`lastname`=?,`address`=?,`city`=?,`country`=?,`zip`=?,`phone`=?,`email`=? WHERE `id` = ?";
$this->ci->db->query($sql, array($firstname,$lastname,$address,$city,$country,$zip,$phone,$email,$id));
// Erase user information
$sql = "DELETE FROM `customerdata` WHERE `id` = ?";
$this->ci->db->query($sql, array($id));
$this->ci->session->sess_destroy();
redirect(base_url().'filter/erased');
}
else
{
redirect(base_url().'filter/error');
}

I like the idea of this, but unfortunately the implementation is completely broken:


SHA1 is hashing, not encryption.
Encryption is not useful here (nor is hashing).
You seem to be trying to implement a form of 'secure erase' (ie. overwrite) - but it doesn't work like that. If anything in your stack is journaled - some databases are, and most modern filesystems are - then it won't work _at all_ as the place on disk where your modifications are written to, is an entirely different place from where the original data was.
Unless the database explicitly provides support for 'secure erase', and the filesystem also supports it, you _cannot securely erase customer data._ Just make it a plain DELETE statement and call it a day - that's the best you can do.

Also keep in mind that it's often better to 'blank out' the data and set a 'deleted' column to '1' or something like that, rather than outright removing the records - that way, you don't get broken references from other tables. Also keep in mind that this functionality may not actually be legal in all jurisdictions, for tax reasons.

*Templates*


$content = 'Update your account settings.<br>';
$content .= '<form method="POST" action="account/update"><br>';
$content .= $this->ci->csrf->csrf_html();
$content .= '<input type="text" name="email" value="'.$email.'" class="form-control" name="email" placeholder="Email"><br>';
$content .= '<input type="password" name="password" value="" class="form-control" name="password" placeholder="Password"><

This is everywhere throughout the code, and it's a very bad idea - it's hard to maintain, hard to understand, and very easy to mess up your layout. You should be using a template engine of some sort.

I have no experience with CodeIgniter, so I can't recommend you any particular library, but look for any library that at the very least supports variables, loops and conditionals.

*Email validation*


$emt = $em;
$emt = str_replace('@', '', $emt);
$emt = str_replace('.', '', $emt);
if(ctype_alnum($emt))
{
$sql = "INSERT INTO `customerdata`(`id`, `firstname`, `lastname`, `address`, `city`, `country`, `zip`, `phone`, `email`, `password`) VALUES (NULL, ?, ?, ?, ?, ?, ?, ?, ?, ?)";
$this->ci->db->query($sql,array($fn,$ln,$ad,$st,$cn,$zp,$ph,$em,$pw));
redirect(base_url().'main/login');
} else {
redirect(base_url().'main/register?invalid_entry');
}

This is not the correct way to validate e-mail addresses. Use filter_var instead.


----------



## KwiceroLTD (Apr 2, 2015)

Thanks joepie91, everything was noted. I'm working to make corrections based off what you stated there.

I'm aware the code isn't the greatest, and am working to improve it. As for the license, I'm planning on updating that to a proper one.


----------



## joepie91 (Apr 2, 2015)

Good to hear


----------



## fixidixi (Apr 4, 2015)

KwiceroLTD said:


> Well, BoxBilling iirc isn't completely opensourced, last time I checked they used ioncube encoding. What's the bonus? The freedom to expand, modify, etc. to your hearts desire. Additionally, we have some features other panels don't, like a erase feature for customers with no active services, to help protect their information.


https://github.com/boxbilling/BoxBilling

just sayin


----------

