onelogin / joomla-saml

Joomla 3.3 SAML plugin based on the OneLogin SAML toolkit
MIT License
14 stars 10 forks source link

Moving configuration settings into admin side component #7

Open Michandrz opened 4 years ago

Michandrz commented 4 years ago

I'd like to work on a component that would configure the plugin and move a lot of the configs into the database. That way we're not limiting attribute mapping and setting up for the future.

pitbulk commented 3 years ago

Hi @Michandrz I want to hear more about this. Were you able to start this improvement?

Michandrz commented 3 years ago

@pitbulk sorry for all the pings. I did, but got sidetracked with life. The code I started is here: https://github.com/Michandrz/joomla-saml/tree/com_onelogin

As it sits right now the admin UI is started, but I need to go back and make the plugin read from the database for the config.

Michandrz commented 3 years ago

So, after a deep dive into the Jooma code, it seems like all the things this plugin is trying to accomplish would be better served as a package and fit more into the Jooma standard of code.

A lot of the user login/out process (namely redirect and forms) should really be handled by a site-side component. Most of the data manipulation of the user object should be happening in a shared model. Then there is the admin side component witch I've started on, but I don't think my database driven approach follows the Jooma standard for configurations. Maybe a system plugin that handles talking to the IDP if not integrating that into a component. Depending on thought and approach a content plugin that modifies the system login forms or a couple modules to just replace them. On top of that, I need to look and see if there is a better way to prevent password login of an SSO user as just setting a random password seems insecure to me vs only allowing login of an SSO user by the IDP. In fact, I could see several scenarios where this would come to being a rather large security hole. (edit: found a fix for this, opening a new issue)

Then that leaves this plugin, which would at that point would not need to do much, if anything, because most of the function would be moved elsewhere and all we would really need to do is trigger the system events for clearing out cookies and such. which, I noticed isn't actually happening right now, because currently the plugin redirects before the joomla dispatcher is able to finish running all the onUserLogout event registers.

Thoughts anyone else?

Michandrz commented 3 years ago

So without comment I've done a lot to this extent. Still getting a few bugs that I need to work out, but it /somewhat/ works through login and local logout. Need to get IDP logout setup and figure out a few redirects.

pitbulk commented 3 years ago

Hi @Michandrz,

I plan to do a complete rework of the extension in January.

Take in mind that the extension was created on 2014 so there were a lot of change in Joomla side that were not reflected on the extension. I worked to make it compatible with new versions but never made a rewrite to use the new features available at Joomla.

I will use your extension as base/inspiration.

I was looking for a Joomla expert to suggest me the way to improve the extension, how build the new file scheme, to handle configuration, ..

I also plan to add SAML support for the admin site as well.

Michandrz commented 3 years ago

I plan to do a complete rework of the extension in January.

That's pretty much what I have done using your original as a concept and adapting it as needed.

Take in mind that the extension was created on 2014 so there were a lot of change in Joomla side that were not reflected on the extension.

I worked to make it compatible with new versions but never made a rewrite to use the new features available at Joomla. Yeah, I believe all of the code I have written is compatible with the 3.x system, however, in my reading it seems like you can't reliably have a 3.x and 4.x extension without a lot of work. My thoughts were to perhaps continue the 1.x line of the saml code for Joomla 3.x and then a 2.x version of the Saml code for joomla 4.x Most of the work would just be adding namespaces and figuring out the new joomla event handler. Which I'd be more than happy to help with.

I will use your extension as base/inspiration.

I was looking for a Joomla expert to suggest me the way to improve the extension, how build the new file scheme, to handle configuration, ..

I also plan to add SAML support for the admin site as well. With the rewrite that I started, it should theoretically work with just another admin side controller to actually handle the login. I've actually added a handful of features as I've been coding.

While I wouldn't consider myself a Joomla expert, I'm more than happy to help where I can.

pitbulk commented 3 years ago

@Michandrz Do you want to review and test the work done on the 2.0.0 branch?

I had to re-implement the plugins to use "system plugins" instead "users plugins" in order to be able to inject properly the SAML Link or be able to force the SAML login process.

I refactored the OneLogin Library to use the version 3.5.1 and take the code from a composer installation to make easier its maintenance.

I refactored some classes and methods and added new ones in order to support the new features.

Michandrz commented 3 years ago

I'd be happy to test it, but just reading through the code I have some concerns.

I don't think you can have a system plgugin with User, and Content plugin calls. They don't run reliably as you actually specify what types of plugins you want when you are making those dispatches. for example this code:

PluginHelper::importPlugin('user'); $app = Factory::getApplication(); $app->triggerEvent('onUserUpdateInfo', array()); will only load PlgUser* and then execute the onUserUpdateInfo function in any of those plugins. If it runs other system plugin or content plugin code, I believe this is only by chance and not reliable.

joomla-saml/onelogin/saml_joomla.php: the functions related to managing users already exist in Jooma checkout Joomla\CMS\User\User and Joomla\CMS\User\UserHelper classes or for a more direct example: https://github.com/Michandrz/joomla-saml/blob/devel/com_oneloginsaml/admin/models/user.php

You're really fighting the Joomla code base instead of working within it. I did a lot of work here: https://github.com/Michandrz/joomla-saml/tree/devel that is more dependent on the Jooma library/style/flow.

the goal for me was to remove a lot of the "out of standard" and especially the out of application code. (direct calling the script). I moved a lot of the data management into models. the controller handles the redirects to the SSO and catching the return to start the login process. The Authentication plugin actually handles the login via extending the standard $app->logon(); function. I still have work to do on it, but I think this should be the way going forward as it is less fighting the joomla code base and more working within it. Let me know what you think.

As far as the admin side, I really need to do some research to figure out what the admin application and com_login in the admin class act as. Just from memory I know it uses the same $app->login(); function, but I'm not sure what the differences are in the siteApplication and adminApplication classes. I'll probably have some time later this week to look at it.

pitbulk commented 3 years ago

I tested the plugin and the SSO, user creation/update and SLO works.

A User plugin only gets a subset of the triggers (only the events related to the user), but a system plugin get all of them. I made that change at the end in order to be able to inject the login link in all the views. I could create a user plugin for user triggers and system plugin for content triggers but that means 4 plugins instead 4 and I believe adds complexity to the admin.

This plugin for example: https://github.com/akeeba/sociallogin defines a system plugin in order to handle content and user login actions.

Related to the models you created, I totally missed that refactor, I took what it was in the PR that was merged. But I did not make big changes on joomla_saml.php code (that code should go on the Onelogin library since is used by both plugins site/admin). Maybe you can drop the models/users.php there and apply the required changes on joomla_saml.php

Michandrz commented 3 years ago

A User plugin only gets a subset of the triggers (only the events related to the user), but a system plugin get all of them. I made that change at the end in order to be able to inject the login link in all the views. I could create a user plugin for user triggers and system plugin for content triggers but that means 4 plugins instead 4 and I believe adds complexity to the admin.

What I am saying is that I believe this is true for system plugins as well. Except they only get called for system type functions. see https://docs.joomla.org/Plugin/Events/System

This plugin for example: https://github.com/akeeba/sociallogin defines a system plugin in order to handle content and user login actions.

that app uses the system plugin to set common vars, but delegates the heavy lifting off to sub-plugins for each login format(at least that's what I gathered from a 30 second read and then uses modules to actually make things appear.

Related to the models you created, I totally missed that refactor, I took what it was in the PR that was merged. But I did not make big changes on joomla_saml.php code (that code should go on the Onelogin library since is used by both plugins site/admin). Maybe you can drop the models/users.php there and apply the required changes on joomla_saml.php

Yeah, I branched away from master as it was such a huge code change(mostly moving) that I didn't want that to be the pull request.

model/users.php is in the component, because it relies on JModel(now Joomla\CMS\MVC\Model\BaseDatabaseModel ) and because that's the Joomla Standard, data manipulation is handled in models. Also, the library in my factoring of the code is a copy of the php-saml with a joomla wrapper that is just passing the config to the library in the format it likes.(see high level overview below).

It may seem like it adds admin complexity, but splitting across multiple plugins is definitely the appropriate way of doing it. Just take a look at the standard joomla plugins for source reference.

What I have written at https://github.com/Michandrz/joomla-saml/tree/devel is actually much more integrated into joomla and attempting to work within the joomla application.

If you look at the standard joomla login process, the user fills out a form which gets submitted to this controller https://github.com/joomla/joomla-cms/blob/staging/components/com_users/controllers/user.php which pulls the logon data and calls on the applications logon function(Line 112 of the previous file). The application's logon function(https://github.com/joomla/joomla-cms/blob/16d0f312a4dd4d7a0b98768507ad3a5d451247f1/libraries/src/Application/CMSApplication.php#L788) then sends the login details to the PlgAuthentication plugins (this happens in the JAuthorization class available here: https://github.com/joomla/joomla-cms/blob/16d0f312a4dd4d7a0b98768507ad3a5d451247f1/libraries/src/Authentication/Authentication.php#L277) to authenticate and authorize the user before setting local session variables and the cookie.

With that being said, here is where I'd like to get the michandrz/joomla-saml/tree/devel code to:

Using the content plugin to add appropriate saml login links. Those links are to the com_onelogin site component(https://github.com/Michandrz/joomla-saml/blob/devel/com_oneloginsaml/site/controller.php) which handles catching a lot of the requests(Joomla has a built in XML processor for the acs that is in it's own controller here: https://github.com/Michandrz/joomla-saml/blob/devel/com_oneloginsaml/site/controller.xml.php) it will then forward the user out to the IDP. the IDP then forwards the user back to the controller.php file (within the normal app loading) which calls on the process response in the library and then passes the library as a "credential" to the normal $app->login(): function. This is where the Authentication plugin gets a reference from the library. Then the Authentication library may update the user/create the user/etc before finishing the login.

The reason I kept a user plugin was to allow for other developers to extend the application more easily. When processing attributes, the Authentication plugin(via the user model) calls on any User Plugins with onUserUpdateInfo as a function to handle the IDP to SP attribute mappings/usages.

Also should mention for the admin it is actually simpler, because the entire system is installed via one package and managed though one admin side component.

Michandrz commented 3 years ago

@pitbulk I got admin side SSO working. That was painful. It seems Joomla really doesn't want you to extend backend login functions. Anyway my commits are here: https://github.com/Michandrz/joomla-saml/tree/devel

Let me know what you think......I know I want to pull some of the library loading from your code into it.