At the moment you've got to install the authlogic gem -- we will eventually need to package it as an RPM and add it as a dependency in the specfile.
Right now any user can create an account and access all pages. We aren't doing any access control beyond requiring a login account.
Signed-off-by: Scott Seago sseago@redhat.com --- src/app/controllers/application_controller.rb | 39 ++++++++++++++++++++ src/app/controllers/instance_controller.rb | 2 + src/app/controllers/portal_pool_controller.rb | 2 + src/app/controllers/provider_controller.rb | 2 + src/app/controllers/user_sessions_controller.rb | 24 ++++++++++++ src/app/controllers/users_controller.rb | 36 ++++++++++++++++++ src/app/helpers/user_sessions_helper.rb | 2 + src/app/helpers/users_helper.rb | 2 + src/app/models/user.rb | 3 ++ src/app/models/user_session.rb | 8 ++++ src/app/views/layouts/_header.rhtml | 4 +- src/app/views/layouts/dcloud.rhtml | 2 +- src/app/views/user_sessions/login.html.erb | 19 ++++++++++ src/app/views/users/_form.erb | 10 +++++ src/app/views/users/edit.html.erb | 12 ++++++ src/app/views/users/new.html.erb | 10 +++++ src/app/views/users/show.html.erb | 37 +++++++++++++++++++ src/config/environment.rb | 1 + src/config/routes.rb | 8 +++- src/db/migrate/20090917192602_create_users.rb | 30 +++++++++++++++ src/test/fixtures/users.yml | 7 ++++ .../functional/user_sessions_controller_test.rb | 8 ++++ src/test/functional/users_controller_test.rb | 8 ++++ src/test/unit/helpers/user_sessions_helper_test.rb | 4 ++ src/test/unit/helpers/users_helper_test.rb | 4 ++ src/test/unit/user_test.rb | 8 ++++ 26 files changed, 287 insertions(+), 5 deletions(-) create mode 100644 src/app/controllers/user_sessions_controller.rb create mode 100644 src/app/controllers/users_controller.rb create mode 100644 src/app/helpers/user_sessions_helper.rb create mode 100644 src/app/helpers/users_helper.rb create mode 100644 src/app/models/user.rb create mode 100644 src/app/models/user_session.rb create mode 100644 src/app/views/user_sessions/login.html.erb create mode 100644 src/app/views/users/_form.erb create mode 100644 src/app/views/users/edit.html.erb create mode 100644 src/app/views/users/new.html.erb create mode 100644 src/app/views/users/show.html.erb create mode 100644 src/db/migrate/20090917192602_create_users.rb create mode 100644 src/test/fixtures/users.yml create mode 100644 src/test/functional/user_sessions_controller_test.rb create mode 100644 src/test/functional/users_controller_test.rb create mode 100644 src/test/unit/helpers/user_sessions_helper_test.rb create mode 100644 src/test/unit/helpers/users_helper_test.rb create mode 100644 src/test/unit/user_test.rb
diff --git a/src/app/controllers/application_controller.rb b/src/app/controllers/application_controller.rb index d32f1de..99433c6 100644 --- a/src/app/controllers/application_controller.rb +++ b/src/app/controllers/application_controller.rb @@ -24,6 +24,8 @@ class ApplicationController < ActionController::Base # FIXME: not sure what we're doing aobut service layer w/ deltacloud include ApplicationService + filter_parameter_logging :password, :password_confirmation + helper_method :current_user_session, :current_user
init_gettext "ovirt" layout :choose_layout @@ -186,4 +188,41 @@ class ApplicationController < ActionController::Base end return hash end + + def current_user_session + return @current_user_session if defined?(@current_user_session) + @current_user_session = UserSession.find + end + + def current_user + return @current_user if defined?(@current_user) + @current_user = current_user_session && current_user_session.user + end + + def require_user + unless current_user + store_location + flash[:notice] = "You must be logged in to access this page" + redirect_to login_url + return false + end + end + + def require_no_user + if current_user + store_location + flash[:notice] = "You must be logged out to access this page" + redirect_to account_url + return false + end + end + + def store_location + session[:return_to] = request.request_uri + end + + def redirect_back_or_default(default) + redirect_to(session[:return_to] || default) + session[:return_to] = nil + end end diff --git a/src/app/controllers/instance_controller.rb b/src/app/controllers/instance_controller.rb index 398fdde..59e19ed 100644 --- a/src/app/controllers/instance_controller.rb +++ b/src/app/controllers/instance_controller.rb @@ -1,4 +1,6 @@ class InstanceController < ApplicationController + before_filter :require_user + def index end
diff --git a/src/app/controllers/portal_pool_controller.rb b/src/app/controllers/portal_pool_controller.rb index 49c2d90..9f41cf6 100644 --- a/src/app/controllers/portal_pool_controller.rb +++ b/src/app/controllers/portal_pool_controller.rb @@ -1,4 +1,6 @@ class PortalPoolController < ApplicationController + before_filter :require_user + def index render :action => 'new' end diff --git a/src/app/controllers/provider_controller.rb b/src/app/controllers/provider_controller.rb index 9b5e841..e0a96b7 100644 --- a/src/app/controllers/provider_controller.rb +++ b/src/app/controllers/provider_controller.rb @@ -1,4 +1,6 @@ class ProviderController < ApplicationController + before_filter :require_user + def index render :action => 'new' end diff --git a/src/app/controllers/user_sessions_controller.rb b/src/app/controllers/user_sessions_controller.rb new file mode 100644 index 0000000..c6cdd80 --- /dev/null +++ b/src/app/controllers/user_sessions_controller.rb @@ -0,0 +1,24 @@ +class UserSessionsController < ApplicationController + before_filter :require_no_user, :only => [:new, :create] + before_filter :require_user, :only => :destroy + + def login + @user_session = UserSession.new + end + + def create + @user_session = UserSession.new(params[:user_session]) + if @user_session.save + flash[:notice] = "Login successful!" + redirect_back_or_default account_url + else + render :action => :new + end + end + + def logout + current_user_session.destroy + flash[:notice] = "Logout successful!" + redirect_back_or_default login_url + end +end diff --git a/src/app/controllers/users_controller.rb b/src/app/controllers/users_controller.rb new file mode 100644 index 0000000..9fd4212 --- /dev/null +++ b/src/app/controllers/users_controller.rb @@ -0,0 +1,36 @@ +class UsersController < ApplicationController + before_filter :require_no_user, :only => [:new, :create] + before_filter :require_user, :only => [:show, :edit, :update] + + def new + @user = User.new + end + + def create + @user = User.new(params[:user]) + if @user.save + flash[:notice] = "User registered!" + redirect_back_or_default account_url + else + render :action => :new + end + end + + def show + @user = @current_user + end + + def edit + @user = @current_user + end + + def update + @user = @current_user # makes our views "cleaner" and more consistent + if @user.update_attributes(params[:user]) + flash[:notice] = "User updated!" + redirect_to account_url + else + render :action => :edit + end + end +end diff --git a/src/app/helpers/user_sessions_helper.rb b/src/app/helpers/user_sessions_helper.rb new file mode 100644 index 0000000..2018402 --- /dev/null +++ b/src/app/helpers/user_sessions_helper.rb @@ -0,0 +1,2 @@ +module UserSessionsHelper +end diff --git a/src/app/helpers/users_helper.rb b/src/app/helpers/users_helper.rb new file mode 100644 index 0000000..2310a24 --- /dev/null +++ b/src/app/helpers/users_helper.rb @@ -0,0 +1,2 @@ +module UsersHelper +end diff --git a/src/app/models/user.rb b/src/app/models/user.rb new file mode 100644 index 0000000..04c7d17 --- /dev/null +++ b/src/app/models/user.rb @@ -0,0 +1,3 @@ +class User < ActiveRecord::Base + acts_as_authentic +end diff --git a/src/app/models/user_session.rb b/src/app/models/user_session.rb new file mode 100644 index 0000000..6f99d26 --- /dev/null +++ b/src/app/models/user_session.rb @@ -0,0 +1,8 @@ +class UserSession < Authlogic::Session::Base + # gettext_activerecord defines a gettext method for the activerecord + # Validations module. Authlogic uses these Validations also but does + # not define the gettext method. We define it here instead. + def gettext(str) + GetText._(str) + end +end diff --git a/src/app/views/layouts/_header.rhtml b/src/app/views/layouts/_header.rhtml index 76f9e73..f85510f 100644 --- a/src/app/views/layouts/_header.rhtml +++ b/src/app/views/layouts/_header.rhtml @@ -1,10 +1,10 @@ <div class="header_logo"><%= image_tag "dcloud.png" %></div>
<div class="header_info"> - <div id="hi-username">Hi, <%= @user %></div> + <div id="hi-username"><%= "Hi, " + @current_user.login if defined? @current_user %></div> <form method="POST" id="search-form" action="<%= url_for :controller => "search", :action => 'results' %>"> <input id="textfield_effect" name="terms" value="Search" onkeypress="" onfocus="if( this.value == this.defaultValue ) this.value='';" type="text"> <input id="searchbox-button" src="<%= image_path "icon_search.png"%>" title="Search" type="image"> | </form> - <%= link_to 'Log out', { :controller => "login", :action => "logout"}%> + <%= link_to 'Log out', { :controller => "user_sessions", :action => "logout"}%> </div> \ No newline at end of file diff --git a/src/app/views/layouts/dcloud.rhtml b/src/app/views/layouts/dcloud.rhtml index 725f630..53dfc67 100644 --- a/src/app/views/layouts/dcloud.rhtml +++ b/src/app/views/layouts/dcloud.rhtml @@ -69,7 +69,7 @@ </div>
<div id="side"> - <%= render :partial => '/layouts/main_nav' %> + <%= render :partial => '/layouts/main_nav' if defined? @current_user %> </div>
<div id="tabs-and-content-container"> diff --git a/src/app/views/user_sessions/login.html.erb b/src/app/views/user_sessions/login.html.erb new file mode 100644 index 0000000..1322fa2 --- /dev/null +++ b/src/app/views/user_sessions/login.html.erb @@ -0,0 +1,19 @@ +<div class="dcloud_form"> + <%= error_messages_for 'user_session' %> + <h2>Login</h2> + + <% form_for @user_session, :url => user_session_path do |f| %> + <%= f.error_messages %> + <%= f.label :login %><br /> + <%= f.text_field :login %><br /> + <br /> + <%= f.label :password %><br /> + <%= f.password_field :password %><br /> + <br /> + <%= f.check_box :remember_me %><%= f.label :remember_me %><br /> + <br /> + <%= f.submit "Login" %> + <% end %> + <%= link_to 'Register', {:controller => 'users', :action => 'new'}, :class => 'actionlink' %> + +</div> diff --git a/src/app/views/users/_form.erb b/src/app/views/users/_form.erb new file mode 100644 index 0000000..a278d9a --- /dev/null +++ b/src/app/views/users/_form.erb @@ -0,0 +1,10 @@ +<%= form.label :login %><br /> +<%= form.text_field :login %><br /> +<%= form.label :email %><br /> +<%= form.text_field :email %><br /> +<br /> +<%= form.label :password, form.object.new_record? ? nil : "Change password" %><br /> +<%= form.password_field :password %><br /> +<br /> +<%= form.label :password_confirmation %><br /> +<%= form.password_field :password_confirmation %><br /> diff --git a/src/app/views/users/edit.html.erb b/src/app/views/users/edit.html.erb new file mode 100644 index 0000000..7c9db32 --- /dev/null +++ b/src/app/views/users/edit.html.erb @@ -0,0 +1,12 @@ +<div class="dcloud_form"> + <%= error_messages_for 'user' %> + <h2>Edit My Profile</h2> + + <% form_for @user, :url => account_path do |f| %> + <%= f.error_messages %> + <%= render :partial => "form", :object => f %> + <%= f.submit "Update" %> + <% end %> + + <br /><%= link_to "My Profile", account_path %> +</div> diff --git a/src/app/views/users/new.html.erb b/src/app/views/users/new.html.erb new file mode 100644 index 0000000..681d9b0 --- /dev/null +++ b/src/app/views/users/new.html.erb @@ -0,0 +1,10 @@ +<div class="dcloud_form"> + <%= error_messages_for 'user' %> + <h2>Register</h2> + + <% form_for @user, :url => account_path do |f| %> + <%= f.error_messages %> + <%= render :partial => "form", :object => f %> + <%= f.submit "Register" %> + <% end %> +</div> diff --git a/src/app/views/users/show.html.erb b/src/app/views/users/show.html.erb new file mode 100644 index 0000000..e4e6e67 --- /dev/null +++ b/src/app/views/users/show.html.erb @@ -0,0 +1,37 @@ +<p> + <b>Login:</b> + <%=h @user.login %> +</p> + +<p> + <b>Login count:</b> + <%=h @user.login_count %> +</p> + +<p> + <b>Last request at:</b> + <%=h @user.last_request_at %> +</p> + +<p> + <b>Last login at:</b> + <%=h @user.last_login_at %> +</p> + +<p> + <b>Current login at:</b> + <%=h @user.current_login_at %> +</p> + +<p> + <b>Last login ip:</b> + <%=h @user.last_login_ip %> +</p> + +<p> + <b>Current login ip:</b> + <%=h @user.current_login_ip %> +</p> + + +<%= link_to 'Edit', edit_account_path %> diff --git a/src/config/environment.rb b/src/config/environment.rb index 5098306..9a3433d 100644 --- a/src/config/environment.rb +++ b/src/config/environment.rb @@ -42,6 +42,7 @@ Rails::Initializer.run do |config| # config.gem "aws-s3", :lib => "aws/s3" config.gem "gettext", :lib => "gettext_rails" config.gem "gettext", :lib => "gettext_activerecord" + config.gem "authlogic"
# Only load the plugins named here, in the order given. By default, all plugins # in vendor/plugins are loaded in alphabetical order. diff --git a/src/config/routes.rb b/src/config/routes.rb index ae9bc75..d2a171b 100644 --- a/src/config/routes.rb +++ b/src/config/routes.rb @@ -33,8 +33,12 @@ ActionController::Routing::Routes.draw do |map| # -- just remember to delete public/index.html. map.default '', :controller => 'provider'
- map.login '/login', :controller => 'resources', :action => 'list' - map.logout '/logout', :controller => 'resources', :action => 'list' + map.resource :user_session + map.root :controller => "user_sessions", :action => "login" + map.login 'login', :controller => "user_sessions", :action => "login" + map.resource :account, :controller => "users" + map.resources :users +
# Allow downloading Web Service WSDL as a file with an extension # instead of a file named 'wsdl' diff --git a/src/db/migrate/20090917192602_create_users.rb b/src/db/migrate/20090917192602_create_users.rb new file mode 100644 index 0000000..1bcf0d4 --- /dev/null +++ b/src/db/migrate/20090917192602_create_users.rb @@ -0,0 +1,30 @@ +class CreateUsers < ActiveRecord::Migration + def self.up + create_table :users do |t| + t.string :login, :null => false + t.string :email, :null => false + t.string :crypted_password, :null => false + t.string :password_salt, :null => false + t.string :persistence_token, :null => false + t.string :single_access_token, :null => false + t.string :perishable_token, :null => false + # Magic columns, just like ActiveRecord's created_at and updated_at. + # These are automatically maintained by Authlogic if they are present. + t.integer :login_count, :null => false, :default => 0 + t.integer :failed_login_count, :null => false, :default => 0 + t.datetime :last_request_at + t.datetime :current_login_at + t.datetime :last_login_at + t.string :current_login_ip + t.string :last_login_ip + t.timestamps + end + add_index :users, :login + add_index :users, :persistence_token + add_index :users, :last_request_at + end + + def self.down + drop_table :users + end +end diff --git a/src/test/fixtures/users.yml b/src/test/fixtures/users.yml new file mode 100644 index 0000000..5bf0293 --- /dev/null +++ b/src/test/fixtures/users.yml @@ -0,0 +1,7 @@ +# Read about fixtures at http://ar.rubyonrails.org/classes/Fixtures.html + +# one: +# column: value +# +# two: +# column: value diff --git a/src/test/functional/user_sessions_controller_test.rb b/src/test/functional/user_sessions_controller_test.rb new file mode 100644 index 0000000..5024b23 --- /dev/null +++ b/src/test/functional/user_sessions_controller_test.rb @@ -0,0 +1,8 @@ +require 'test_helper' + +class UserSessionsControllerTest < ActionController::TestCase + # Replace this with your real tests. + test "the truth" do + assert true + end +end diff --git a/src/test/functional/users_controller_test.rb b/src/test/functional/users_controller_test.rb new file mode 100644 index 0000000..c3db123 --- /dev/null +++ b/src/test/functional/users_controller_test.rb @@ -0,0 +1,8 @@ +require 'test_helper' + +class UsersControllerTest < ActionController::TestCase + # Replace this with your real tests. + test "the truth" do + assert true + end +end diff --git a/src/test/unit/helpers/user_sessions_helper_test.rb b/src/test/unit/helpers/user_sessions_helper_test.rb new file mode 100644 index 0000000..20dabdf --- /dev/null +++ b/src/test/unit/helpers/user_sessions_helper_test.rb @@ -0,0 +1,4 @@ +require 'test_helper' + +class UserSessionsHelperTest < ActionView::TestCase +end diff --git a/src/test/unit/helpers/users_helper_test.rb b/src/test/unit/helpers/users_helper_test.rb new file mode 100644 index 0000000..96af37a --- /dev/null +++ b/src/test/unit/helpers/users_helper_test.rb @@ -0,0 +1,4 @@ +require 'test_helper' + +class UsersHelperTest < ActionView::TestCase +end diff --git a/src/test/unit/user_test.rb b/src/test/unit/user_test.rb new file mode 100644 index 0000000..a64d2d3 --- /dev/null +++ b/src/test/unit/user_test.rb @@ -0,0 +1,8 @@ +require 'test_helper' + +class UserTest < ActiveSupport::TestCase + # Replace this with your real tests. + test "the truth" do + assert true + end +end
Sorry for the delay, had to read up on authlogic to understand what you were doing before I could really review. Mostly good, but NACK because of a few issues (with fixes) and suggestions inline. Snipped parts with no comments. One here that doesn't fit anywhere else though. Since we have no rpm for the gem currently, this will kill rpm builds, so I think we should include an update to the README in this patch explaining that.
On Thu, Sep 24, 2009 at 1:56 PM, Scott Seago sseago@redhat.com wrote:
At the moment you've got to install the authlogic gem -- we will eventually need to package it as an RPM and add it as a dependency in the specfile.
Right now any user can create an account and access all pages. We aren't doing any access control beyond requiring a login account.
Signed-off-by: Scott Seago sseago@redhat.com diff --git a/src/app/views/layouts/_header.rhtml b/src/app/views/layouts/_header.rhtml index 76f9e73..f85510f 100644 --- a/src/app/views/layouts/_header.rhtml +++ b/src/app/views/layouts/_header.rhtml @@ -1,10 +1,10 @@ <div class="header_logo"><%= image_tag "dcloud.png" %></div>
<div class="header_info">
- <div id="hi-username">Hi, <%= @user %></div>
- <div id="hi-username"><%= "Hi, " + @current_user.login if defined? @current_user %></div>
I ran into a problem with the above line (clicking register would blow up), because @current_user is sometimes defined when you would think it should not be, so the template tries to render nil.login. Changing this check to @current_user.login unless @current_user.nil? fixed this issue for me. Also, jp may have another idea of where to put this, but for now, it would be nice to be able to get to the user page (and subsequently the edit page). I was thinking an easy way for now would be to have username be a link to account/
<form method="POST" id="search-form" action="<%= url_for :controller => "search", :action => 'results' %>"> <input id="textfield_effect" name="terms" value="Search" onkeypress="" onfocus="if( this.value == this.defaultValue ) this.value='';" type="text"> <input id="searchbox-button" src="<%= image_path "icon_search.png"%>" title="Search" type="image"> | </form>
- <%= link_to 'Log out', { :controller => "login", :action => "logout"}%>
- <%= link_to 'Log out', { :controller => "user_sessions", :action => "logout"}%>
The above link should toggle between login and logout, as appropriate.
</div> \ No newline at end of file diff --git a/src/app/views/layouts/dcloud.rhtml b/src/app/views/layouts/dcloud.rhtml index 725f630..53dfc67 100644 --- a/src/app/views/layouts/dcloud.rhtml +++ b/src/app/views/layouts/dcloud.rhtml @@ -69,7 +69,7 @@ </div>
<div id="side">
- <%= render :partial => '/layouts/main_nav' %>
- <%= render :partial => '/layouts/main_nav' if defined? @current_user %>
Same change as above, for check to work properly (can see this by the 'add provider' link disappearing), this check should be 'unless @current_user.nil? '
</div>
<div id="tabs-and-content-container"> diff --git a/src/app/views/user_sessions/login.html.erb b/src/app/views/user_sessions/login.html.erb new file mode 100644 index 0000000..1322fa2 --- /dev/null +++ b/src/app/views/user_sessions/login.html.erb @@ -0,0 +1,19 @@ +<div class="dcloud_form">
- <%= error_messages_for 'user_session' %>
This probably isn't strictly in the view template, but in the controller, but testing this is what made me notice- if you have errors, you are not rerouted properly to see them. Instead you get a server error, with the following log:
ActionView::MissingTemplate (Missing template user_sessions/new.erb in view path app/views): /usr/lib/ruby/gems/1.8/gems/locale_rails-2.0.4/lib/locale_rails/action_view.rb:46:in `find_template' app/controllers/user_sessions_controller.rb:15:in `create'
Line 15 of that file is: 'render :action => :new'. It looks like this should go to UserController::new instead, since there is none in UserSessions.
- <h2>Login</h2>
- <% form_for @user_session, :url => user_session_path do |f| %>
- <%= f.error_messages %>
- <%= f.label :login %><br />
- <%= f.text_field :login %><br />
- <br />
- <%= f.label :password %><br />
- <%= f.password_field :password %><br />
- <br />
- <%= f.check_box :remember_me %><%= f.label :remember_me %><br />
- <br />
- <%= f.submit "Login" %>
- <% end %>
- <%= link_to 'Register', {:controller => 'users', :action => 'new'}, :class => 'actionlink' %>
+</div> diff --git a/src/app/views/users/edit.html.erb b/src/app/views/users/edit.html.erb new file mode 100644 index 0000000..7c9db32 --- /dev/null +++ b/src/app/views/users/edit.html.erb @@ -0,0 +1,12 @@ +<div class="dcloud_form">
- <%= error_messages_for 'user' %>
Either this (^^) or the one below needs to go, otherwise summary message is listed twice.
- <h2>Edit My Profile</h2>
- <% form_for @user, :url => account_path do |f| %>
- <%= f.error_messages %>
- <%= render :partial => "form", :object => f %>
- <%= f.submit "Update" %>
- <% end %>
- <br /><%= link_to "My Profile", account_path %>
+</div> diff --git a/src/app/views/users/new.html.erb b/src/app/views/users/new.html.erb new file mode 100644 index 0000000..681d9b0 --- /dev/null +++ b/src/app/views/users/new.html.erb @@ -0,0 +1,10 @@ +<div class="dcloud_form">
- <%= error_messages_for 'user' %>
Both the error dup issue and probably the same failure as noted in login view comments.
- <h2>Register</h2>
- <% form_for @user, :url => account_path do |f| %>
- <%= f.error_messages %>
- <%= render :partial => "form", :object => f %>
- <%= f.submit "Register" %>
- <% end %>
+</div> diff --git a/src/app/views/users/show.html.erb b/src/app/views/users/show.html.erb new file mode 100644 index 0000000..e4e6e67 --- /dev/null +++ b/src/app/views/users/show.html.erb @@ -0,0 +1,37 @@
This is just an opinion, but from a UX standpoint, I think this would look nicer if it were wrapped in the same dcloud_form block as the actual forms, with a heading something like 'My Profile' (at least as first pass). Also, I think it would be more consistent to give the link the 'actionlink' class, as in the forms and nav (we might think about flipping this usage, as 'actionlink currently seems more common than not).
+<p>
- <b>Login:</b>
- <%=h @user.login %>
+</p>
+<p>
- <b>Login count:</b>
- <%=h @user.login_count %>
+</p>
+<p>
- <b>Last request at:</b>
- <%=h @user.last_request_at %>
+</p>
+<p>
- <b>Last login at:</b>
- <%=h @user.last_login_at %>
+</p>
+<p>
- <b>Current login at:</b>
- <%=h @user.current_login_at %>
+</p>
+<p>
- <b>Last login ip:</b>
- <%=h @user.last_login_ip %>
+</p>
+<p>
- <b>Current login ip:</b>
- <%=h @user.current_login_ip %>
+</p>
+<%= link_to 'Edit', edit_account_path %> diff --git a/src/config/routes.rb b/src/config/routes.rb index ae9bc75..d2a171b 100644 --- a/src/config/routes.rb +++ b/src/config/routes.rb @@ -33,8 +33,12 @@ ActionController::Routing::Routes.draw do |map| # -- just remember to delete public/index.html. map.default '', :controller => 'provider'
- map.login '/login', :controller => 'resources', :action => 'list'
- map.logout '/logout', :controller => 'resources', :action => 'list'
- map.resource :user_session
- map.root :controller => "user_sessions", :action => "login"
- map.login 'login', :controller => "user_sessions", :action => "login"
why not map logout the same way, so it doesn't have 'user_sessions' in the url?
- map.resource :account, :controller => "users"
- map.resources :users
# Allow downloading Web Service WSDL as a file with an extension # instead of a file named 'wsdl' diff --git a/src/db/migrate/20090917192602_create_users.rb b/src/db/migrate/20090917192602_create_users.rb new file mode 100644 index 0000000..1bcf0d4 --- /dev/null +++ b/src/db/migrate/20090917192602_create_users.rb @@ -0,0 +1,30 @@ +class CreateUsers < ActiveRecord::Migration
- def self.up
- create_table :users do |t|
- t.string :login, :null => false
- t.string :email, :null => false
- t.string :crypted_password, :null => false
- t.string :password_salt, :null => false
Are salted passwords strong enough encryption? Or is this just temporary? While reading about authlogic, I saw there are several other mechanisms that work with it available, but this could certainly be in a subsequent patch
- t.string :persistence_token, :null => false
- t.string :single_access_token, :null => false
- t.string :perishable_token, :null => false
- # Magic columns, just like ActiveRecord's created_at and updated_at.
- # These are automatically maintained by Authlogic if they are present.
- t.integer :login_count, :null => false, :default => 0
- t.integer :failed_login_count, :null => false, :default => 0
- t.datetime :last_request_at
- t.datetime :current_login_at
- t.datetime :last_login_at
- t.string :current_login_ip
- t.string :last_login_ip
- t.timestamps
- end
- add_index :users, :login
- add_index :users, :persistence_token
- add_index :users, :last_request_at
- end
- def self.down
- drop_table :users
- end
+end diff --git a/src/test/fixtures/users.yml b/src/test/fixtures/users.yml new file mode 100644 index 0000000..5bf0293 --- /dev/null +++ b/src/test/fixtures/users.yml @@ -0,0 +1,7 @@
No tests/fixtures! :( <soapbox> /me thinks it would be good to start building these from the start on this project while things are simple, my errors in testing were a good example of things we could catch with tests </soapbox>
+# Read about fixtures at http://ar.rubyonrails.org/classes/Fixtures.html
+# one: +# column: value +# +# two: +# column: value diff --git a/src/test/functional/user_sessions_controller_test.rb b/src/test/functional/user_sessions_controller_test.rb new file mode 100644 index 0000000..5024b23 --- /dev/null +++ b/src/test/functional/user_sessions_controller_test.rb @@ -0,0 +1,8 @@ +require 'test_helper'
+class UserSessionsControllerTest < ActionController::TestCase
- # Replace this with your real tests.
- test "the truth" do
- assert true
- end
+end
<snip/>
Hope that didn't come off too harsh, didn't expect to have so much to say when I started this review. Even with all that though, this is already far superior, imho, to the path we took with ovirt, would love to see us retrofit this in there at some point.
Forgot to mention one other thing - we don't currently have anywhere to put the success flash messages. Not sure if we want them in the main content area or up in the header like we did for ovirt, but would be nice to sneak them into the general layout, either in this patch, or a subsequent one.
-j
Jason Guiditta wrote:
Sorry for the delay, had to read up on authlogic to understand what you were doing before I could really review. Mostly good, but NACK because of a few issues (with fixes) and suggestions inline. Snipped parts with no comments. One here that doesn't fit anywhere else though. Since we have no rpm for the gem currently, this will kill rpm builds, so I think we should include an update to the README in this patch explaining that.
Yeah I didn't want to go to the trouble of patching the authlogic gem until it was clear that we were going to stick with it. Once that happens we need to submit it for Fedora. Once we start generating official releases, we may need to set up a deltacloud repo -- like we did for ovirt -- for dependencies that are not _yet_ in fedora, etc.
On Thu, Sep 24, 2009 at 1:56 PM, Scott Seago sseago@redhat.com wrote:
At the moment you've got to install the authlogic gem -- we will eventually need to package it as an RPM and add it as a dependency in the specfile.
Right now any user can create an account and access all pages. We aren't doing any access control beyond requiring a login account.
Signed-off-by: Scott Seago sseago@redhat.com diff --git a/src/app/views/layouts/_header.rhtml b/src/app/views/layouts/_header.rhtml index 76f9e73..f85510f 100644 --- a/src/app/views/layouts/_header.rhtml +++ b/src/app/views/layouts/_header.rhtml @@ -1,10 +1,10 @@
<div class="header_logo"><%= image_tag "dcloud.png" %></div>
<div class="header_info"> - <div id="hi-username">Hi, <%= @user %></div> + <div id="hi-username"><%= "Hi, " + @current_user.login if defined? @current_user %></div>
I ran into a problem with the above line (clicking register would blow up), because @current_user is sometimes defined when you would think it should not be, so the template tries to render nil.login. Changing this check to @current_user.login unless @current_user.nil? fixed this issue for me. Also, jp may have another idea of where to put this, but for now, it would be nice to be able to get to the user page (and subsequently the edit page). I was thinking an easy way for now would be to have username be a link to account/
nil? will blow up when defined? is false -- so we'd really need "if defined? @current_user and !@current_user.nil?" to handle both cases.
I hadn't linked to the user/edit page yet for this patch since I was pretty sure the default page as I have it isn't styled properly. I was thinking that we could link to and restyle the user/edit page as necessary in a future patch.
<form method="POST" id="search-form" action="<%= url_for :controller => "search", :action => 'results' %>"> <input id="textfield_effect" name="terms" value="Search" onkeypress="" onfocus="if( this.value == this.defaultValue ) this.value='';" type="text"> <input id="searchbox-button" src="<%= image_path "icon_search.png"%>" title="Search" type="image"> | </form> - <%= link_to 'Log out', { :controller => "login", :action => "logout"}%> + <%= link_to 'Log out', { :controller => "user_sessions", :action => "logout"}%>
The above link should toggle between login and logout, as appropriate.
Good idea. I hadn't put in a login link since pretty much every page requires it -- but the "logged out" page should make it obvious how to log back in.
</div> \ No newline at end of file diff --git a/src/app/views/layouts/dcloud.rhtml b/src/app/views/layouts/dcloud.rhtml index 725f630..53dfc67 100644 --- a/src/app/views/layouts/dcloud.rhtml +++ b/src/app/views/layouts/dcloud.rhtml @@ -69,7 +69,7 @@ </div>
<div id="side"> - <%= render :partial => '/layouts/main_nav' %> + <%= render :partial => '/layouts/main_nav' if defined? @current_user %>
Same change as above, for check to work properly (can see this by the 'add provider' link disappearing), this check should be 'unless @current_user.nil? '
Right -- although we need the same double check for defined and nil
</div>
<div id="tabs-and-content-container"> diff --git a/src/app/views/user_sessions/login.html.erb b/src/app/views/user_sessions/login.html.erb new file mode 100644 index 0000000..1322fa2 --- /dev/null +++ b/src/app/views/user_sessions/login.html.erb @@ -0,0 +1,19 @@ +<div class="dcloud_form"> + <%= error_messages_for 'user_session' %>
This probably isn't strictly in the view template, but in the controller, but testing this is what made me notice- if you have errors, you are not rerouted properly to see them. Instead you get a server error, with the following log:
ActionView::MissingTemplate (Missing template user_sessions/new.erb in view path app/views): /usr/lib/ruby/gems/1.8/gems/locale_rails-2.0.4/lib/locale_rails/action_view.rb:46:in `find_template' app/controllers/user_sessions_controller.rb:15:in `create'
Line 15 of that file is: 'render :action => :new'. It looks like this should go to UserController::new instead, since there is none in UserSessions.
Actually this is a refactoring bug. I changed user_session/new to user_session/login since that made more sense as an action name for a new user session -- but the render line didn't get updated properly.
<h2>Login</h2>
- <% form_for @user_session, :url => user_session_path do |f| %>
- <%= f.error_messages %>
- <%= f.label :login %><br />
- <%= f.text_field :login %><br />
<br />
- <%= f.label :password %><br />
- <%= f.password_field :password %><br />
<br />
- <%= f.check_box :remember_me %><%= f.label :remember_me %><br />
<br />
- <%= f.submit "Login" %>
- <% end %>
- <%= link_to 'Register', {:controller => 'users', :action => 'new'}, :class => 'actionlink' %>
+</div> diff --git a/src/app/views/users/edit.html.erb b/src/app/views/users/edit.html.erb new file mode 100644 index 0000000..7c9db32 --- /dev/null +++ b/src/app/views/users/edit.html.erb @@ -0,0 +1,12 @@ +<div class="dcloud_form">
- <%= error_messages_for 'user' %>
Either this (^^) or the one below needs to go, otherwise summary message is listed twice.
I'll get that change done too.
<h2>Edit My Profile</h2>
- <% form_for @user, :url => account_path do |f| %>
- <%= f.error_messages %>
- <%= render :partial => "form", :object => f %>
- <%= f.submit "Update" %>
- <% end %>
- <br /><%= link_to "My Profile", account_path %>
+</div> diff --git a/src/app/views/users/new.html.erb b/src/app/views/users/new.html.erb new file mode 100644 index 0000000..681d9b0 --- /dev/null +++ b/src/app/views/users/new.html.erb @@ -0,0 +1,10 @@ +<div class="dcloud_form">
- <%= error_messages_for 'user' %>
Both the error dup issue and probably the same failure as noted in login view comments.
<h2>Register</h2>
- <% form_for @user, :url => account_path do |f| %>
- <%= f.error_messages %>
- <%= render :partial => "form", :object => f %>
- <%= f.submit "Register" %>
- <% end %>
+</div> diff --git a/src/app/views/users/show.html.erb b/src/app/views/users/show.html.erb new file mode 100644 index 0000000..e4e6e67 --- /dev/null +++ b/src/app/views/users/show.html.erb @@ -0,0 +1,37 @@
This is just an opinion, but from a UX standpoint, I think this would look nicer if it were wrapped in the same dcloud_form block as the actual forms, with a heading something like 'My Profile' (at least as first pass). Also, I think it would be more consistent to give the link the 'actionlink' class, as in the forms and nav (we might think about flipping this usage, as 'actionlink currently seems more common than not).
Yes this would definitely improve it -- although I think making the view/edit profile pages look better might be better done as a subsequent patch -- also incorporating possible feedback from Jeremy.
+<p>
- <b>Login:</b>
- <%=h @user.login %>
+</p>
+<p>
- <b>Login count:</b>
- <%=h @user.login_count %>
+</p>
+<p>
- <b>Last request at:</b>
- <%=h @user.last_request_at %>
+</p>
+<p>
- <b>Last login at:</b>
- <%=h @user.last_login_at %>
+</p>
+<p>
- <b>Current login at:</b>
- <%=h @user.current_login_at %>
+</p>
+<p>
- <b>Last login ip:</b>
- <%=h @user.last_login_ip %>
+</p>
+<p>
- <b>Current login ip:</b>
- <%=h @user.current_login_ip %>
+</p>
+<%= link_to 'Edit', edit_account_path %> diff --git a/src/config/routes.rb b/src/config/routes.rb index ae9bc75..d2a171b 100644 --- a/src/config/routes.rb +++ b/src/config/routes.rb @@ -33,8 +33,12 @@ ActionController::Routing::Routes.draw do |map| # -- just remember to delete public/index.html. map.default '', :controller => 'provider'
- map.login '/login', :controller => 'resources', :action => 'list'
- map.logout '/logout', :controller => 'resources', :action => 'list'
- map.resource :user_session
- map.root :controller => "user_sessions", :action => "login"
- map.login 'login', :controller => "user_sessions", :action => "login"
why not map logout the same way, so it doesn't have 'user_sessions' in the url?
Sure, why not. It doesn't matter so much to me, but if we provide it for one we ought to provide it for the other in the same way for consistency.
map.resource :account, :controller => "users"
map.resources :users
# Allow downloading Web Service WSDL as a file with an extension # instead of a file named 'wsdl'
diff --git a/src/db/migrate/20090917192602_create_users.rb b/src/db/migrate/20090917192602_create_users.rb new file mode 100644 index 0000000..1bcf0d4 --- /dev/null +++ b/src/db/migrate/20090917192602_create_users.rb @@ -0,0 +1,30 @@ +class CreateUsers < ActiveRecord::Migration
- def self.up
- create_table :users do |t|
t.string :login, :null => false
t.string :email, :null => false
t.string :crypted_password, :null => false
t.string :password_salt, :null => false
Are salted passwords strong enough encryption? Or is this just temporary? While reading about authlogic, I saw there are several other mechanisms that work with it available, but this could certainly be in a subsequent patch
Yeah I'm just sticking with the default for now -- right now the only firm auth requirement is "not kerberos" -- any refinement in our use of authlogic over time shouldn't be a problem.
t.string :persistence_token, :null => false
t.string :single_access_token, :null => false
t.string :perishable_token, :null => false
# Magic columns, just like ActiveRecord's created_at and updated_at.
# These are automatically maintained by Authlogic if they are present.
t.integer :login_count, :null => false, :default => 0
t.integer :failed_login_count, :null => false, :default => 0
t.datetime :last_request_at
t.datetime :current_login_at
t.datetime :last_login_at
t.string :current_login_ip
t.string :last_login_ip
t.timestamps
- end
- add_index :users, :login
- add_index :users, :persistence_token
- add_index :users, :last_request_at
- end
- def self.down
- drop_table :users
- end
+end diff --git a/src/test/fixtures/users.yml b/src/test/fixtures/users.yml new file mode 100644 index 0000000..5bf0293 --- /dev/null +++ b/src/test/fixtures/users.yml @@ -0,0 +1,7 @@
No tests/fixtures! :(
<soapbox> /me thinks it would be good to start building these from the start on this project while things are simple, my errors in testing were a good example of things we could catch with tests </soapbox>
+# Read about fixtures at http://ar.rubyonrails.org/classes/Fixtures.html
+# one: +# column: value +# +# two: +# column: value diff --git a/src/test/functional/user_sessions_controller_test.rb b/src/test/functional/user_sessions_controller_test.rb new file mode 100644 index 0000000..5024b23 --- /dev/null +++ b/src/test/functional/user_sessions_controller_test.rb @@ -0,0 +1,8 @@ +require 'test_helper'
+class UserSessionsControllerTest < ActionController::TestCase
- # Replace this with your real tests.
- test "the truth" do
- assert true
- end
+end
<snip/>
Hope that didn't come off too harsh, didn't expect to have so much to say when I started this review. Even with all that though, this is already far superior, imho, to the path we took with ovirt, would love to see us retrofit this in there at some point.
Yeah ovirt retrofit might be difficult in that we'd have to balance it with the sso requirements for kerberos. It might be that we can have httpd attempt kerberos auth and rely on rails for the fallback rather than the http auth we're doing -- and we'd just need to connect the authlogic back end into either ldap or kerberos for login auth.
I'll work on an updated patch to fix most of the above later today.
Scott
Scott Seago wrote:
On Thu, Sep 24, 2009 at 1:56 PM, Scott Seago sseago@redhat.com wrote:
@@ -1,10 +1,10 @@
<div class="header_logo"><%= image_tag "dcloud.png" %></div>
<div class="header_info"> - <div id="hi-username">Hi, <%= @user %></div> + <div id="hi-username"><%= "Hi, " + @current_user.login if defined? @current_user %></div>
I ran into a problem with the above line (clicking register would blow up), because @current_user is sometimes defined when you would think it should not be, so the template tries to render nil.login. Changing this check to @current_user.login unless @current_user.nil? fixed this issue for me. Also, jp may have another idea of where to put this, but for now, it would be nice to be able to get to the user page (and subsequently the edit page). I was thinking an easy way for now would be to have username be a link to account/
nil? will blow up when defined? is false -- so we'd really need "if defined? @current_user and !@current_user.nil?" to handle both cases.
As Bob just reminded me, I was confusing local var behavior with instance var behavior -- the single .nil? check is sufficient here.
Same change as above, for check to work properly (can see this by the 'add provider' link disappearing), this check should be 'unless @current_user.nil? '
Right -- although we need the same double check for defined and nil
Or not.
Scott
deltacloud-devel@lists.fedorahosted.org