Skip to content

refactor session#570

Open
simonLeary42 wants to merge 9 commits intomainfrom
refactor-session
Open

refactor session#570
simonLeary42 wants to merge 9 commits intomainfrom
refactor-session

Conversation

@simonLeary42
Copy link
Member

@simonLeary42 simonLeary42 commented Jan 26, 2026

The $_SESSION keys is_pi, user_exists, is_admin are used so that the navbar behaves properly on non-authenticated pages:

// $_SERVER["REMOTE_USER"] is only defined for pages where httpd requies authentication
// the home page does not require authentication,
// so if the user goes to a secure page and then back to home, they've effectively logged out
// it would be bad UX to show the user that they are effectively logging in and out,
// so we use session cache to remember if they have logged in recently and then pretend
// they're logged in even if they aren't

I think the names should be more specific. is_admin could intuitively be used for authorization, even though the user may be logged out. navbar_show_admin_pages is a hint that the user is probably an admin.

There is also some unnecessarily confusing behavior that I have changed. When you view as another user, $_SESSION["is_admin"] no longer reflects the currently logged in user. It is used for authorization even though it doesn't really need to be, and then the admin links on the navbar are never displayed while viewing as another user. Instead, I have made it so $_SESSION["is_admin"] always reflects the current user. Some authorization using is_admin has been removed, see comments below.

I also removed unused variables $_SESSION["SSO"] and $OPERATOR

Comment on lines 163 to +164
echo "</div>";
if (
isset($_SESSION["is_admin"])
&& $_SESSION["is_admin"]
&& isset($_SESSION["viewUser"])
) {
if (isset($_SESSION["viewUser"])) {
Copy link
Member Author

@simonLeary42 simonLeary42 Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Authorization not needed here because authorization is already required to set $_SESSION["viewUser"] and $_SESSION["viewUser"] gets cleaned up properly

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors session variable naming to be more descriptive and changes the behavior of session flags when viewing as another user. The main changes aim to make session variable names clearer (e.g., is_adminnavbar_show_admin_pages) and to ensure that admin navigation links are always shown when an admin is logged in, even when viewing as another user.

Changes:

  • Renamed session variables from user_exists, is_admin, is_pi to navbar_show_logged_in_user_pages, navbar_show_admin_pages, navbar_show_pi_pages
  • Removed the $OPERATOR variable (now only stored in session, not as an object)
  • Removed authorization check from the clearView form handler
  • Simplified the user existence check in the redirect logic
  • Removed commented-out assertion in ViewAsUserTest

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
resources/init.php Refactored session variable initialization, removed UserFlag import, simplified USER creation logic
resources/templates/header.php Updated to use new session variable names, removed authorization checks from clearView handler, simplified navbar display logic
resources/templates/home.php Updated to use new session variable name
test/functional/ViewAsUserTest.php Removed commented-out assertion

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@simonLeary42 simonLeary42 marked this pull request as ready for review January 26, 2026 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants