Extend auth plugin support: enable autoprovision and add authenticator page support #1333

V1pr · 2018-04-15T16:45:52Z

Added authenticator page support (changes the form destination in login_password_page to plugin page)
added autoprovisioning support for CredentialsPage
~~added automatic admin directory security with .htaccess if a specific file (install.done) exists. This is created upon a successful install~~ (removed/reverted)

https://www.mantisbt.org/bugs/view.php?id=24535

…all. Newly added .htaccess will handle the rest.

- If autoprovision is supported by the plugin, than redirection is allowed to credentials page (if set by plugin)

no-fast-forward

vboctor

Thanks @V1pr for your contribution.

I have added few comments in the code that would need addressing.
Please create issue(s) in the bug tracker and reference them from the pull request.
Would be good to explain in the PR / issues some of the flows that would be possible because of the changes in this PR. Screenshots would be a plus if you have them.
If you have a gist or similar with sample code, then reference them from PR. I noticed you had a PR for SampleAuth, but I didn't check it out yet.
Do we have the right hooks for an authenticator plugin to plugin the setup of the authenticator app?

.gitignore

core/authentication_api.php

vboctor · 2018-04-15T18:18:40Z

core/classes/AuthFlags.class.php

+	 * @see $logout_page
+	 * @var string|null
+	 */
+	private $authenticator_page = null;


Is there a reason why this shouldn't be transparent from core and implemented as part of $credentials_page and potentially other pages related to it as per plugin implementation? For example, you can imagine the credentials page asking for password, then redirecting to authenticator, then logging in the user.

This is separate by intention. In the updated readme of SampleAuth I described it in detail. Main reason is, that $credentials_page is mainly for SSO IMHO (with separate password request page), while $authenticator_page replaced the standard login.php for access validation - but uses the Mantis username/password aquisition pages.

core/classes/AuthFlags.class.php

vboctor · 2018-04-15T18:30:04Z

login_password_page.php

 $t_user_id = auth_get_user_id_from_login_name( $t_username );
-if( $t_user_id !== false && auth_credential_page( '', $t_user_id ) != AUTH_PAGE_CREDENTIAL ) {
+if( ( $t_user_id !== false || authenticator_autoprov_capable() ) && auth_credential_page( '', $t_user_id ) != AUTH_PAGE_CREDENTIAL ) {


Should we have a logic like the following before this block:

if( $t_user_id === false ) { $t_user_id = auth_provision_user( $p_username ); } // then use the existing logic

In this case, we always give plugins the change to provision the user, but they make decide that a username doesn't exist in the identity system and hence it shouldn't be provisioned or they don't support auto-provision. Hence, we replace authenticator_autoprov_capable() with auth_provision_user( $p_username ). The auth_provision_user() returns user id created or false.

I think this will not work with SSO systems. I mean, if you set $credentials_page to support SSO/external password aqusition page, than you'll not get redirected to the plugin page.

vboctor · 2018-04-15T18:38:02Z

login_password_page.php

+#
+# OR
+#
+# if the plugin provides autoprovisioning, than we can also redirect to the plugin page


Please explain the expected flow(s) when user is not found. Explain the case where use should be provisioned and the other where user wouldn't be provisioned because it doesn't exist in identity system.

In my opinion, user provisioning must always run afterwards authentication. If I use a plugin for auth and the user not yet exists in the system, than I want to do provisioning only, if the user authenticated himself correctly (that's why I set the flag for autoprov capable)

The flow:

if user found -> go to plugin page and to the auth

if user not found:

if plugin can do autoprov, than continue to auth

if pluging cannot do autoprov, than skip plugin auth and use standard flow

vboctor · 2018-04-15T18:44:00Z

core/user_api.php

-	user_ensure_email_unique( $p_email );
-	email_ensure_valid( $p_email );
-	email_ensure_not_disposable( $p_email );
+	if ( ! is_blank($p_email) ) {


Proper formatting is if( !is_blank( $p_email ) ) {. But why do we need this check? The existing methods should apply the right validation based on configuration like whether or not to allow empty email addresses via $g_allow_blank_email.

I've added this to make the autoprov work. I've failed to notice the $g_allow_blank_email in the code, but there can be scenarios, when it's not possible to turn it on (=allowing blank e-mails); like if signup is enabled, but I want to use external auth besides it with autoprov.

My initial problem was, than if I supply an e-mail to user_create, than a signup e-mail will be sent to the user, which I want to avoid. I would also be possible to add a new parameter to the function, like 'p_dont_send_signup_email', but I did not want to do that not knowing your future ideas with user_create.

vboctor · 2018-04-15T18:47:29Z

core/authentication_api.php

+ * @param string $p_username The username
+ * @return string The authenticator page with query string.
+ */
+function auth_authenticator_page( $p_user_id = null, $p_username = '' ) {


The term authenticator reflects a specific implementation. We should probably consider extended_auth. It can work with all kinds of 2FA or any other approach (e.g. security questions), etc.

I would expect such page to be visible only after user enters their correct credentials via the standard password page. If user is using a custom credentials page, then this setting wouldn't be applicable.

The user is either using credentials_page OR authenticator_page. The former request the password from the user and authenticates it (or does SSO), while the later replaces the standard login.php and handles the authentication (redirects to login.php if unsuccessful).

Reasoning: I wanted to use the standard mantis flow (username and password aquisition), but was not able to change login.php to support the plugin as a second auth mech, since the authflags got resetted to default before the redirect and changing this seemed to be a much deeper change. I named is authenticator, because the page actually does the authentication with the username/password POST-ed to it (see SampleAuth readme in PR)

added phpdoc to auth_generate_random_password()

…ful install. Newly added .htaccess will handle the rest." This reverts commit 1c6cfc6. install.done is not the best idea, since not everyone uses Apache

This reverts commit 7faf9bb. Using install.done is not the best idea

V1pr · 2018-04-17T17:37:11Z

I can't seem to find the place, where I can create an issue#/feture req.#. What am I missing?

About your last question: this setup works with my extended version on SampleAuth plugin (also added a lot of comments to it, with a more detailed readme). However, I found that the auth plugin concept might needs some work. I would change it to a "stack like" method. A plugin registers itself to the core with a set of flags (including priority of the mech). Upon login the core checks the corresponding plugins in priority order taking the flag into consideration when checking.

V1pr · 2018-05-30T07:44:10Z

Ping?

aborderon · 2018-06-05T09:23:36Z

@vboctor Do you have any elements to communicate to advance this amendment? Because it interests us.
Thank you

aborderon · 2018-06-07T12:12:37Z

@V1pr about creating problems in the bug tracker and referencing them from the pull request. You have to go here : https://www.mantisbt.org/bugs/view_all_bug_page.php

V1pr · 2018-06-11T17:41:51Z

@aborderon thanks, I've opened a ticket.
@vboctor @dregad is there anything else I need to do speed the merge up a bit?

dregad · 2018-06-12T08:17:50Z

With the revert of the .gitignore change, I don't have any issues with your PR. I let you follow up with vboctor.

atrol · 2018-06-12T18:25:46Z

core/authentication_api.php

+/**
+ * Gets the page that validates the user credentials based on the standard flow.
+ *
+ * @param string $p_query_string The query string, can be empty.


parameter $p_query_string does not exist

sorry, I don't get this one. $p_query_string exists as an input parameter for auth_credential_page

Isn't this the PHPdoc for auth_authenticator_page ?

core/authentication_api.php

core/classes/AuthFlags.class.php

atrol · 2018-06-12T18:32:11Z

core/classes/AuthFlags.class.php

+	/**
+	 * Sets if the auth pugin is capable of provision users
+	 *
+	 * @param bool capability


$p_autoprov

core/classes/AuthFlags.class.php

atrol · 2018-06-12T18:34:56Z

login_password_page.php

@@ -201,7 +206,7 @@
 					<?php echo $t_form_title ?>
 				</h4>
 				<div class="space-10"></div>
-	<form id="login-form" method="post" action="login.php">
+	<form id="login-form" method="post" action="<?php echo auth_authenticator_page($t_user_id, $t_username); ?>">


spaces after/before parentheses

atrol · 2018-06-12T18:52:56Z

core/classes/AuthFlags.class.php

+	 * @see setAutoprovisionCapability()
+	 */
+	function getAutoprovisionCapability() {
+		if( is_null( $this->autoprovision_capability ) ) {


s/autoprovision_capability/autoprovision_capable

atrol · 2018-06-12T18:53:07Z

core/classes/AuthFlags.class.php

+			return false;
+		}
+
+		return $this->autoprovision_capability;


s/autoprovision_capability/autoprovision_capable

atrol · 2018-06-12T19:08:22Z

core/user_api.php

-	user_ensure_email_unique( $p_email );
-	email_ensure_valid( $p_email );
-	email_ensure_not_disposable( $p_email );
+	if( !is_blank( $p_email ) ) {


Why did you introduce this check?

After your change, blank email addresses can be set even if $g_allow_blank_email = OFF; (our default setting)

If I remember correctly, I ran into issues upon automatic user creation, but I must have overlooked something. I'll revert this one and add a comment to SampleAuth,

I've found the answer in SampleAuth: the reason was behind this, that Mantis will send a SignUP e-mail, if e-mail is provided (which I did not want), so I create the use with empty e-mail and assign the email afterwards. See SampleAuth plugin. Anyway, I'll change the logic/doc in SampleAuth.

This reverts commit fea867a. Conflicts: core/user_api.php

core/user_api.php

aborderon · 2018-07-03T06:31:26Z

@V1pr news ?

V1pr · 2018-08-31T09:42:08Z

@aborderon none, that I know of :( I did what I can :(

@dregad @vboctor @atrol is there anything I can do, to speed the things up?

V1pr · 2018-10-01T11:26:10Z

@dregad @vboctor @atrol ping?

atrol · 2018-10-01T13:13:05Z

I am a bit concerned, as this is security related authentication code and it seems that this code has not been tested that much.
e.g. getAutoprovisionCapability did not work due to access to non-existing autoprovision_capability before V1pr addressed my comments in b036694

When I reviewed in June, I didn't test anything, just did some static code review.
I don't use auth plugin functionality myself and I am not able to invest time in having a deeper look and tests.

Sorry @V1pr, I will not be the one who approves the PR.

dregad

As mentioned in #1333 (comment), I'm fine with the PR as it stands. But since @vboctor had some remarks initially (which you seem to have addressed), I'd like to have his feedback before merging it.

V1pr · 2018-10-01T14:46:08Z

Thank you Gents! I'll wait for @vboctor 's approval/comments.

vboctor

I have added few comments based on current approach.

I think with the possible ways to override the auth flow it is getting a bit too complicated. I wonder if we can simplify by having the standard flow inject the following events:

EVENT_AUTH_VERIFY_PASSWORD (taking username / password) - this is used to validate user password instead of the MantisBT database.
EVENT_AUTH_PROVISION_USER (taking username - plugin can implement by fetching user info and creating a MantisBT user or just do nothing)

In this case case, there would be two approaches for auth plugins:

Simple - Use standard flow but inject external password validation and possibly auto-provisioning.
Advanced - Take over credential page and implement a completely custom credential - e.g. password + 2FA.

This would remove the use of authenticator page, the auto-provision auth flag, etc.

core/authentication_api.php

core/classes/AuthFlags.class.php

login_password_page.php

core/classes/AuthFlags.class.php

vboctor · 2018-10-03T15:38:27Z

We can even have a single plugin event to verify password and provision if necessary. The event can take the username and password and return false in case of failure or user id (matched or provisioned).

EVENT_AUTH_VERIFY_PASSWORD_AND_PROVISION

You can imagine multiple auth plugins subscribed to this event and the first one that succeeds would win.

Thanks @V1pr

V1pr · 2020-05-25T15:04:28Z

We can even have a single plugin event to verify password and provision if necessary. The event can take the username and password and return false in case of failure or user id (matched or provisioned).

EVENT_AUTH_VERIFY_PASSWORD_AND_PROVISION

You can imagine multiple auth plugins subscribed to this event and the first one that succeeds would win.

Thanks @V1pr

In this case, you'll likely have an userid collision soon. Using multiple auth backends in a frontend application is imho never a good idea. Authenticator backends should be handled by the authenticator app (SSO or something like that), not an application.

root and others added 11 commits April 14, 2018 23:12

Modified install.php to secure the admin folder after successful inst…

1c6cfc6

…all. Newly added .htaccess will handle the rest.

Add Authenticator page url and autoprovision_capavility flag support

a389df2

Add Authenticator page url and autoprovision_capavility flag support

aaac3d7

- Added authenticator page support for login_user_password.php

f5f97d4

- If autoprovision is supported by the plugin, than redirection is allowed to credentials page (if set by plugin)

Merge branch 'master' of https://github.com/V1pr/mantisbt

a63d095

no-fast-forward

typo fix in authenticator_autoprov_capable

6fb96ae

finetuning the Authenticator concept

46636b8

added generate_random_password length support default length of 16

01b38c1

allow user creation without e-mail

fea867a

added install.done support

7faf9bb

fixed auth_authenticator_page params

3a9f058

vboctor requested changes Apr 15, 2018

View reviewed changes

vboctor self-assigned this Apr 15, 2018

V1pr added 5 commits April 16, 2018 13:40

renamed authenticator_autoprov_capable to auth_can_auto_provision()

282c0f2

added phpdoc to auth_generate_random_password()

formatting fix

60ee26e

format change

ea5e4fb

Revert "Modified install.php to secure the admin folder after success…

4cd9025

…ful install. Newly added .htaccess will handle the rest." This reverts commit 1c6cfc6. install.done is not the best idea, since not everyone uses Apache

Revert "added install.done support"

71aba7a

This reverts commit 7faf9bb. Using install.done is not the best idea

V1pr changed the title ~~Authenticator support with autoprovision; admin dir security~~ Authenticator support with autoprovision Apr 17, 2018

V1pr changed the title ~~Authenticator support with autoprovision~~ Extend auth plugin support: enable autoprovision and add authenticator page support Apr 17, 2018

atrol reviewed Jun 12, 2018

View reviewed changes

core/authentication_api.php Outdated Show resolved Hide resolved

atrol reviewed Jun 12, 2018

View reviewed changes

core/authentication_api.php Outdated Show resolved Hide resolved

atrol reviewed Jun 12, 2018

View reviewed changes

core/classes/AuthFlags.class.php Outdated Show resolved Hide resolved

atrol reviewed Jun 12, 2018

View reviewed changes

core/classes/AuthFlags.class.php Outdated Show resolved Hide resolved

atrol reviewed Jun 12, 2018

View reviewed changes

V1pr added 4 commits June 12, 2018 22:30

typo fixes requested by atrol

b036694

Revert "allow user creation without e-mail"

407a940

This reverts commit fea867a. Conflicts: core/user_api.php

fixed PHPDoc for auth_authenticator_page

df3a074

PHPDoc fix

845c2ec

atrol reviewed Jun 13, 2018

View reviewed changes

core/user_api.php Outdated Show resolved Hide resolved

Resolved conflict in user_api

ec447dc

dregad approved these changes Oct 1, 2018

View reviewed changes

vboctor requested changes Oct 3, 2018

View reviewed changes

core/authentication_api.php Show resolved Hide resolved

core/classes/AuthFlags.class.php Show resolved Hide resolved

login_password_page.php Show resolved Hide resolved

core/classes/AuthFlags.class.php Show resolved Hide resolved

vboctor added the plugins Pull requests related to plugins and events label Apr 9, 2023

Extend auth plugin support: enable autoprovision and add authenticator page support #1333

Are you sure you want to change the base?

Extend auth plugin support: enable autoprovision and add authenticator page support #1333

Conversation

V1pr commented Apr 15, 2018 • edited

vboctor left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

V1pr commented Apr 17, 2018

V1pr commented May 30, 2018

aborderon commented Jun 5, 2018

aborderon commented Jun 7, 2018

V1pr commented Jun 11, 2018

dregad commented Jun 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aborderon commented Jul 3, 2018

V1pr commented Aug 31, 2018

V1pr commented Oct 1, 2018

atrol commented Oct 1, 2018

dregad left a comment

Choose a reason for hiding this comment

V1pr commented Oct 1, 2018

vboctor left a comment

Choose a reason for hiding this comment

vboctor commented Oct 3, 2018

V1pr commented May 25, 2020

V1pr commented Apr 15, 2018 •

edited

vboctor left a comment •

edited