UniverseUniversity


Home Projects Jobs Clientele Contact

uu


[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: User registration/authentication



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


Authoright © Total Knowledge: 2001-2008