Re: User registration/authentication

["Alexey Parshin" <alexeyp@gmail.com>]

Actually,

It should be possible to use just:

sinfo.person_id = idField;

2007/3/17, sergey@total-knowledge.com < sergey@total-knowledge.com>:

I had it initially as
sinfo.person_id = idField.asInteger ();

But I thought it's a wrong way to do it since person_id is type 'long' in
SessionInfo vector.

> Well, as an expert in C++ I can tell that's how I do casting from string
> to long.
> There is no asLong() function in C++ as much as I know, what's the right
> way to do it?
>
>> Can anyone tell me, why do we need this cast:
>>
>>     sinfo.person_id = lexical_cast<long>( idField.asString());
>>
>>
>> 2007/3/17, Ilya A. Volynets-Evenbakh <ilya@total-knowledge.com>:
>>>
>>> sergey@total-knowledge.com wrote:
>>> > Attached trunk0316.diff file with current application.
>>> >
>>> > It has Register, Login and Home pages. Home page is login protected
>>> and
>>> > not-logged-in user gets redirected to Login page.
>>> >
>>> Why?
>>> >
>>> > Index: libui/RegisterServlet.h
>>> > ===================================================================
>>> > --- libui/RegisterServlet.h   (revision 0)
>>> > +++ libui/RegisterServlet.h   (revision 0)
>>> > @@ -0,0 +1,28 @@
>>> > +#ifndef REGISTERSERVLET_H
>>> > +#define REGISTERSERVLET_H
>>> > +
>>> > +#include <boost/lexical_cast.hpp>
>>> > +#include "UUServlet.h"
>>> > +
>>> > +using boost::lexical_cast;
>>> > +using namespace std;
>>> > +using namespace servlet;
>>> >
>>> Never put "using" statements in header files.
>>> > +
>>> > +/**
>>> > +  * This servlet:
>>> > +  * Receives registration request.
>>> > +  * Accesses User model object.
>>> > +  * Stores results in request context.
>>> > +  * Determines the language to render page in based on user
>>> preferences.
>>> > +  * Forwards the request to the view object (HomeView.csp) if no
>>> errors.
>>> >
>>> Shouldn't RegisterServlet be using something like Register.csp?
>>> > Index: libui/UUServlet.cpp
>>> > ===================================================================
>>> > --- libui/UUServlet.cpp       (revision 0)
>>> > +++ libui/UUServlet.cpp       (revision 0)
>>> > @@ -0,0 +1,48 @@
>>> > +#include "UUServlet.h"
>>> > +
>>> > +UUServlet::UUServlet ( ) { }
>>> >
>>> Do not create empty constructors.
>>> > +UUServlet::~UUServlet ( ) { }
>>> >
>>> Only create empty destructors if they are virtual (so, it's OK in this
>>> case).
>>> > +
>>> > +/**
>>> > + * Returns true if the request already has valid associated session,
>>> and that
>>> > + * session contains valid User object
>>> > + * @return bool
>>> > + * @param  req Request to check
>>> > + */
>>> > +
>>> > +bool UUServlet::isUserLoggedIn (HttpServletRequest& req ) const {
>>> >
>>> Put opening '{' on new line for functions (not for blocks inside
>>> functions though)
>>> > +
>>> > +  HttpSession* session = req.getSession(true);
>>> > +  if(!session) throw runtime_error("no session");
>>> >
>>> That error message doesn't make that much sense.
>>> > +  string sessionid = session->getId();
>>> > +  user_t user;
>>> > +  bool hasValidLogin = user.isSessionValid (sessionid);
>>> >
>>> Session validity is not defined by whether user is logged in or not,
>>> so I guess that's a misnomer.
>>> > +  return hasValidLogin;
>>> > +
>>> > +}
>>> > +
>>> > +
>>> > +/**
>>> > + * Redirects browser to login page
>>> > + * @param  resp send redirect through this response object
>>> > + */
>>> > +void UUServlet::redirectToLogin (HttpServletRequest& req,
>>> HttpServletResponse& resp) {
>>> > +
>>> > +  req.setAttribute ("Message", setattr_t(new string("Please login
>>> first!")));
>>> >
>>> What does setattr_t mean? (Again - load your names with real meaning)
>>> Another thing - why are you specifying that exact message? Couldn't it
>>> so happen
>>> that you are redirecting to the login page from somewhere where no log
>>> in
>>> is
>>> required (LogoutServlet perhaps?)
>>> > +   req.getRequestDispatcher("LoginView.csp")->forward(req,resp);
>>> > +
>>> > +}
>>> > +
>>> > +
>>> > +/**
>>> > + * @return User
>>> > + * @param  req
>>> > + */
>>> > +User UUServlet::getCurrentUser (HttpServletRequest& req ) {
>>> > +
>>> > +  user_t user;
>>> > +  return user;
>>> >
>>> Eh? If it isn't implemented - leave //FIXME: or //TODO: marker.
>>> Otherwise, comment this piece of crypto properly.
>>> > +
>>> > +}
>>> > +
>>> > +
>>> >
>>> >
>>> > Index: libui/LoginServlet.h
>>> > ===================================================================
>>> > --- libui/LoginServlet.h      (revision 0)
>>> > +++ libui/LoginServlet.h      (revision 0)
>>> > @@ -0,0 +1,25 @@
>>> > +#ifndef LOGINSERVLET_H
>>> > +#define LOGINSERVLET_H
>>> > +
>>> > +#include "UUServlet.h"
>>> > +
>>> > +using namespace std;
>>> > +using namespace servlet;
>>> > +
>>> > +/**
>>> > +  *  This servlet:
>>> > +  *  Receives login request.
>>> > +  *  Accesses User model object.
>>> > +  *  Stores results in request context.
>>> > +  *  Determines the language to render page in based on user
>>> preferences.
>>> > +  *  Forwards the request to the view object (HomeView.csp) if no
>>> errors.
>>> > +  */
>>> > +
>>> > +class LoginServlet : public UUServlet
>>> > +{
>>> > +public:
>>> > +  void service(HttpServletRequest& request, HttpServletResponse
>>> &response);
>>> > +};
>>> > +
>>> > +#endif//LOGINSERVLET_H
>>> > +
>>> >
>>> >
>>> > Index: libui/UUServlet.h
>>> > ===================================================================
>>> > --- libui/UUServlet.h (revision 0)
>>> > +++ libui/UUServlet.h (revision 0)
>>> > @@ -0,0 +1,57 @@
>>> > +#ifndef UUSERVLET_H
>>> > +#define UUSERVLET_H
>>> > +class UUServlet : public servlet::HttpServlet
>>> > +{
>>> > +public:
>>> > +  /**
>>> > +   * Redirects browser to login page
>>> > +   * @param  resp send redirect through this response object
>>> > +   */
>>> > +  void redirectToLogin (servlet::HttpServletRequest& req,
>>> servlet::HttpServletResponse& resp);
>>> >
>>> Do you need request arg here?
>>> >
>>> > Index: libui/Makefile.adon
>>> > ===================================================================
>>> > --- libui/Makefile.adon       (revision 82)
>>> > +++ libui/Makefile.adon       (working copy)
>>> > +noinst_HEADERS:=../libdm/User.h
>>> Can't do that. Only files current dir can be mentioned.
>>> > +
>>> > +EXTRA_DIST:=Footer.csp Header.csp Sidebar.csp
>>> > +libuu_SOURCES:=../libdm/User.cpp UUServlet.cpp ../libdm/uudb.cpp
>>> >
>>> Again. Can't do that. libdm should be a separate library, that you will
>>> link with.
>>> >
>>> > Index: engine.xml
>>> > ===================================================================
>>> > --- engine.xml        (revision 0)
>>> > +++ engine.xml        (revision 0)
>>> > @@ -0,0 +1,10 @@
>>> > +<?xml version="1.0"?>
>>> > +<listener protocol="unix" path="/tmp/cppserv.sock"/>
>>> > +<app name="">
>>> > +    <servlet name="HomeServlet" dso="./debug/libui/HomeServlet.so"/>
>>> > +    <servlet name="RegisterServlet"
>>> dso="./debug/libui/RegisterServlet.so"/>
>>> > +    <servlet name="LoginServlet"
>>> dso="./debug/libui/LoginServlet.so"/>
>>> > +    <csp name="themes/HomeView.csp" path=" HomeView.csp"
>>> dso="./debug/themes/HomeView.so" hidden="true"/>
>>> > +    <csp name="themes/RegistrationView.csp"
>>> path="RegistrationView.csp "
>>> dso="./debug/themes/RegistrationView.so" hidden="true"/>
>>> > +    <csp name="themes/LoginView.csp" path="LoginView.csp"
>>> dso="./debug/themes/LoginView.so" hidden="true"/>
>>> > +</app>
>>> >
>>> You know that cppserv now supports multiple servlets in single DSO,
>>> right?
>>> i.e. you could link them all into a single library.. That might make
>>> some things easier
>>> (not necessary by all means, but probably good idea for themes).
>>> Speaking of themes, make sure your templates are in subdirectory of
>>> themes, not
>>> in themes directory itself. something like themes/default/
>>> > Index: libdm/User.cpp
>>> > ===================================================================
>>> > --- libdm/User.cpp    (revision 0)
>>> > +++ libdm/User.cpp    (revision 0)
>>> > @@ -0,0 +1,128 @@
>>> > +#include "User.h"
>>> > +
>>> > +User::User() {}
>>> > +User::~User() {}
>>> > +
>>> > +/**
>>> > + * Performs authentication.
>>> > + * @return bool
>>> > + */
>>> > +
>>> > +bool User::isLoginPasswordValid(string login, string password) {
>>> > +
>>> > +  CODBCDatabase db("DSN=PostgreSQL;UID=sergey;PWD=;DATABASE=uu");
>>> > +  bool isValid = false;
>>> > +  try {
>>> > +    CQuery qrySelect(&db,"select pl_id from person_list where
>>> pl_login=:login and pl_password=:password");
>>> > +    qrySelect.param("login") = login;
>>> > +    qrySelect.param("password") = password;
>>> > +     qrySelect.open();
>>> > +    while ( ! qrySelect.eof() ) {
>>> > +      isValid = true;
>>> > +      qrySelect.fetch();
>>> > +    }
>>> > +    qrySelect.close ();
>>> > +  }
>>> > +  catch (exception& e) {
>>> > +    cout<<"\nError: " <<e.what();
>>> > +    return false;
>>> > +  }
>>> > +  return isValid;
>>> > +
>>> > +}
>>> > +
>>> > +/**
>>> > + * Sets data to session_info temp table after successful
>>> authentication
>>> > + */
>>> > +
>>> > +bool User::setSessionInfo(string login, string password, int server,
>>> string sessionid) {
>>> > +
>>> > +  CODBCDatabase db("DSN=PostgreSQL;UID=sergey;PWD=;DATABASE=uu");
>>> > +  try {
>>> > +    CQuery qryInsert(&db,"select login('"+login+"', '"+password+"',
>>> "+lexical_cast<string>(server)+")");
>>> > +    qryInsert.exec();
>>> > +  }
>>> > +  catch (exception& e) {
>>> > +    cout<<"\nError: " <<e.what();
>>> > +    return false;
>>> > +  }
>>> > +  return true;
>>> > +
>>> > +}
>>> > +
>>> > +/**
>>> > + * Gets user data from session_info table.
>>> > + */
>>> > +
>>> > +User::sessioninfo_t User::getSessionInfo(string sessionid) {
>>> > +
>>> > +  CODBCDatabase db("DSN=PostgreSQL;UID=sergey;PWD=;DATABASE=uu");
>>> > +  sessioninfo_t sessioninfo;
>>> > +  try {
>>> > +    CQuery qrySelect(&db,"select * from session_info where
>>> si_session=:sessionid");
>>> > +    qrySelect.param("sessionid") = sessionid;
>>> > +    qrySelect.open();
>>> > +    CField& idField = qrySelect["si_person"];
>>> > +    CField& fnameField = qrySelect["si_person_name"];
>>> > +    CField& serverField = qrySelect["si_server"];
>>> > +    while ( ! qrySelect.eof() ) {
>>> > +      SessionInfo sinfo;
>>> > +      sinfo.person_id = lexical_cast<long>(idField.asString());
>>> > +      sinfo.person_name = fnameField.asString();
>>> > +      sinfo.server = serverField.asInteger();
>>> > +      sessioninfo.push_back (sinfo);
>>> > +      qrySelect.fetch();
>>> > +    }
>>> > +    qrySelect.close();
>>> > +  }
>>> > +  catch (exception& e) {
>>> > +    cout<<"\nError: " << e.what();
>>> > +  }
>>> > +  return sessioninfo;
>>> > +
>>> > +}
>>> > +
>>> > +/**
>>> > + * Gets user id from session_info table. Returns true if such user
>>> exist.
>>> > + */
>>> > +
>>> > +bool User::isSessionValid(string sessionid) {
>>> > +
>>> > +  CODBCDatabase db("DSN=PostgreSQL;UID=sergey;PWD=;DATABASE=uu");
>>> > +  bool isValid = false;
>>> > +  try {
>>> > +    CQuery qrySelect(&db,"select si_person from session_info where
>>> si_session=:sessionid");
>>> > +    qrySelect.param("sessionid") = sessionid;
>>> > +    qrySelect.open();
>>> > +    while ( ! qrySelect.eof() ) {
>>> > +      isValid = true;
>>> > +      qrySelect.fetch();
>>> > +    }
>>> > +    qrySelect.close();
>>> > +  }
>>> > +  catch (exception& e) {
>>> > +    cout<<"\nError: " << e.what();
>>> > +  }
>>> > +  return isValid;
>>> > +
>>> > +}
>>> > +
>>> > +/**
>>> > + * Sets all user data during a registration process. Calls stored
>>> procedure "person_create" to insert user data into "person_list" table.
>>> If
>>> insert is successful, returns true. Otherwise returns false.
>>> > + */
>>> > +
>>> > +bool User::setUserInfo(string login, string password, int language,
>>> string firstName, string lastName, string street, string city, string
>>> state,
>>> string zip, string country, string email, string phone ) {
>>> > +
>>> > +  CODBCDatabase db("DSN=PostgreSQL;UID=sergey;PWD=;DATABASE=uu");
>>> > +  try {
>>> > +    CQuery qryInsert(&db,"select
>>> person_create('"+firstName+"','"+lastName+"','"+street+"','"+city+"','"+state+"','"+zip+"','"+country+"','"+email+"','"+phone+"','"+login+"','"+password+"')");
>>> > +    qryInsert.exec();
>>> > +  }
>>> > +  catch (exception& e) {
>>> > +    cout<<"\nError: " <<e.what();
>>> > +    return false;
>>> > +  }
>>> > +  return true;
>>> > +
>>> > +}
>>> > +
>>> >
>>> >
>>> I will let Alexey review and comment your database code.
>>> >
>>> > Index: libdm/uudb.cpp
>>> > ===================================================================
>>> > --- libdm/uudb.cpp    (revision 0)
>>> > +++ libdm/uudb.cpp    (revision 0)
>>> > @@ -0,0 +1,33 @@
>>> >
>>> All of the uudb code you wrote should be replaced with db pool code.
>>>
>>>
>>> I didn't review some of your actual code, for now fixing issues that
>>> has already been pointed out will be enough for next iteration.
>>>
>>> --
>>> Ilya A. Volynets-Evenbakh
>>> Total Knowledge. CTO
>>> http://www.total-knowledge.com
>>>
>>>
>>
>>
>> --
>> Alexey Parshin,
>> http://www.sptk.net
>>
>
>
>

-- 
Alexey Parshin,
http://www.sptk.net