Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: rename config to cas_config in CasClientAuthBackend and CasStaffAuthBackend #36

Merged
merged 1 commit into from
Nov 29, 2022

Conversation

larueli
Copy link
Contributor

@larueli larueli commented Nov 18, 2022

Symptom

When updating to osTicket 1.17, several users got the following error : Fatal error: Cannot redeclare non static AuthenticationBackend::$config as static CasStaffAuthBackend::$config in phar://C:/include/plugins/auth-cas.phar/cas.php. See issue #35

Cause

After investigating, it appears that 6 month ago during the development of the multi-instance feature, the following commit was issued to the core osTicket repo : osTicket/osTicket@0b93b48. This commit created a new protected attribute config for the AuthenticationBackend class.

However, in the auth-cas plugin, the CasStaffAuthBackend and CasClientAuthBackend both have an already declared config attribute since 7 years, and extends respectively osTicket classes ExternalStaffAuthenticationBackend and ExternalUserAuthenticationBackend, which in turn extends respectively StaffAuthenticationBackend and UserAuthenticationBackend which in turn extends AuthenticationBackend which had the new config attribute. This attribute being only protected, it went all the way down to our classes : CasStaffAuthBackend and CasClientAuthBackend, conflicting with their config attribute.

Solution

Our attribute being private, the easiest solution I have thinked of is changing the name of our variable config to cas_config in our two classes CasStaffAuthBackend and CasClientAuthBackend. Because they are private, they can only be accessed within the source code of theses two classes, so the scope of the fix keeps limited and simple.

Tests

Phar successfully rebuilt and works under osticket v1.17

Additional question

@kevinoconnor7 I can try to provide some fixes or updates as I already done in #33 while being a maintainer of this repo with you if you wish.

I would like to try this things :

  • update dependancies / check link with the https://github.com/osTicket/osTicket-plugins repo. Because of it we are stuck to https://github.com/apereo/phpCAS 1.3.8 whereas the 1.6.0 has just been released
  • relaunching releases process (bump new versions, ensuring cicleci still working or moving to github actions or whatever),
  • provide issues and PR templates
  • check the possibility of disabling the login form for client and keep only the cas button
  • check issues and fix what can be fixed

That would be already enough for now !

@larueli
Copy link
Contributor Author

larueli commented Nov 25, 2022

@kevinoconnor7 Maybe you haven't still seen this PR ? Tell me if you need help to understand it or change something !

@kevinoconnor7
Copy link
Owner

kevinoconnor7 commented Nov 25, 2022 via email

@larueli
Copy link
Contributor Author

larueli commented Nov 25, 2022

No worries, I understand, it's also a side project for me. It was just to make sure that you were still watching this repo ;)

For those who want to build the phar by hand immediatly with the current PR (you should only build locally, not on your production instance for safety reasons -> https://www.php.net/manual/en/phar.configuration.php) :

mkdir test
cd test
git clone https://github.com/larueli/osTicket-auth-cas.git
git clone https://github.com/osTicket/osTicket-plugins.git
rm -rf osTicket-plugins/auth-cas
cp -rf osTicket-auth-cas/auth-cas osTicket-plugins/auth-cas
cd osTicket-plugins
php8.0 -dphar.readonly=0 make.php build auth-cas

You'll find the auth-cas.phar in the current folder, and send it to your server (make sure to make a full backup first).

@kevinoconnor7 kevinoconnor7 merged commit 4fe353d into kevinoconnor7:master Nov 29, 2022
@kevinoconnor7
Copy link
Owner

Cut a new release: https://github.com/kevinoconnor7/osTicket-auth-cas/releases/tag/v1.2.2

Sorry.. took a bit of effort. I update/release so infrequently that something is either broken or some access token expired.

Thank you for your help on this! Your detailed description made this very easy to review and was very much appreciated.

@larueli
Copy link
Contributor Author

larueli commented Nov 29, 2022

Thanks a lot !

As I said in my first message, if you're looking for a maintainer, I'd like to help you on my free time.

The issue #35 is now solved and can be closed.

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