Merge pull request #11717 from neinteractiveliterature/fix/become-user-pkce-auth

Fix become/revert_become after PKCE auth switch

Author: nbudin

app/controllers/concerns/oauth_session_management.rb

@@ -0,0 +1,47 @@
+# frozen_string_literal: true
+
+# Shared logic for managing the SPA's OAuth session cookie across controllers
+# that need to swap the active user (e.g. the "become user" feature).
+module OAuthSessionManagement
+  REFRESH_COOKIE_NAME = "__Host-intercode_refresh"
+
+  private
+
+  def issue_oauth_session_for_user(user)
+    frontend_app = OAuthApplication.find_by(is_intercode_frontend: true)
+    return unless frontend_app
+
+    revoke_current_oauth_session
+
+    new_token = create_frontend_access_token(user, frontend_app)
+    cookies[REFRESH_COOKIE_NAME] = {
+      value: new_token.plaintext_refresh_token,
+      httponly: true,
+      secure: true,
+      same_site: :strict,
+      path: "/",
+      max_age: 90.days.to_i
+    }
+  end
+
+  def create_frontend_access_token(user, frontend_app)
+    scopes = Doorkeeper.config.scopes.to_s
+    # Mirror the custom_access_token_expires_in block in config/initializers/doorkeeper.rb
+    expires_in =
+      Doorkeeper::OAuth::Scopes.from_string(scopes).any? { |s| s.start_with?("manage_") } ? 30.minutes : 2.weeks
+    Doorkeeper::AccessToken.create!(
+      application: frontend_app,
+      resource_owner_id: user.id,
+      scopes: scopes,
+      expires_in: expires_in,
+      use_refresh_token: true
+    )
+  end
+
+  def revoke_current_oauth_session
+    refresh_value = cookies[REFRESH_COOKIE_NAME]
+    return if refresh_value.blank?
+
+    Doorkeeper::AccessToken.by_refresh_token(refresh_value)&.revoke
+  end
+end

app/controllers/user_con_profiles_controller.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 class UserConProfilesController < ApplicationController
   include SendCsv
+  include OAuthSessionManagement
 
   # Normally we'd just use the name of the resource as the instance variable name.  Here that'd be
   # @user_con_profile, which is unsafe for us to use because ApplicationController uses it to mean
@@ -12,15 +13,15 @@ class UserConProfilesController < ApplicationController
 
   def become
     identity_assumer = assumed_identity_from_profile || user_con_profile
-    subject_profile = convention.user_con_profiles.find(params[:id])
+    subject_profile = convention.user_con_profiles.find(params.expect(:id))
     authorize subject_profile, :become?
 
     new_session =
       AssumedIdentitySession.new(
         assumed_profile: subject_profile,
         assumer_profile: identity_assumer,
         justification: params[:justification],
-        started_at: Time.now
+        started_at: Time.current
       )
 
     unless new_session.save
@@ -31,6 +32,7 @@ def become
     sign_in subject_profile.user
     session[:assumed_identity_from_profile_id] = identity_assumer.id
     session[:assumed_identity_session_id] = new_session.id
+    issue_oauth_session_for_user(subject_profile.user)
     redirect_to root_url
   end
 
@@ -40,8 +42,10 @@ def revert_become
         redirect_to(
           root_url,
           alert:
+            # rubocop:disable Rails/I18nLocaleTexts
             "You haven't assumed an identity, so you can't revert \
 back to your normal identity (since you already are your normal identity)."
+          # rubocop:enable Rails/I18nLocaleTexts
         )
       )
     end
@@ -52,7 +56,8 @@ def revert_become
     sign_in regular_user_con_profile.user
     session.delete(:assumed_identity_from_profile_id)
     session.delete(:assumed_identity_session_id)
-    current_session&.update!(finished_at: Time.now) if current_session
+    current_session&.update!(finished_at: Time.current)
+    issue_oauth_session_for_user(regular_user_con_profile.user)
     redirect_to root_url
   end
 

app/javascript/NavigationBar/UserNavigationSection.tsx

@@ -111,10 +111,17 @@ const LoggedInDropdownTarget = forwardRef<HTMLButtonElement, LoggedInDropdownTar
 
 function RevertAssumedIdentityButton() {
   const { assumedIdentityFromProfile } = useContext(AppRootContext);
+  const authenticationManager = useContext(AuthenticationManagerContext);
 
   const revertAssumedIdentity = async () => {
+    const token = await authenticationManager.ensureFreshAccessToken();
+    const headers: Record<string, string> = {};
+    if (token) {
+      headers['Authorization'] = `Bearer ${token}`;
+    }
     const response = await htmlFetch('/user_con_profiles/revert_become', {
       method: 'POST',
+      headers,
     });
 
     window.location.href = response.url;

app/javascript/UserConProfiles/UserConProfileAdminDisplay.tsx

@@ -1,4 +1,4 @@
-import { Suspense, useMemo, useState } from 'react';
+import { Suspense, useContext, useMemo, useState } from 'react';
 import {
   ActionFunction,
   Link,
@@ -34,6 +34,7 @@ import { DeleteUserConProfileDocument } from './mutations.generated';
 import invariant from 'tiny-invariant';
 import { UserConProfile } from 'graphqlTypes.generated';
 import { apolloClientContext } from 'AppContexts';
+import { AuthenticationManagerContext } from '../Authentication/authenticationManager';
 
 export const action: ActionFunction<RouterContextProvider> = async ({ context, request, params: { id } }) => {
   const client = context.get(apolloClientContext);
@@ -59,16 +60,19 @@ export const action: ActionFunction<RouterContextProvider> = async ({ context, r
   }
 };
 
-async function becomeUser(userConProfileId: string, justification: string) {
+async function becomeUser(userConProfileId: string, justification: string, token?: string) {
   const formData = new FormData();
   formData.append('justification', justification);
 
+  const headers: Record<string, string> = { Accept: 'application/json' };
+  if (token) {
+    headers['Authorization'] = `Bearer ${token}`;
+  }
+
   const response = await fetch(`/user_con_profiles/${userConProfileId}/become`, {
     method: 'POST',
     credentials: 'include',
-    headers: {
-      Accept: 'application/json',
-    },
+    headers,
     body: formData,
   });
 
@@ -98,6 +102,7 @@ function BecomeUserModal({
   close,
 }: BecomeUserModalProps): React.JSX.Element {
   const { t } = useTranslation();
+  const authenticationManager = useContext(AuthenticationManagerContext);
   const [justification, setJustification] = useState('');
   const [becomeAsync, error, inProgress] = useAsyncFunction(becomeUser);
 
@@ -106,7 +111,8 @@ function BecomeUserModal({
       return;
     }
 
-    await becomeAsync(userConProfileId, justification);
+    const token = await authenticationManager.ensureFreshAccessToken();
+    await becomeAsync(userConProfileId, justification, token);
     close();
   };
 

test/controllers/user_con_profiles_controller_test.rb

@@ -1,4 +1,6 @@
-require 'test_helper'
+# frozen_string_literal: true
+# rubocop:disable Metrics/BlockLength
+require "test_helper"
 
 describe UserConProfilesController do
   let(:user_con_profile) { create(:user_con_profile) }
@@ -10,13 +12,71 @@
     profile
   end
   let(:con_admin) { con_admin_profile.user }
+  let(:frontend_app) { create(:oauth_application, is_intercode_frontend: true) }
 
   setup do
     set_convention convention
     sign_in con_admin
-
     user_con_profile
+    frontend_app
   end
 
-  # TODO write tests for become/revert_become
+  describe "POST become" do
+    it "creates an assumed identity session and issues an OAuth session for the assumed user" do
+      OAuthApplication.stub(:find_by, frontend_app) do
+        assert_difference("AssumedIdentitySession.count", 1) do
+          assert_difference("Doorkeeper::AccessToken.count", 1) do
+            post :become, params: { id: user_con_profile.id, justification: "testing become" }
+          end
+        end
+      end
+
+      assert_redirected_to root_url
+
+      new_token = Doorkeeper::AccessToken.last
+      assert_equal user_con_profile.user.id, new_token.resource_owner_id
+      assert_equal frontend_app.id, new_token.application_id
+    end
+
+    it "revokes the admin's previous OAuth session cookie when one exists" do
+      admin_token =
+        Doorkeeper::AccessToken.create!(
+          application: frontend_app,
+          resource_owner_id: con_admin.id,
+          scopes: "public",
+          expires_in: 2.weeks,
+          use_refresh_token: true
+        )
+      @request.cookies[OAuthSessionManagement::REFRESH_COOKIE_NAME] = admin_token.plaintext_refresh_token
+
+      OAuthApplication.stub(:find_by, frontend_app) do
+        post :become, params: { id: user_con_profile.id, justification: "testing cookie revocation" }
+      end
+
+      assert admin_token.reload.revoked?, "Expected the admin's previous OAuth token to be revoked"
+    end
+  end
+
+  describe "POST revert_become" do
+    setup do
+      OAuthApplication.stub(:find_by, frontend_app) do
+        post :become, params: { id: user_con_profile.id, justification: "setup for revert test" }
+      end
+    end
+
+    it "reverts to the original admin user and issues a new OAuth session for them" do
+      assumed_user_token = Doorkeeper::AccessToken.last
+
+      OAuthApplication.stub(:find_by, frontend_app) do
+        assert_difference("Doorkeeper::AccessToken.count", 1) { post :revert_become }
+      end
+
+      assert_redirected_to root_url
+
+      new_token = Doorkeeper::AccessToken.last
+      assert_equal con_admin.id, new_token.resource_owner_id
+      assert assumed_user_token.reload.revoked?, "Expected the assumed user's OAuth token to be revoked after revert"
+    end
+  end
 end
+# rubocop:enable Metrics/BlockLength