r69156 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r69155‎ | r69156 | r69157 >
Date:21:31, 7 July 2010
Author:jeroendedauw
Status:deferred (Comments)
Tags:
Comment:
Work on porting WP filesystem abstraction classes
Modified paths:
  • /trunk/extensions/Deployment/includes/filesystems/Ssh2Filesystem.php (modified) (history)

Diff [purge]

Index: trunk/extensions/Deployment/includes/filesystems/Ssh2Filesystem.php
@@ -27,11 +27,18 @@
2828 /**
2929 * The FTP connection link.
3030 *
31 - * @var resource
 31+ * @var FTP resource
3232 */
3333 protected $connection;
3434
3535 /**
 36+ * The SFTP connection link.
 37+ *
 38+ * @var SSH2 SFTP resource
 39+ */
 40+ protected $sftpConnection;
 41+
 42+ /**
3643 * Indicates if public key authentication is used instead of a regular password.
3744 *
3845 * @var boolean
@@ -68,12 +75,17 @@
6976 // TODO: validate that both keys are set (error if only one)
7077 $this->publicKeyAuthentication = array_key_exists( 'public_key', $options ) && array_key_exists( 'private_key', $options );
7178
 79+ if ( $this->publicKeyAuthentication ) {
 80+ $options['hostkey'] = array( 'hostkey' => 'ssh-rsa' );
 81+ }
 82+
7283 // Regular authentication needs a username.
7384 if ( !$this->publicKeyAuthentication && !array_key_exists( 'username', $options ) ) {
7485 $this->addError( 'deploy-ssh2-username-required' );
7586 }
7687
7788 // Regular authentication needs a password.
 89+ // TODO: if publick key: make sure the key is not empty
7890 if ( !$this->publicKeyAuthentication && !array_key_exists( 'password', $options ) ) {
7991 $this->addError( 'deploy-ssh2-password-required' );
8092 }
@@ -91,7 +103,41 @@
92104 * @see Filesystem::connect
93105 */
94106 public function connect() {
95 -
 107+ if ( $this->publicKeyAuthentication ) {
 108+ wfSuppressWarnings();
 109+ $this->connection = ssh2_connect( $this->options['hostname'], $this->options['port'], $this->options['hostkey'] );
 110+ wfRestoreWarnings();
 111+ } else {
 112+ wfSuppressWarnings();
 113+ $this->connection = ssh2_connect( $this->options['hostname'], $this->options['port'] );
 114+ wfRestoreWarnings();
 115+ }
 116+
 117+ if ( !$this->connection ) {
 118+ $this->addErrorMessage( wfMsgExt( 'deploy-ssh2-connect-failed', $this->options['hostname'], $this->options['port'] ) );
 119+ return false;
 120+ }
 121+
 122+ if ( $this->publicKeyAuthentication ) {
 123+ $ssh2_auth_pubkey_file = ssh2_auth_pubkey_file($this->link, $this->options['username'], $this->options['public_key'], $this->options['private_key'], $this->options['password'] );
 124+
 125+ if ( !$ssh2_auth_pubkey_file ) {
 126+ $this->addErrorMessage( wfMsgExt( 'deploy-ssh2-key-authentication-failed', $this->options['username'] ) );
 127+ return false;
 128+ }
 129+
 130+ } else {
 131+ $ssh2_auth_password = ssh2_auth_password( $this->connection, $this->options['username'], $this->options['password'] );
 132+
 133+ if ( !$ssh2_auth_password ) {
 134+ $this->addErrorMessage( wfMsgExt( 'deploy-ssh2-password-authentication-failed', $this->options['username'] ) );
 135+ return false;
 136+ }
 137+ }
 138+
 139+ $this->sftpConnection = ssh2_sftp( $this->connection );
 140+
 141+ return true;
96142 }
97143
98144 /**

Follow-up revisions

RevisionCommit summaryAuthorDate
r69184Follow up to r69156jeroendedauw12:57, 8 July 2010

Comments

#Comment by Nikerabbit (talk | contribs)   05:20, 8 July 2010
+$this->addErrorMessage( wfMsgExt( 'deploy-ssh2-key-authentication-failed', $this->options['username'] ) );

You are missing the second parameter to wfMsgExt in many places. Also, I don't think the message passing interface is optimal right now.

#Comment by Jeroen De Dauw (talk | contribs)   10:13, 8 July 2010

> You are missing the second parameter to wfMsgExt in many places. What should I do here then? I have to admit I find the messaging methods rather confusing - I just want to get a message with some vars in it, why do I need other parameters?

> Also, I don't think the message passing interface is optimal right now. Feel free to suggest a better approach or implement it yourself. In any case, I've set this up so it can easily be changed with minimal work.

#Comment by Nikerabbit (talk | contribs)   10:16, 8 July 2010

Maybe wfMsg or wfMsgExt with 'parsemag' as the second parameter? I'm currently pushing the new message class on Wikitech (again), so I don't know yet what to suggest :)

#Comment by Jeroen De Dauw (talk | contribs)   10:21, 8 July 2010

I guess using such a new class would be acceptable for this project, as it'll require the latest code anyway, so I hope it gets accepted soon! :)

So I should do this right? wfMsgExt( 'deploy-ssh2-key-authentication-failed', 'parsemag', $this->options['username'] )

Any docs on that parameter?

#Comment by Nikerabbit (talk | contribs)   10:23, 8 July 2010

Yes your syntax is correct. See documentation for wfMsgExt in GlobalFunctions.php. You can use plain string instead of array when using only one option. parsemag replaces {{..}}-stuff (so magic words like plural) but does not parse any other wiki markup.

#Comment by Jeroen De Dauw (talk | contribs)   10:29, 8 July 2010

Right. Will fix. Have to agree with MaxSem about those functions though :p

#Comment by Jeroen De Dauw (talk | contribs)   12:58, 8 July 2010

Made follow up commit :)

Status & tagging log