Remove string constructor of X509_Time()

* Break down string representations to to_string() and readable_string()
* Add m_ prefix to member variable names
* Fix order of methods
* Move comments Doxygen friendly to header
* Make set_to() private (future subjejt of refectoring); People should
use constructor

Closes #185

7 files changed, 176 insertions, 228 deletions

diff --git a/src/lib/asn1/asn1_time.cpp b/src/lib/asn1/asn1_time.cpp
index 72bf87df9..f6aceeefb 100644
--- a/src/lib/asn1/asn1_time.cpp
+++ b/src/lib/asn1/asn1_time.cpp
@@ -16,155 +16,37 @@
 
 namespace Botan {
 
-/*
-* Create an X509_Time
-*/
-X509_Time::X509_Time(const std::string& time_str)
-   {
-   set_to(time_str);
-   }
-
-/*
-* Create a X509_Time from a time point
-*/
 X509_Time::X509_Time(const std::chrono::system_clock::time_point& time)
    {
    calendar_point cal = calendar_value(time);
 
-   year   = cal.year;
-   month  = cal.month;
-   day    = cal.day;
-   hour   = cal.hour;
-   minute = cal.minutes;
-   second = cal.seconds;
+   m_year   = cal.year;
+   m_month  = cal.month;
+   m_day    = cal.day;
+   m_hour   = cal.hour;
+   m_minute = cal.minutes;
+   m_second = cal.seconds;
 
-   tag = (year >= 2050) ? GENERALIZED_TIME : UTC_TIME;
+   m_tag = (m_year >= 2050) ? GENERALIZED_TIME : UTC_TIME;
    }
 
-/*
-* Create an X509_Time
-*/
-X509_Time::X509_Time(const std::string& t_spec, ASN1_Tag t) : tag(t)
+X509_Time::X509_Time(const std::string& t_spec, ASN1_Tag tag)
    {
    set_to(t_spec, tag);
    }
 
-/*
-* Set the time with a human readable string
-*/
-void X509_Time::set_to(const std::string& time_str)
-   {
-   if(time_str == "")
-      {
-      year = month = day = hour = minute = second = 0;
-      tag = NO_OBJECT;
-      return;
-      }
-
-   std::vector<std::string> params;
-   std::string current;
-
-   for(size_t j = 0; j != time_str.size(); ++j)
-      {
-      if(Charset::is_digit(time_str[j]))
-         current += time_str[j];
-      else
-         {
-         if(current != "")
-            params.push_back(current);
-         current.clear();
-         }
-      }
-   if(current != "")
-      params.push_back(current);
-
-   if(params.size() < 3 || params.size() > 6)
-      throw Invalid_Argument("Invalid time specification " + time_str);
-
-   year   = to_u32bit(params[0]);
-   month  = to_u32bit(params[1]);
-   day    = to_u32bit(params[2]);
-   hour   = (params.size() >= 4) ? to_u32bit(params[3]) : 0;
-   minute = (params.size() >= 5) ? to_u32bit(params[4]) : 0;
-   second = (params.size() == 6) ? to_u32bit(params[5]) : 0;
 
-   tag = (year >= 2050) ? GENERALIZED_TIME : UTC_TIME;
-
-   if(!passes_sanity_check())
-      throw Invalid_Argument("Invalid time specification " + time_str);
-   }
-
-/*
-* Set the time with an ISO time format string
-*/
-void X509_Time::set_to(const std::string& t_spec, ASN1_Tag spec_tag)
-   {
-   if(spec_tag == GENERALIZED_TIME)
-      {
-      if(t_spec.size() != 13 && t_spec.size() != 15)
-         throw Invalid_Argument("Invalid GeneralizedTime: " + t_spec);
-      }
-   else if(spec_tag == UTC_TIME)
-      {
-      if(t_spec.size() != 11 && t_spec.size() != 13)
-         throw Invalid_Argument("Invalid UTCTime: " + t_spec);
-      }
-   else
-      {
-      throw Invalid_Argument("Invalid time tag " + std::to_string(spec_tag) + " val " + t_spec);
-      }
-
-   if(t_spec[t_spec.size()-1] != 'Z')
-      throw Invalid_Argument("Invalid time encoding: " + t_spec);
-
-   const size_t YEAR_SIZE = (spec_tag == UTC_TIME) ? 2 : 4;
-
-   std::vector<std::string> params;
-   std::string current;
-
-   for(size_t j = 0; j != YEAR_SIZE; ++j)
-      current += t_spec[j];
-   params.push_back(current);
-   current.clear();
-
-   for(size_t j = YEAR_SIZE; j != t_spec.size() - 1; ++j)
-      {
-      current += t_spec[j];
-      if(current.size() == 2)
-         {
-         params.push_back(current);
-         current.clear();
-         }
-      }
-
-   year   = to_u32bit(params[0]);
-   month  = to_u32bit(params[1]);
-   day    = to_u32bit(params[2]);
-   hour   = to_u32bit(params[3]);
-   minute = to_u32bit(params[4]);
-   second = (params.size() == 6) ? to_u32bit(params[5]) : 0;
-   tag    = spec_tag;
-
-   if(spec_tag == UTC_TIME)
-      {
-      if(year >= 50) year += 1900;
-      else           year += 2000;
-      }
-
-   if(!passes_sanity_check())
-      throw Invalid_Argument("Invalid time specification " + t_spec);
-   }
 
 /*
 * DER encode a X509_Time
 */
 void X509_Time::encode_into(DER_Encoder& der) const
    {
-   if(tag != GENERALIZED_TIME && tag != UTC_TIME)
+   if(m_tag != GENERALIZED_TIME && m_tag != UTC_TIME)
       throw Invalid_Argument("X509_Time: Bad encoding tag");
 
-   der.add_object(tag, UNIVERSAL,
-                  Charset::transcode(as_string(),
+   der.add_object(m_tag, UNIVERSAL,
+                  Charset::transcode(to_string(),
                                      LOCAL_CHARSET,
                                      LATIN1_CHARSET));
    }
@@ -182,33 +64,36 @@ void X509_Time::decode_from(BER_Decoder& source)
           ber_time.type_tag);
    }
 
-/*
-* Return a string representation of the time
-*/
-std::string X509_Time::as_string() const
+std::string X509_Time::to_string() const
    {
    if(time_is_set() == false)
       throw Invalid_State("X509_Time::as_string: No time set");
 
-   u32bit full_year = year;
+   u32bit full_year = m_year;
 
-   if(tag == UTC_TIME)
+   if(m_tag == UTC_TIME)
       {
-      if(year < 1950 || year >= 2050)
+      if(m_year < 1950 || m_year >= 2050)
          throw Encoding_Error("X509_Time: The time " + readable_string() +
                               " cannot be encoded as a UTCTime");
 
-      full_year = (year >= 2000) ? (year - 2000) : (year - 1900);
+      full_year = (m_year >= 2000) ? (m_year - 2000) : (m_year - 1900);
       }
 
-   std::string repr = std::to_string(full_year*10000000000 +
-                                     month*100000000 +
-                                     day*1000000 +
-                                     hour*10000 +
-                                     minute*100 +
-                                     second) + "Z";
+   const auto factor_y = uint64_t{10000000000ull}; // literal exceeds 32bit int range
+   const auto factor_m = uint64_t{100000000ull};
+   const auto factor_d = uint64_t{1000000ull};
+   const auto factor_h = uint64_t{10000ull};
+   const auto factor_i = uint64_t{100ull};
+
+   std::string repr = std::to_string(factor_y * full_year +
+                                     factor_m * m_month +
+                                     factor_d * m_day +
+                                     factor_h * m_hour +
+                                     factor_i * m_minute +
+                                     m_second) + "Z";
 
-   u32bit desired_size = (tag == UTC_TIME) ? 13 : 15;
+   u32bit desired_size = (m_tag == UTC_TIME) ? 13 : 15;
 
    while(repr.size() < desired_size)
       repr = "0" + repr;
@@ -216,17 +101,6 @@ std::string X509_Time::as_string() const
    return repr;
    }
 
-/*
-* Return if the time has been set somehow
-*/
-bool X509_Time::time_is_set() const
-   {
-   return (year != 0);
-   }
-
-/*
-* Return a human readable string representation
-*/
 std::string X509_Time::readable_string() const
    {
    if(time_is_set() == false)
@@ -237,33 +111,19 @@ std::string X509_Time::readable_string() const
       {
       using namespace std;
       output << setfill('0')
-             << setw(4) << year << "/" << setw(2) << month << "/" << setw(2) << day
+             << setw(4) << m_year << "/" << setw(2) << m_month << "/" << setw(2) << m_day
              << " "
-             << setw(2) << hour << ":" << setw(2) << minute << ":" << setw(2) << second
+             << setw(2) << m_hour << ":" << setw(2) << m_minute << ":" << setw(2) << m_second
              << " UTC";
       }
    return output.str();
    }
 
-/*
-* Do a general sanity check on the time
-*/
-bool X509_Time::passes_sanity_check() const
+bool X509_Time::time_is_set() const
    {
-   if(year < 1950 || year > 2100)
-      return false;
-   if(month == 0 || month > 12)
-      return false;
-   if(day == 0 || day > 31)
-      return false;
-   if(hour >= 24 || minute > 60 || second > 60)
-      return false;
-   return true;
+   return (m_year != 0);
    }
 
-/*
-* Compare this time against another
-*/
 s32bit X509_Time::cmp(const X509_Time& other) const
    {
    if(time_is_set() == false)
@@ -271,22 +131,96 @@ s32bit X509_Time::cmp(const X509_Time& other) const
 
    const s32bit EARLIER = -1, LATER = 1, SAME_TIME = 0;
 
-   if(year < other.year)     return EARLIER;
-   if(year > other.year)     return LATER;
-   if(month < other.month)   return EARLIER;
-   if(month > other.month)   return LATER;
-   if(day < other.day)       return EARLIER;
-   if(day > other.day)       return LATER;
-   if(hour < other.hour)     return EARLIER;
-   if(hour > other.hour)     return LATER;
-   if(minute < other.minute) return EARLIER;
-   if(minute > other.minute) return LATER;
-   if(second < other.second) return EARLIER;
-   if(second > other.second) return LATER;
+   if(m_year < other.m_year)     return EARLIER;
+   if(m_year > other.m_year)     return LATER;
+   if(m_month < other.m_month)   return EARLIER;
+   if(m_month > other.m_month)   return LATER;
+   if(m_day < other.m_day)       return EARLIER;
+   if(m_day > other.m_day)       return LATER;
+   if(m_hour < other.m_hour)     return EARLIER;
+   if(m_hour > other.m_hour)     return LATER;
+   if(m_minute < other.m_minute) return EARLIER;
+   if(m_minute > other.m_minute) return LATER;
+   if(m_second < other.m_second) return EARLIER;
+   if(m_second > other.m_second) return LATER;
 
    return SAME_TIME;
    }
 
+void X509_Time::set_to(const std::string& t_spec, ASN1_Tag spec_tag)
+   {
+   if(spec_tag == GENERALIZED_TIME)
+      {
+      if(t_spec.size() != 13 && t_spec.size() != 15)
+         throw Invalid_Argument("Invalid GeneralizedTime string: '" + t_spec + "'");
+      }
+   else if(spec_tag == UTC_TIME)
+      {
+      if(t_spec.size() != 11 && t_spec.size() != 13)
+         throw Invalid_Argument("Invalid UTCTime string: '" + t_spec + "'");
+      }
+   else
+      {
+      throw Invalid_Argument("Invalid time tag " + std::to_string(spec_tag) + " val " + t_spec);
+      }
+
+   if(t_spec[t_spec.size()-1] != 'Z')
+      throw Invalid_Argument("Invalid time encoding: " + t_spec);
+
+   const size_t YEAR_SIZE = (spec_tag == UTC_TIME) ? 2 : 4;
+
+   std::vector<std::string> params;
+   std::string current;
+
+   for(size_t j = 0; j != YEAR_SIZE; ++j)
+      current += t_spec[j];
+   params.push_back(current);
+   current.clear();
+
+   for(size_t j = YEAR_SIZE; j != t_spec.size() - 1; ++j)
+      {
+      current += t_spec[j];
+      if(current.size() == 2)
+         {
+         params.push_back(current);
+         current.clear();
+         }
+      }
+
+   m_year   = to_u32bit(params[0]);
+   m_month  = to_u32bit(params[1]);
+   m_day    = to_u32bit(params[2]);
+   m_hour   = to_u32bit(params[3]);
+   m_minute = to_u32bit(params[4]);
+   m_second = (params.size() == 6) ? to_u32bit(params[5]) : 0;
+   m_tag    = spec_tag;
+
+   if(spec_tag == UTC_TIME)
+      {
+      if(m_year >= 50) m_year += 1900;
+      else             m_year += 2000;
+      }
+
+   if(!passes_sanity_check())
+      throw Invalid_Argument("Time did not pass sanity check: " + t_spec);
+   }
+
+/*
+* Do a general sanity check on the time
+*/
+bool X509_Time::passes_sanity_check() const
+   {
+   if(m_year < 1950 || m_year > 2100)
+      return false;
+   if(m_month == 0 || m_month > 12)
+      return false;
+   if(m_day == 0 || m_day > 31)
+      return false;
+   if(m_hour >= 24 || m_minute > 60 || m_second > 60)
+      return false;
+   return true;
+   }
+
 /*
 * Compare two X509_Times for in various ways
 */
diff --git a/src/lib/asn1/asn1_time.h b/src/lib/asn1/asn1_time.h
index 6200d7b62..f2b1c7975 100644
--- a/src/lib/asn1/asn1_time.h
+++ b/src/lib/asn1/asn1_time.h
@@ -22,24 +22,38 @@ class BOTAN_DLL X509_Time : public ASN1_Object
       void encode_into(class DER_Encoder&) const override;
       void decode_from(class BER_Decoder&) override;
 
-      std::string as_string() const;
+      /// Return an internal string representation of the time
+      std::string to_string() const;
+
+      /// Returns a human friendly string replesentation of no particular formatting
       std::string readable_string() const;
-      bool time_is_set() const;
 
-      std::string to_string() const { return readable_string(); }
+      /// Return if the time has been set somehow
+      bool time_is_set() const;
 
-      s32bit cmp(const X509_Time&) const;
+      ///  Compare this time against another
+      s32bit cmp(const X509_Time& other) const;
 
-      void set_to(const std::string&);
-      void set_to(const std::string&, ASN1_Tag);
+      /// Create an invalid X509_Time
+      X509_Time() {}
 
+      /// Create a X509_Time from a time point
       X509_Time(const std::chrono::system_clock::time_point& time);
-      X509_Time(const std::string& = "");
-      X509_Time(const std::string&, ASN1_Tag);
+
+      /// Create an X509_Time from string
+      X509_Time(const std::string& t_spec, ASN1_Tag tag);
+
    private:
+      void set_to(const std::string& t_spec, ASN1_Tag);
       bool passes_sanity_check() const;
-      u32bit year, month, day, hour, minute, second;
-      ASN1_Tag tag;
+
+      u32bit m_year = 0;
+      u32bit m_month = 0;
+      u32bit m_day = 0;
+      u32bit m_hour = 0;
+      u32bit m_minute = 0;
+      u32bit m_second = 0;
+      ASN1_Tag m_tag = NO_OBJECT;
    };
 
 /*
diff --git a/src/lib/cert/x509/x509_crl.cpp b/src/lib/cert/x509/x509_crl.cpp
index e3dfb787e..5dce682c2 100644
--- a/src/lib/cert/x509/x509_crl.cpp
+++ b/src/lib/cert/x509/x509_crl.cpp
@@ -102,8 +102,8 @@ void X509_CRL::force_decode()
 
    X509_Time start, end;
    tbs_crl.decode(start).decode(end);
-   info.add("X509.CRL.start", start.readable_string());
-   info.add("X509.CRL.end", end.readable_string());
+   info.add("X509.CRL.start", start.to_string());
+   info.add("X509.CRL.end", end.to_string());
 
    BER_Object next = tbs_crl.get_next_object();
 
@@ -177,7 +177,7 @@ u32bit X509_CRL::crl_number() const
 */
 X509_Time X509_CRL::this_update() const
    {
-   return info.get1("X509.CRL.start");
+   return X509_Time(info.get1("X509.CRL.start"), ASN1_Tag::UTC_TIME);
    }
 
 /*
@@ -185,7 +185,7 @@ X509_Time X509_CRL::this_update() const
 */
 X509_Time X509_CRL::next_update() const
    {
-   return info.get1("X509.CRL.end");
+   return X509_Time(info.get1("X509.CRL.end"), ASN1_Tag::UTC_TIME);
    }
 
 }
diff --git a/src/lib/cert/x509/x509cert.cpp b/src/lib/cert/x509/x509cert.cpp
index 195af7730..f6f87bbf4 100644
--- a/src/lib/cert/x509/x509cert.cpp
+++ b/src/lib/cert/x509/x509cert.cpp
@@ -136,8 +136,8 @@ void X509_Certificate::force_decode()
 
    subject.add("X509.Certificate.version", version);
    subject.add("X509.Certificate.serial", BigInt::encode(serial_bn));
-   subject.add("X509.Certificate.start", start.readable_string());
-   subject.add("X509.Certificate.end", end.readable_string());
+   subject.add("X509.Certificate.start", start.to_string());
+   subject.add("X509.Certificate.end", end.to_string());
 
    issuer.add("X509.Certificate.v2.key_id", v2_issuer_key_id);
    subject.add("X509.Certificate.v2.key_id", v2_subject_key_id);
diff --git a/src/lib/cert/x509/x509opt.cpp b/src/lib/cert/x509/x509opt.cpp
index c620ab25b..ef83124e6 100644
--- a/src/lib/cert/x509/x509opt.cpp
+++ b/src/lib/cert/x509/x509opt.cpp
@@ -17,7 +17,7 @@ namespace Botan {
 */
 void X509_Cert_Options::not_before(const std::string& time_string)
    {
-   start = X509_Time(time_string);
+   start = X509_Time(time_string, ASN1_Tag::UTC_TIME);
    }
 
 /*
@@ -25,7 +25,7 @@ void X509_Cert_Options::not_before(const std::string& time_string)
 */
 void X509_Cert_Options::not_after(const std::string& time_string)
    {
-   end = X509_Time(time_string);
+   end = X509_Time(time_string, ASN1_Tag::UTC_TIME);
    }
 
 /*
diff --git a/src/lib/cert/x509/x509path.cpp b/src/lib/cert/x509/x509path.cpp
index fa6d34a2d..b6c0df126 100644
--- a/src/lib/cert/x509/x509path.cpp
+++ b/src/lib/cert/x509/x509path.cpp
@@ -107,10 +107,10 @@ check_chain(const std::vector<X509_Certificate>& cert_path,
          }
 
       // Check all certs for valid time range
-      if(current_time < X509_Time(subject.start_time()))
+      if(current_time < X509_Time(subject.start_time(), ASN1_Tag::UTC_TIME))
          status.insert(Certificate_Status_Code::CERT_NOT_YET_VALID);
 
-      if(current_time > X509_Time(subject.end_time()))
+      if(current_time > X509_Time(subject.end_time(), ASN1_Tag::UTC_TIME))
          status.insert(Certificate_Status_Code::CERT_HAS_EXPIRED);
 
       // Check issuer constraints
diff --git a/src/tests/catchy/test_x509.cpp b/src/tests/catchy/test_x509.cpp
index 01c01dbdc..ad4aa1b1b 100644
--- a/src/tests/catchy/test_x509.cpp
+++ b/src/tests/catchy/test_x509.cpp
@@ -3,45 +3,45 @@
 
 #include "catchy_tests.h"
 
-// deacticate due to
-// https://github.com/randombit/botan/issues/185
-
-#if 0
-
 #if defined(BOTAN_HAS_ASN1)
 
 #include <botan/asn1_time.h>
 
+using namespace Botan;
+
 TEST_CASE("human readable time", "[X509]")
    {
-   auto time1 = Botan::X509_Time("2008-02-01");
-   auto time2 = Botan::X509_Time("2008-02-01 17:24:33");
-   auto time3 = Botan::X509_Time("2004-06-14T23:34:30");
+   auto time1 = X509_Time("0802010000Z", ASN1_Tag::UTC_TIME);
+   auto time2 = X509_Time("0802011724Z", ASN1_Tag::UTC_TIME);
+   auto time3 = X509_Time("040614233430Z", ASN1_Tag::UTC_TIME);
 
-   CHECK(( time1.time_is_set() == true ));
-   CHECK(( time2.time_is_set() == true ));
-   CHECK(( time3.time_is_set() == true ));
+   CHECK_THAT(time1.time_is_set(), Equals(true));
+   CHECK_THAT(time2.time_is_set(), Equals(true));
+   CHECK_THAT(time3.time_is_set(), Equals(true));
 
-   CHECK(( time1.readable_string() == "2008/02/01 00:00:00 UTC" ));
-   CHECK(( time2.readable_string() == "2008/02/01 17:24:33 UTC" ));
-   CHECK(( time3.readable_string() == "2004/06/14 23:34:30 UTC" ));
+   CHECK_THAT(time1.readable_string(), Equals("2008/02/01 00:00:00 UTC"));
+   CHECK_THAT(time2.readable_string(), Equals("2008/02/01 17:24:00 UTC"));
+   CHECK_THAT(time3.readable_string(), Equals("2004/06/14 23:34:30 UTC"));
    }
 
 TEST_CASE("no time", "[X509]")
    {
-   auto time = Botan::X509_Time("");
-   CHECK(( time.time_is_set() == false ));
+   auto time = X509_Time();
+   CHECK_THAT(time.time_is_set(), Equals(false));
    }
 
 TEST_CASE("invalid time", "[X509]")
    {
-   CHECK_THROWS( Botan::X509_Time(" ") );
-   CHECK_THROWS( Botan::X509_Time("2008`02-01") );
-   CHECK_THROWS( Botan::X509_Time("9999-02-01") );
-   CHECK_THROWS( Botan::X509_Time("2000-02-01 17") );
-   CHECK_THROWS( Botan::X509_Time("999921") );
+   CHECK_THROWS(X509_Time("", ASN1_Tag::UTC_TIME));
+   CHECK_THROWS(X509_Time(" ", ASN1_Tag::UTC_TIME));
+   CHECK_THROWS(X509_Time("2008`02-01", ASN1_Tag::UTC_TIME));
+   CHECK_THROWS(X509_Time("9999-02-01", ASN1_Tag::UTC_TIME));
+   CHECK_THROWS(X509_Time("2000-02-01 17", ASN1_Tag::UTC_TIME));
+   CHECK_THROWS(X509_Time("999921", ASN1_Tag::UTC_TIME));
+
+   // wrong time zone
+   CHECK_THROWS(X509_Time("0802010000", ASN1_Tag::UTC_TIME));
+   CHECK_THROWS(X509_Time("0802010000z", ASN1_Tag::UTC_TIME));
    }
 
 #endif // BOTAN_HAS_ASN1
-
-#endif