Omniauth: Add after_login, remove on_success

After a successfull login seems to be a much clearer description of
when the callback will be executed than "on successful authorization".
E.g. it's not clear whether ending up on the registration page also
indicates successful authorization (which it does in the OmniAuth
sense, but not in the OpenProject sense).

The after login callback is called each time a user is logged in, no
matter whether that happens via regular Omniauth login or after
completing the registration process and being automatically logged in
(when automated account activation is enabled).

app/controllers/account_controller.rb

@@ -316,8 +316,6 @@ class AccountController < ApplicationController
   def register_by_email_activation(user, opts = {})
     token = Token.new(:user => user, :action => "register")
     if user.save and token.save
-      opts[:on_success].call user if opts[:on_success]
-
       UserMailer.user_signed_up(token).deliver
       flash[:notice] = l(:notice_account_register_done)
       redirect_to :action => 'login'
@@ -335,9 +333,9 @@ class AccountController < ApplicationController
     user.last_login_on = Time.now
 
     if user.save
-      opts[:on_success].call user if opts[:on_success]
-
       self.logged_user = user
+      opts[:after_login].call user if opts[:after_login]
+
       flash[:notice] = l(:notice_account_registered_and_logged_in)
       redirect_after_login(user)
     else
@@ -350,8 +348,6 @@ class AccountController < ApplicationController
   # Pass a block for behavior when a user fails to save
   def register_manually_by_administrator(user, opts = {})
     if user.save
-      opts[:on_success].call user if opts[:on_success]
-
       # Sends an email to the administrators
       admins = User.admin.active
       admins.each do |admin|

app/controllers/concerns/omniauth_login.rb

@@ -53,7 +53,7 @@ module Concerns::OmniauthLogin
     else
       if user.active?
         user.log_successful_login
-        OpenProject::OmniAuth::Authorization.authorized! user, auth_hash
+        OpenProject::OmniAuth::Authorization.after_login! user, auth_hash
       end
       login_user_if_active(user)
     end
@@ -77,7 +77,7 @@ module Concerns::OmniauthLogin
 
     fill_user_fields_from_omniauth user, auth_hash
 
-    opts = { on_success: ->(u) { OpenProject::OmniAuth::Authorization.authorized! u, auth_hash } }
+    opts = { after_login: ->(u) { OpenProject::OmniAuth::Authorization.after_login! u, auth_hash } }
 
     # Create on the fly
     register_user_according_to_setting(user, opts) do
@@ -99,7 +99,7 @@ module Concerns::OmniauthLogin
     fill_user_fields_from_omniauth(user, auth)
     user.update_attributes(permitted_params.user_register_via_omniauth)
 
-    opts = { on_success: ->(u) { OpenProject::OmniAuth::Authorization.authorized! u, auth } }
+    opts = { after_login: ->(u) { OpenProject::OmniAuth::Authorization.after_login! u, auth } }
     register_user_according_to_setting user, opts
   end
 

lib/open_project/omni_auth/authorization.rb

@@ -21,12 +21,12 @@ module OpenProject
       end
 
       ##
-      # Signals that the given user has been successfully authorized.
+      # Signals that the given user has been logged in.
       #
       # Note: Only call if you know what you are doing.
-      def self.authorized!(user, auth_hash)
-        authorized_callbacks.each do |callback|
-          callback.authorized user, auth_hash
+      def self.after_login!(user, auth_hash)
+        after_login_callbacks.each do |callback|
+          callback.after_login user, auth_hash
         end
       end
 
@@ -67,12 +67,16 @@ module OpenProject
       end
 
       ##
-      # Registers a callback on the event of a successful user authorization.
+      # Registers a callback on the event of a successful login.
       #
-      # @yield [user] Callback called with the successfully authorized user.
-      # @yieldparam user [User] User who has been authorized.
-      def self.on_success(&block)
-        add_authorized_callback AuthorizedBlockCallback.new(&block)
+      # Called directly after logging in.
+      # This usually happens when the user logged in normally or was logged in
+      # automatically after on-the-fly registration via automated account activation.
+      #
+      # @yield [user] Callback called with the successfully logged in user.
+      # @yieldparam user [User] User who has been logged in.
+      def self.after_login(&block)
+        add_after_login_callback AfterLoginBlockCallback.new(&block)
       end
 
       ##
@@ -88,15 +92,15 @@ module OpenProject
       end
 
       ##
-      # Registers a new callback to successful user authorization.
+      # Registers a new callback to successful user login.
       #
-      # @param [AuthorizedCallback] Callback to be called upon successful authorization.
-      def self.add_authorized_callback(callback)
-        authorized_callbacks << callback
+      # @param [AfterLoginCallback] Callback to be called upon successful authorization.
+      def self.add_after_login_callback(callback)
+        after_login_callbacks << callback
       end
 
-      def self.authorized_callbacks
-        @authorized_callbacks ||= []
+      def self.after_login_callbacks
+        @after_login_callbacks ||= []
       end
 
       ##
@@ -132,28 +136,28 @@ module OpenProject
       end
 
       ##
-      # A callback for reacting to a user being authorized.
-      class AuthorizedCallback
+      # A callback for reacting to a user being logged in.
+      class AfterLoginCallback
         ##
-        # Is called after a user has been authorized successfully.
+        # Is called after a user has been logged in successfully.
         #
-        # @param [User] User who has been authorized.
+        # @param [User] User who has been logged in.
         # @param [Omniauth::AuthHash] Omniauth authentication info including credentials.
-        def authorized(user, auth_hash)
-          fail "subclass responsibility: authorized(#{user}, #{auth_hash})"
+        def after_login(user, auth_hash)
+          fail "subclass responsibility: after_login(#{user}, #{auth_hash})"
         end
       end
 
       ##
-      # A authorized callback triggering a given block.
-      class AuthorizedBlockCallback < AuthorizedCallback
+      # A after_login callback triggering a given block.
+      class AfterLoginBlockCallback < AfterLoginCallback
         attr_reader :block
 
         def initialize(&block)
           @block = block
         end
 
-        def authorized(user, auth_hash)
+        def after_login(user, auth_hash)
           block.call user, auth_hash
         end
       end

spec/controllers/concerns/omniauth_login_spec.rb

@@ -96,9 +96,10 @@ describe AccountController do
         end
 
         it 'registers user via post' do
-          expect(OpenProject::OmniAuth::Authorization).to receive(:authorized!) do |user|
+          expect(OpenProject::OmniAuth::Authorization).to receive(:after_login!) do |user, auth_hash|
             new_user = User.find_by_login('login@bar.com')
             expect(user).to eq new_user
+            expect(auth_hash).to include(omniauth_hash)
           end
 
           auth_source_registration = omniauth_hash.merge(
@@ -263,7 +264,7 @@ describe AccountController do
           end
 
           it 'works' do
-            expect(OpenProject::OmniAuth::Authorization).to receive(:authorized!) do |u, auth|
+            expect(OpenProject::OmniAuth::Authorization).to receive(:after_login!) do |u, auth|
               expect(u).to eq user
               expect(auth).to eq omniauth_hash
             end
@@ -279,7 +280,7 @@ describe AccountController do
             end
 
             it 'is rejected against google' do
-              expect(OpenProject::OmniAuth::Authorization).not_to receive(:authorized!).with(user)
+              expect(OpenProject::OmniAuth::Authorization).not_to receive(:after_login!).with(user)
 
               post :omniauth_login
 
@@ -288,7 +289,7 @@ describe AccountController do
             end
 
             it 'is rejected against any other provider too' do
-              expect(OpenProject::OmniAuth::Authorization).not_to receive(:authorized!).with(user)
+              expect(OpenProject::OmniAuth::Authorization).not_to receive(:after_login!).with(user)
 
               omniauth_hash.provider = 'any other'
               post :omniauth_login
@@ -306,7 +307,7 @@ describe AccountController do
             end
 
             it 'is rejected against google' do
-              expect(OpenProject::OmniAuth::Authorization).not_to receive(:authorized!).with(user)
+              expect(OpenProject::OmniAuth::Authorization).not_to receive(:after_login!).with(user)
 
               post :omniauth_login
 
@@ -315,7 +316,7 @@ describe AccountController do
             end
 
             it 'is approved against any other provider' do
-              expect(OpenProject::OmniAuth::Authorization).to receive(:authorized!) do |u|
+              expect(OpenProject::OmniAuth::Authorization).to receive(:after_login!) do |u|
                 new_user = User.find_by_identity_url 'some other:123545'
 
                 expect(u).to eq new_user
@@ -333,7 +334,7 @@ describe AccountController do
 
             # ... and to confirm that, here's what happens when the authorization fails
             it 'is rejected against any other provider with the wrong email' do
-              expect(OpenProject::OmniAuth::Authorization).not_to receive(:authorized!).with(user)
+              expect(OpenProject::OmniAuth::Authorization).not_to receive(:after_login!).with(user)
 
               omniauth_hash.provider = 'yet another'
               config.global_email = 'yarrrr@joro.es'

spec/lib/open_project/omni_auth/authorization_spec.rb

@@ -29,30 +29,30 @@
 require 'spec_helper'
 
 describe OpenProject::OmniAuth::Authorization do
-  describe '.authorized!' do
+  describe '.after_login!' do
     let(:auth_hash) { Struct.new(:uid).new 'bar' }
     let(:user)  { FactoryGirl.create :user, mail: 'foo@bar.de' }
     let(:state) { Struct.new(:number, :user_email, :uid).new 0, nil, nil }
 
     before do
-      OpenProject::OmniAuth::Authorization.authorized_callbacks.clear
+      OpenProject::OmniAuth::Authorization.after_login_callbacks.clear
 
-      OpenProject::OmniAuth::Authorization.on_success do |_, _|
+      OpenProject::OmniAuth::Authorization.after_login do |_, _|
         state.number = 42
       end
 
-      OpenProject::OmniAuth::Authorization.on_success do |user, auth|
+      OpenProject::OmniAuth::Authorization.after_login do |user, auth|
         state.user_email = user.mail
         state.uid = auth.uid
       end
     end
 
     after do
-      OpenProject::OmniAuth::Authorization.authorized_callbacks.clear
+      OpenProject::OmniAuth::Authorization.after_login_callbacks.clear
     end
 
     it 'triggers every callback setting uid to "bar", number to 42 and user_email to foo@bar.de' do
-      OpenProject::OmniAuth::Authorization.authorized! user, auth_hash
+      OpenProject::OmniAuth::Authorization.after_login! user, auth_hash
 
       expect(state.number).to eq 42
       expect(state.user_email).to eq 'foo@bar.de'