Initial upload #2
Conversation
|
See my notes in the other PR #1 (review) |
Updates following feedback from @dregad
There was a problem hiding this comment.
I have added some minor comments inline. Here are the high level ones:
- I'm surprised there are no configs for the IDP itself. Like IDP url, login url, logout url, and cert. So I'm unsure how you redirect to IDP and how you validate that tokens are signed by IDP.
- It would be good to have a config option for
users_samlthat can be set to comma separated usernames during config testing. - Maybe have a
users_no_samlfor comma separate array of usernames to not use SAML auth for. This provides the backdoor for administration if SAML is not working for some reason for the admins. We can limit this to admins. - I would bundle the SimpleSAMLAuth class rather than having a config option to point to it elsewhere.
- It would be useful to add instructions to the readme about how to test for developers to be able to verify.
|
|
||
| # Passwords managed externally for all users | ||
| $t_flags->setCanUseStandardLogin( false ); | ||
| $t_flags->setPasswordManagedExternallyMessage( 'Passwords are no more, you cannot change them!' ); |
There was a problem hiding this comment.
This message should be localized via a language file.
| # No one can use standard auth mechanism | ||
|
|
||
| # Override Login page and Logout Redirect | ||
| $t_flags->setCredentialsPage( helper_url_combine( plugin_page( 'login', /* redirect */ true ), 'username=' . $t_username ) ); |
There was a problem hiding this comment.
You should url encode the username.
|
|
||
| # Enable re-authentication and use more aggressive timeout. | ||
| $t_flags->setReauthenticationEnabled( true ); | ||
| $t_flags->setReauthenticationLifetime( 10 ); |
There was a problem hiding this comment.
The default 5 minutes seems reasonable. 10 seconds seems to be an overkill even if the overhead is just a redirect to an IDP and back. I suggest leaving this to default in MantisBT or having it as a config for the plugin.
|
|
||
| function config() { | ||
| return array( | ||
| 'autoloader_path' => '', |
There was a problem hiding this comment.
Why do we need a file path in the configs? Why can't this path be known by the plugin assuming the library is bundled with the plugin? Even if for some reason we think this is needed, I wouldn't surface it in the UI and keep it as an internal config option that can be overridden in config/config_inc.php. But ideally, I would try to get rid of this config.
There was a problem hiding this comment.
This is linked to the workings of SimpleSAMLphp.
It is a standalone installation, separate from the core Mantis installation.
| function config() { | ||
| return array( | ||
| 'autoloader_path' => '', | ||
| 'SP_name' => '', |
There was a problem hiding this comment.
How about service_provider as the config option name?
|
|
||
| $s_plugin_SimpleSAMLphpAuth_configuration = 'Configuration'; | ||
| $s_plugin_SimpleSAMLphpAuth_config_autoloader_path = 'SimpleSAMLphp autoloader path'; | ||
| $s_plugin_SimpleSAMLphpAuth_config_SP_name = 'SimpleSAMLphp SP name'; |
There was a problem hiding this comment.
"config_" isn't really necessary. No need for "SimpleSAMLphp" in the UI string. Also I would use "Service Provider Name" instead of "SP name".
| } | ||
|
|
||
| function config() { | ||
| return array( |
There was a problem hiding this comment.
How about adding a test_users that can be set to a comma separated usernames to only enable the plugin for such users for testing. If not set/empty, then plugin applies to all. That enables the admin to test on a specific accounts before playing around with configs which may lock everybody out.
| $t_simplesamlphp_instance = new SimpleSAML_Auth_Simple( plugin_config_get( 'SP_name' ) ); | ||
| $t_simplesamlphp_instance->requireAuth(); | ||
| if( $t_simplesamlphp_instance->isAuthenticated() ) { | ||
| $t_simplesamlphp_attributes = $t_simplesamlphp_instance->getAttributes(); |
There was a problem hiding this comment.
indentation seems to be inconsistent.
| $t_simplesamlphp_instance->requireAuth(); | ||
| if( $t_simplesamlphp_instance->isAuthenticated() ) { | ||
| $t_simplesamlphp_attributes = $t_simplesamlphp_instance->getAttributes(); | ||
| $f_username = $t_simplesamlphp_attributes[ plugin_config_get( 'auth_attributes_username' ) ][0]; |
There was a problem hiding this comment.
Check that the attribute is set before retrieving it to avoid php errors in case plugin is misconfigured during testing.
readme.md
Outdated
| This is an authentication plugin for SimpleSAMLphp. | ||
|
|
||
| The authentication mechanism implemented by this plugin works as follows: | ||
| - If user ID is 1 (Administrator), use standard authentication. |
Updates following feedback from @dregad, @vboctor and @libregeek
|
Hm, why were the latest changes not reviewed? |
No description provided.