Creating a new authentication provider extension

[cheshirekow]

I've created a new authentication provider to enable authentication using a username passed through a reverse proxy configuration (my use case is nginx auth_proxy providing the username). I think it would be nice to turn it into an extension to share... and for my own sanity in keeping my forum up to date. I followed the official tutorial on creating an authentication provider, and I took a look at the extension skeleton. When I made my authentication provider, there were a couple of steps where I modified core files, leading me to the following questions:

• The tutorial was a little ambiguous when it came to editing auth.yml (which I couldn't find). In the end I added an entry to config/default/container/services_auth.yml. If I were to turn this into an extension, would this block of YAML just go in config/services.yml, as in the "acme" skeleton?
• I added my language text to phpBB/language/en/acp/board.php, because thats where the builtin authenticators put their language text. Looking at the skeleton it looks like I should just move these to language/en/common.php. I don't see any references to common.php anywhere else in the skeleton so is this file arbitrarily named? If i put my language text in language/en/foobarbaz.php will it get pulled in automatically?
• Do auth_provider_xxx.html templates get pulled in from extensions? The acme extension appears to have it's own code for the admin control panel, whereas I think I need my auth provider settings to show up within the auth selection dialog. Can I just save the file at adm/style/auth_provider_remote_user.html within my extension tree?

Here is the complete code of my auth provider and changes, for reference.

Code: Select all

diff --git a/phpBB/adm/style/auth_provider_remote_user.html b/phpBB/adm/style/auth_provider_remote_user.html
new file mode 100644
index 000000000..85535a2b1
--- /dev/null
+++ b/phpBB/adm/style/auth_provider_remote_user.html
@@ -0,0 +1,11 @@
+<fieldset id="auth_remote_user_settings">
+  <legend>{L_REMOTE_USER}</legend>
+  <dl>
+    <dt><label for="remote_user_varname">
+        {L_REMOTE_USER_VARNAME}{L_COLON}</label><br />
+      <span>{L_REMOTE_USER_VARNAME_EXPLAIN}</span></dt>
+    <dd><input type="text" id="remote_user_varname" size="40"
+        name="config[remote_user_varname]" value="{AUTH_REMOTE_USER_VARNAME}" />
+    </dd>
+  </dl>
+</fieldset>
diff --git a/phpBB/config/default/container/services_auth.yml b/phpBB/config/default/container/services_auth.yml
index ed8dc90a7..797c19cad 100644
--- a/phpBB/config/default/container/services_auth.yml
+++ b/phpBB/config/default/container/services_auth.yml
@@ -108,3 +108,18 @@ services:
             - '@request'
         tags:
             - { name: auth.provider.oauth.service }
+
+    # NOTE(josh): this is what is added
+    auth.provider.remote_user:
+        class: phpbb\auth\provider\remote_user
+        arguments:
+            - '@dbal.conn'
+            - '@config'
+            - '@passwords.manager'
+            - '@request'
+            - '@user'
+            - '@service_container'
+            - '%core.root_path%'
+            - '%core.php_ext%'
+        tags:
+            - { name: auth.provider }
diff --git a/phpBB/language/en/acp/board.php b/phpBB/language/en/acp/board.php
index 4887a73ca..20252db2d 100644
--- a/phpBB/language/en/acp/board.php
+++ b/phpBB/language/en/acp/board.php
@@ -458,7 +458,13 @@ $lang = array_merge($lang, array(
 	'LDAP_USER'						=> 'LDAP user <var>dn</var>',
 	'LDAP_USER_EXPLAIN'				=> 'Leave blank to use anonymous binding. If filled in phpBB uses the specified distinguished name on login attempts to find the correct user, e.g. <samp>uid=Username,ou=MyUnit,o=MyCompany,c=US</samp>. Required for Active Directory Servers.',
 	'LDAP_USER_FILTER'				=> 'LDAP user filter',
-	'LDAP_USER_FILTER_EXPLAIN'		=> 'Optionally you can further limit the searched objects with additional filters. For example <samp>objectClass=posixGroup</samp> would result in the use of <samp>(&amp;(uid=$username)(objectClass=posixGroup))</samp>',
+  'LDAP_USER_FILTER_EXPLAIN'		=> 'Optionally you can further limit the searched objects with additional filters. For example <samp>objectClass=posixGroup</samp> would result in the use of <samp>(&amp;(uid=$username)(objectClass=posixGroup))</samp>',
+
+  'REMOTE_USER' => 'Remote User',
+  'REMOTE_USER_VARNAME' => 'Remote User CGI-Var',
+  'REMOTE_USER_VARNAME_EXPLAIN' => 'CGI variable in which the server will place the username (if authenticated through Remote User Token)',
+  'REMOTE_USER_SETUP_BEFORE_USE' => 'You have to setup remote user authentication before you switch phpBB to this authentication method. Make sure your reverse proxy is passing in the right variable.',
+  'REMOTE_USER_INVALID_USERNAME' => 'You have to setup remote user authentication before you switch phpBB to this authentication method. The rerverse proxy is providing a username (%s) other than your current one (%s).',
 ));
 
 // Server Settings
diff --git a/phpBB/phpbb/auth/provider/remote_user.php b/phpBB/phpbb/auth/provider/remote_user.php
new file mode 100755
index 000000000..7aaf1d9b1
--- /dev/null
+++ b/phpBB/phpbb/auth/provider/remote_user.php
@@ -0,0 +1,431 @@
+<?php
+/**
+ *
+ * This file is part of the phpBB Forum Software package.
+ *
+ * @copyright (c) phpBB Limited <https://www.phpbb.com>
+ * @license GNU General Public License, version 2 (GPL-2.0)
+ *
+ * For full copyright and license information, please see
+ * the docs/CREDITS.txt file.
+ *
+ */
+
+namespace phpbb\auth\provider;
+
+/**
+ * Database authentication provider for phpBB3
+ * This is for authentication via the integrated user table
+ */
+class remote_user extends \phpbb\auth\provider\base {
+  /**
+   * phpBB passwords manager
+   *
+   * @var \phpbb\passwords\manager
+   */
+  protected $passwords_manager;
+
+  /**
+   * DI container
+   *
+   * @var \Symfony\Component\DependencyInjection\ContainerInterface
+   */
+  protected $phpbb_container;
+
+  /**
+   * Database Authentication Constructor
+   *
+   * @param    \phpbb\db\driver\driver_interface        $db
+   * @param    \phpbb\config\config         $config
+   * @param    \phpbb\passwords\manager    $passwords_manager
+   * @param    \phpbb\request\request        $request
+   * @param    \phpbb\user            $user
+   * @param    \Symfony\Component\DependencyInjection\ContainerInterface
+   *             $phpbb_container DI container
+   * @param    string                $phpbb_root_path
+   * @param    string                $php_ext
+   */
+  public function __construct(\phpbb\db\driver\driver_interface $db,
+    \phpbb\config\config $config,
+    \phpbb\passwords\manager $passwords_manager,
+    \phpbb\request\request $request,
+    \phpbb\user $user,
+    \Symfony\Component\DependencyInjection\ContainerInterface $phpbb_container,
+    $phpbb_root_path, $php_ext) {
+    $this->db = $db;
+    $this->config = $config;
+    $this->passwords_manager = $passwords_manager;
+    $this->request = $request;
+    $this->user = $user;
+    $this->phpbb_root_path = $phpbb_root_path;
+    $this->php_ext = $php_ext;
+    $this->phpbb_container = $phpbb_container;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function init() {
+    if (empty($config['remote_user_varname'])) {
+      $this->config['remote_user_varname'] = "REMOTE_USER";
+    }
+    $varname = $this->config['remote_user_varname'];
+
+    if (!$this->request->is_set($varname,
+      \phpbb\request\request_interface::SERVER)) {
+      return $this->user->lang['REMOTE_USER_SETUP_BEFORE_USE'];
+    }
+
+    $remoteuser = htmlspecialchars_decode(
+      $this->request->server($varname));
+    if ($this->user->data['username'] !== $remoteuser) {
+      return sprintf($this->user->lang['REMOTE_USER_INVALID_USERNAME'],
+        $remoteuser, $this->user->data['username']);
+    }
+    return false;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function login($username, $password) {
+    // Auth plugins get the password untrimmed.
+    // For compatibility we trim() here.
+    $password = trim($password);
+
+    if (!$username) {
+      return array(
+        'status' => LOGIN_ERROR_USERNAME,
+        'error_msg' => 'LOGIN_ERROR_USERNAME',
+        'user_row' => array('user_id' => ANONYMOUS),
+      );
+    }
+    $username_clean = utf8_clean_string($username);
+
+    // If password is empyt, check for RUT, otherwise disallow
+    $remoteauth_skip_pass = False;
+    if (!$password) {
+      $err = array(
+        'status' => LOGIN_ERROR_PASSWORD,
+        'error_msg' => 'NO_PASSWORD_SUPPLIED',
+        'user_row' => array('user_id' => ANONYMOUS),
+      );
+
+      // If no password was supplied but we have a remote user token
+      // supplied by the reverse proxy, then we approve the login. This allows
+      // remote user token to provide admin auth.
+      if (empty($config['remote_user_varname'])) {
+        $this->config['remote_user_varname'] = "REMOTE_USER";
+      }
+      $varname = $this->config['remote_user_varname'];
+
+      if (!$this->request->is_set($varname,
+        \phpbb\request\request_interface::SERVER)) {
+        return $err;
+      }
+
+      $remote_user = htmlspecialchars_decode($this->request->server($varname));
+      if ($remote_user != $username_clean) {
+        return $err;
+      }
+
+      $remoteauth_skip_pass = True;
+    }
+
+    $sql = 'SELECT *
+			FROM ' . USERS_TABLE . "
+			WHERE username_clean = '" . $this->db->sql_escape($username_clean) . "'";
+    $result = $this->db->sql_query($sql);
+    $row = $this->db->sql_fetchrow($result);
+    $this->db->sql_freeresult($result);
+
+    // Successful login... set user_login_attempts to zero...
+    if ($remoteauth_skip_pass) {
+      return array(
+        'status' => LOGIN_SUCCESS,
+        'error_msg' => false,
+        'user_row' => $row,
+      );
+    }
+
+    if (
+      ($this->user->ip && !$this->config['ip_login_limit_use_forwarded']) ||
+      ($this->user->forwarded_for &&
+        $this->config['ip_login_limit_use_forwarded'])) {
+      $sql = 'SELECT COUNT(*) AS attempts
+				FROM ' . LOGIN_ATTEMPT_TABLE . '
+        WHERE attempt_time > '
+        . (time() - (int) $this->config['ip_login_limit_time']);
+      if ($this->config['ip_login_limit_use_forwarded']) {
+        $sql .= " AND attempt_forwarded_for = '"
+        . $this->db->sql_escape($this->user->forwarded_for) . "'";
+      } else {
+        $sql .= " AND attempt_ip = '"
+        . $this->db->sql_escape($this->user->ip) . "' ";
+      }
+
+      $result = $this->db->sql_query($sql);
+      $attempts = (int) $this->db->sql_fetchfield('attempts');
+      $this->db->sql_freeresult($result);
+
+      $attempt_data = array(
+        'attempt_ip' => $this->user->ip,
+        'attempt_browser' => trim(substr($this->user->browser, 0, 149)),
+        'attempt_forwarded_for' => $this->user->forwarded_for,
+        'attempt_time' => time(),
+        'user_id' => ($row) ? (int) $row['user_id'] : 0,
+        'username' => $username,
+        'username_clean' => $username_clean,
+      );
+      $sql = 'INSERT INTO ' . LOGIN_ATTEMPT_TABLE
+      . $this->db->sql_build_array('INSERT', $attempt_data);
+      $this->db->sql_query($sql);
+    } else {
+      $attempts = 0;
+    }
+
+    if (!$row) {
+      if ($this->config['ip_login_limit_max'] &&
+        $attempts >= $this->config['ip_login_limit_max']) {
+        return array(
+          'status' => LOGIN_ERROR_ATTEMPTS,
+          'error_msg' => 'LOGIN_ERROR_ATTEMPTS',
+          'user_row' => array('user_id' => ANONYMOUS),
+        );
+      }
+
+      return array(
+        'status' => LOGIN_ERROR_USERNAME,
+        'error_msg' => 'LOGIN_ERROR_USERNAME',
+        'user_row' => array('user_id' => ANONYMOUS),
+      );
+    }
+
+    $show_captcha = ($this->config['max_login_attempts'] &&
+      $row['user_login_attempts'] >= $this->config['max_login_attempts']) ||
+      ($this->config['ip_login_limit_max'] &&
+      $attempts >= $this->config['ip_login_limit_max']);
+
+    // If there are too many login attempts, we need to check for a confirm
+    // image. Every auth module is able to define what to do by itself...
+    if ($show_captcha) {
+      /* @var $captcha_factory \phpbb\captcha\factory */
+      $captcha_factory = $this->phpbb_container->get('captcha.factory');
+      $captcha =
+      $captcha_factory->get_instance($this->config['captcha_plugin']);
+      $captcha->init(CONFIRM_LOGIN);
+      $vc_response = $captcha->validate($row);
+      if ($vc_response) {
+        return array(
+          'status' => LOGIN_ERROR_ATTEMPTS,
+          'error_msg' => 'LOGIN_ERROR_ATTEMPTS',
+          'user_row' => $row,
+        );
+      } else {
+        $captcha->reset();
+      }
+
+    }
+
+    // Check password ...
+    if ($this->passwords_manager->check(
+      $password, $row['user_password'], $row)) {
+      // Check for old password hash...
+      if ($this->passwords_manager->convert_flag ||
+        strlen($row['user_password']) == 32) {
+        $hash = $this->passwords_manager->hash($password);
+
+        // Update the password in the users table to the new format
+        $sql = 'UPDATE ' . USERS_TABLE . "
+					SET user_password = '" . $this->db->sql_escape($hash) . "'
+					WHERE user_id = {$row['user_id']}";
+        $this->db->sql_query($sql);
+
+        $row['user_password'] = $hash;
+      }
+
+      $sql = 'DELETE FROM ' . LOGIN_ATTEMPT_TABLE . '
+				WHERE user_id = ' . $row['user_id'];
+      $this->db->sql_query($sql);
+
+      if ($row['user_login_attempts'] != 0) {
+        // Successful, reset login attempts (the user passed all stages)
+        $sql = 'UPDATE ' . USERS_TABLE . '
+					SET user_login_attempts = 0
+					WHERE user_id = ' . $row['user_id'];
+        $this->db->sql_query($sql);
+      }
+
+      // User inactive...
+      if ($row['user_type'] == USER_INACTIVE ||
+        $row['user_type'] == USER_IGNORE) {
+        return array(
+          'status' => LOGIN_ERROR_ACTIVE,
+          'error_msg' => 'ACTIVE_ERROR',
+          'user_row' => $row,
+        );
+      }
+
+      // Successful login... set user_login_attempts to zero...
+      return array(
+        'status' => LOGIN_SUCCESS,
+        'error_msg' => false,
+        'user_row' => $row,
+      );
+    }
+
+    // Password incorrect - increase login attempts
+    $sql = 'UPDATE ' . USERS_TABLE . '
+			SET user_login_attempts = user_login_attempts + 1
+			WHERE user_id = ' . (int) $row['user_id'] . '
+				AND user_login_attempts < ' . LOGIN_ATTEMPTS_MAX;
+    $this->db->sql_query($sql);
+
+    // Give status about wrong password...
+    return array(
+      'status' => ($show_captcha) ? LOGIN_ERROR_ATTEMPTS : LOGIN_ERROR_PASSWORD,
+      'error_msg' => 'LOGIN_ERROR_PASSWORD',
+      'user_row' => $row,
+    );
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function acp() {
+    // These are fields required in the config table
+    return array(
+      'remote_user_varname',
+    );
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function get_acp_template($new_config) {
+    return array(
+      'TEMPLATE_FILE' => 'auth_provider_remote_user.html',
+      'TEMPLATE_VARS' => array(
+        'AUTH_REMOTE_USER_VARNAME' => $new_config['remote_user_varname'],
+      ),
+    );
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function autologin() {
+    if (empty($config['remote_user_varname'])) {
+      $this->config['remote_user_varname'] = "REMOTE_USER";
+    }
+    $varname = $this->config['remote_user_varname'];
+    if (!$this->request->is_set($varname,
+      \phpbb\request\request_interface::SERVER)) {
+      return array();
+    }
+
+    $remote_user = htmlspecialchars_decode($this->request->server($varname));
+
+    if (!empty($remote_user)) {
+      set_var($remote_user, $remote_user, 'string', true);
+
+      $sql = 'SELECT *
+				FROM ' . USERS_TABLE . "
+				WHERE username = '" . $this->db->sql_escape($remote_user) . "'";
+      $result = $this->db->sql_query($sql);
+      $row = $this->db->sql_fetchrow($result);
+      $this->db->sql_freeresult($result);
+
+      if ($row) {
+        if ($row['user_type'] == USER_INACTIVE ||
+          $row['user_type'] == USER_IGNORE) {
+          return array();
+        } else {
+          return $row;
+        }
+      }
+
+      if (!function_exists('user_add')) {
+        include $this->phpbb_root_path
+        . 'includes/functions_user.'
+        . $this->php_ext;
+      }
+
+      // create the user if she does not exist yet
+      user_add($this->user_row($remote_user));
+
+      $sql = 'SELECT *
+				FROM ' . USERS_TABLE . "
+        WHERE username_clean = '"
+      . $this->db->sql_escape(utf8_clean_string($remote_user)) . "'";
+      $result = $this->db->sql_query($sql);
+      $row = $this->db->sql_fetchrow($result);
+      $this->db->sql_freeresult($result);
+
+      if ($row) {
+        return $row;
+      }
+    }
+
+    return array();
+  }
+
+  /**
+   * This function generates an array which can be passed to the user_add
+   * function in order to create a user
+   *
+   * @param   string  $username   The username of the new user.
+   * @return   array  Contains data that can be passed directly to
+   *                  the user_add function.
+   */
+  private function user_row($username) {
+    // first retrieve default group id
+    $sql = 'SELECT group_id
+			FROM ' . GROUPS_TABLE . "
+			WHERE group_name = '" . $this->db->sql_escape('REGISTERED') . "'
+				AND group_type = " . GROUP_SPECIAL;
+    $result = $this->db->sql_query($sql);
+    $row = $this->db->sql_fetchrow($result);
+    $this->db->sql_freeresult($result);
+
+    if (!$row) {
+      trigger_error('NO_GROUP');
+    }
+
+    // generate user account data
+    return array(
+      'username' => $username,
+      'user_password' => substr(md5(rand()), 0, 7),
+      'user_email' => '',
+      'group_id' => (int) $row['group_id'],
+      'user_type' => USER_NORMAL,
+      'user_ip' => $this->user->ip,
+      'user_new' => ($this->config['new_member_post_limit']) ? 1 : 0,
+    );
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function validate_session($user) {
+    if (empty($config['remote_user_varname'])) {
+      $this->config['remote_user_varname'] = "REMOTE_USER";
+    }
+    $varname = $this->config['remote_user_varname'];
+
+    // Check if remote user token is set and handle this case
+    if ($this->request->is_set($varname,
+      \phpbb\request\request_interface::SERVER)) {
+      $php_auth_user = $this->request->server($varname);
+      return ($php_auth_user === $user['username']) ? true : false;
+    }
+
+    // remote user token is not set. A valid session is now determined by the
+    // user type (anonymous/bot or not)
+    if ($user['user_type'] == USER_IGNORE) {
+      return true;
+    }
+
+    return false;
+  }
+}

[mrgoldy]

The skeleton extension is a basic set up of an extension showing what is possible and what should go where, structure-wise.

- Your YML (service declarations) should indeed go into config/services.yml within your extension directory.
- For your language there are multiple options. You can add them to language/en/info_acp_SOME_NAME. All info_acp_* files from all extensions are automatically loaded for all the ACP pages. Alternatively you can hook into an event for that ACP page (https://github.com/phpbb/phpbb/blob/mas ... d.php#L478) and load your language file yourself:

Code: Select all

'core.acp_board_config_edit_add' => 'my_function'

public function my_function($event)
{
	if ($event['mode'] === 'auth')
	{
		$this->lang->add_lang('my_language', 'vendor/extension');
	}

And for "select" box that is displayed on the Authentication page in the ACP, the following method is used to build it: https://github.com/phpbb/phpbb/blob/mas ... d.php#L761
So your auth provider will have to be "tagged" (in yml with - tags { name: auth.provider }) to be included in the list. See here for some examples: https://github.com/phpbb/phpbb/blob/68c ... uth.yml#L7
Here is an example of a current auth provider: https://github.com/phpbb/phpbb/blob/68c ... der/db.php Others are in that directory aswell.

- And you'll have to use these functions to determine your admin template: https://github.com/phpbb/phpbb/blob/68c ... p#L61-L103

Hope this puts you in the right direction

[cheshirekow]

Awesome. I think that covers all of my questions. Thanks for all the pointers!

[cheshirekow]

Thanks again mrgoldy for your help. I think I managed to port this to a working extension. Next up is to try and get a working ci test, add some docs, and then publish.

[mrgoldy]

Happy to help, glad I could be of assistance.
Seems like you've made a nice extension, elegance in it's simplicity, well done!

A few minor things, and please note that these are just tips / possibilities:
- You can use twig in your admin template
- opening brackets { go on a new line.
- Use tabs (width of 4 spaces) instead of spaces.
- Empty line at the end of a file
- Didn't check but make sure to use the correct line endings. (phpBB's Coding Guidelines - not mandatory but prefered by phpBB)
- all $this->something fields should be declared before your constructor, like you are already doing with the passwords manager and the container.
- You are using the container to get the captcha factory, while I believe you can just inject this as a service itself. Not sure about this one though as I come across this method quite a few times in other places aswell. If injecting is possible, you don't even need the container. But perhaps something to look into.
- You can (and probably should) use $this->lang->lang('') instead of $this->user->lang['']. Then you also do not have to use sprintf().
- Perhaps it's possible to use the passwords manager to generate a password instead of this.
- You can use the phpbb's root path to get to the path for your template file. Or you can even use the extension manager to do that with $this->extension_manager->get_extension_path('cheshirekow/remoteuserauth', true) . 'adm/style/auth_provider_remoteuser.html';

Please do not take this is a review or something, just tips to help you move forward!

And I saw in your to-do list that you're working (and figuring out) the tests.
Not sure if you already found these, so just linking them to make sure you have them:
- Auth tests
- Functional auth tests

[cheshirekow]

Thanks for all the tips!

1. Re: style - Thanks for pointing these things out. My plan is to deal with that after things are working. Hopefully there's a recommended tool to just autoformat according to the style guide. I think I saw a note about a beautifier config file somewhere in the developer docs...
2. Thanks for the note on the container and captcha factory things. I think that if I just derive from the db provider that I can defer to the implementation of this stuff from that class. If not, I'll look into your note further.
4. Thanks for the tip on the password manager, I'll take a look to see if there's something already in there to (securely) generate a random password. What I currently have is probably not OK.
5. Thanks for the tip on the path resolution. ../../ felt pretty dirty to write.

And yes, I did find those existing auth tests and functional tests but thank you for pointing them out. One thing i'm not clear on is whether or not the functional tests are executed in CI as part of the existing test harness. I think there should be some extra Travis config to start a web server in order to run those, but I don't see that anywhere. If you happen to know about that and can point me in the right direction, I would appreciate it!

[cheshirekow]

oops. Nevermind. I found the webserver setup. In the script called setup-webserver.sh...

[cheshirekow]

A few more comments:

You can use twig in your admin template

Am I not? Isn't that what {L_REMOTE_USER} is in my template? A Twig
substitution?

You can (and probably should) use $this->lang->lang('')

I can't figure out what you meant by $this->lang->lang(''). I don't see a member `lang` in any of the interface, base class, or db
auth providers. Can you elaborate on what you meant by that?

Perhaps it's possible to use the passwords manager to generate a password

I don't see anything in the password manager that can generate a random password. Am I missing it or is there another place to look?

You can use the phpbb's root path to get to the path for your template file

Neither of these seemed to work:

• $tplpath = $this->phpbb_root_path . '/ext/cheshirekow/remoteuserauth' . '/adm/style/auth_provider_remoteuser.html';
• $tplpath = $this->extension_manager->get_extension_path('cheshirekow/remoteuserauth', true) . 'adm/style/auth_provider_remoteuser.hml';

I think the problem is it has to be a relative path from "adm/style"?

Please do not take this is a review or something, just tips to help you move forward!

Yeah thanks again! If you have any more to add feel free to let me know. I'm quite open to suggestions and criticisms.

Do you know if there's any kind of process for requesting reviews of new extensions? If not I can post a reply to my extension thread asking anyone to review, or I could start a new thread in this forum.

[mrgoldy]

{L_REMOTE_USER}
This is the phpBB syntax, where the L_ prefix determines that it is a language string, so it starts looking in the language files for REMOTE_USER. The TWIG Syntax for this would be: {{ lang('REMOTE_USER') }}.
Currently phpBB's core file still use that syntax, but will eventually also be converted to TWIG syntax. Or more likely when the new base style is introduced, Chameleon. That will be a while, but extensions can already start using it. It's slightly faster and easier on the eye.

lang
My bad on that one, I should be more explicit. lang is what is commonly used in extensions to refer to the language class.
So while the TWIG syntax (HTML) was {{ lang('REMOTE_USER') }}, PHP variant would be: $this->lang->lang('REMOTE_USER');.
The lang can also be language or anything you make it to be in your class.
The user->lang[''] is deprecated and should only be used when dealing with phpBB < 3.2 (eg. 3.1.x).

template file
Stack trace: step 1 - step 2 - step 3
Didn't properly look how that string was used. What you can do is "@vendor_extension/template_file.html", so in your case: "@cheshirekow_remoteuser/auth_provider_remoteuser.html".
Using the @ construct, tells the template that it is an extension file that is being loaded. So it takes the "vendor" and "extension" and automatically turns that in the correct path to the style.

password
I am unfamiliar with the password manager, just thought I would throw it out there as something to look into. I do not know if it provides a password generator of sorts.

---

You can't really get a "review" in this forum, here you can ask questions on stuff you're having trouble with and there are plenty of people that are willing to help. If you feel you've done enough testing and are done developing, you can choose to submit it to the Customisation Database, where your extension will be reviewed. It will be tested and the code will be checked by the Extension team member and the Jr. Validators (hey, thats me!).

Good luck!

[cheshirekow]

re: {L_REMOTE_USER}
Got it! Thanks for that. I've updated the code.

re: lang
Alright, I think I understand... but I'm inferring past some of my ignorance here. I think the "correct" thing to do is add - @language to services.yml in order to take advantage of dependency injection. Then I add \phpbb\language\language $language as a parameter to my auth provider c'tor. Then wherever I'm currently using $this->$user->lang replace that with $this->lang->lang('').

I gave this a try (diff here) and it seems to work. I've manually triggered the error cases and I do see the error messages that I expect. If you don't mind taking a quick look at that diff and letting me know if that matches what you were saying I would appreciate that.

re: password
Got it! I'll look around a bit and see if I can't find something already in phpbb.

re:

You can't really get a "review" in this forum...

Ok cool. I think I'll let it sit in the "Extensions in Progress" forum for a little while and see if anyone else uses it and exposes any issues. Than maybe I'll submit it to the customisation database.

[mrgoldy]

Yeah that's absolutely fine! The approach you're taking is fine. Just throwing it out there, so you know the possibilities:
I believe what you can also do is instead of re-creating the entire constructor, is declare the provider's base class as the parent in yml, and use a separate call function to inject the language class. Here's an example how it is done with notifictions, for the config class.
Service declaration and the called function
Not sure how this would affect your tests though!

Few last things, there should be no need to trim(), as this is already done by the request class itself.

[cheshirekow]

Ok great, thanks for taking a look. Thanks also for the additional pointers. I don't fully understand how that example works but once I grok that I'll think about whether or not I think it's worth it for my case. And thanks for the note on trim as well, I will remove that.

[mrgoldy]

Code: Select all

services:
    auth.provider.remoteuser:
        class: cheshirekow\remoteuserauth\provider
        parent: auth.provider.db
        shared: false
	calls:
            - [set_language, ['@language']]
        tags:
            - { name: auth.provider }

Then you can remove the entire __construct() and replace it with:

Code: Select all

	public function set_language(\phpbb\language\language $lang)
	{
		$this->lang = $lang;
	}

[cheshirekow]

Oooooh, now I see. Yeah that makes sense. Should be easy to update the test too. Thanks!

[3Di]

Not sure why the function random_str() is located outside of the class itself?
Btw, being called just one time in there could it be better to make it private or protected?