JPERF-729: Create a way to configure Jira admin user password during database setup #107
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,3 @@ | ||
| .gradle | ||
| build | ||
| .idea | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| package com.atlassian.performance.tools.infrastructure.api.database.passwordoverride | ||
|
|
||
| import com.atlassian.performance.tools.infrastructure.database.SshSqlClient | ||
| import com.atlassian.performance.tools.ssh.api.SshConnection | ||
|
|
||
| interface JiraUserEncryptedPasswordProvider { | ||
| fun getEncryptedPassword(ssh: SshConnection): String | ||
| } | ||
|
|
||
| class CrowdEncryptedPasswordProvider( | ||
|
mgrzaslewicz marked this conversation as resolved.
Outdated
Contributor
Author
There was a problem hiding this comment. I believe Also in the idea of SOLID I'd personally split this class into smaller modules:
Glueing this together can be done in different ways. The idea is to represent every behavior as class for better modularity. Ideally we could allow every module to be overridable, however the example below doesn't do that. Decide yourself about the details and if such change could be better or not.
Contributor
There was a problem hiding this comment. We can generate infinite abstractions. The question is: "which ones are useful?". Each abstraction should have a practical (not purely hypothetical) purpose.
Contributor
There was a problem hiding this comment. IMHO this is overcomplicating simple job and premature optimization. The example provided is hard to understand, at least for me. String result from db contains also some additional lines including header. You can prepare here super precise implementation and do not use String.contains. But what would be the benefit?
Contributor
Author
There was a problem hiding this comment.
I'm not pointing abstractions here. There is limited number of responsibilities and this class has more than one. At least query execution and excetional result condition handling IMHO should be split. If we are assuming a contract that is not given to us explicitly I'd prefer to have this contract isolated as much as possible. In our case we are assuming that:
Ability to split it to many responsibilities proves that this is not so simple and fact that I'm able to use this way to spot some implicit contracts that the code starts to rely on makes it not premature optimization, but rather quality assurance. Still I'm not insisting in chaging it if both of you don't see value in it. So far my understanding of SRP was that we should split as much as possible and that SRP is something that we are following. Possibly one of those is incorrect. |
||
| private val jiraDatabaseSchemaName: String, | ||
| private val passwordPlainText: String, | ||
| private val passwordEncryptedWithAtlassianSecurityPasswordEncoder: String, | ||
| private val sqlClient: SshSqlClient | ||
| ) : JiraUserEncryptedPasswordProvider { | ||
|
|
||
| override fun getEncryptedPassword(ssh: SshConnection): String { | ||
| val sqlResult = | ||
| sqlClient.runSql(ssh, "select attribute_value from ${jiraDatabaseSchemaName}.cwd_directory_attribute where attribute_name = 'user_encryption_method';").output | ||
| return when { | ||
| sqlResult.contains("plaintext") -> passwordPlainText | ||
| sqlResult.contains("atlassian-security") -> passwordEncryptedWithAtlassianSecurityPasswordEncoder | ||
| else -> throw RuntimeException("Unknown jira user password encryption type") | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| package com.atlassian.performance.tools.infrastructure.api.database.passwordoverride | ||
|
|
||
| import com.atlassian.performance.tools.infrastructure.api.database.Database | ||
| import com.atlassian.performance.tools.infrastructure.database.SshMysqlClient | ||
| import com.atlassian.performance.tools.infrastructure.database.SshSqlClient | ||
| import com.atlassian.performance.tools.ssh.api.SshConnection | ||
| import org.apache.logging.log4j.LogManager | ||
| import org.apache.logging.log4j.Logger | ||
| import java.net.URI | ||
|
|
||
| class JiraUserPasswordOverridingDatabase private constructor( | ||
| private val databaseDelegate: Database, | ||
| private val sqlClient: SshSqlClient, | ||
| private val username: String, | ||
| private val schema: String, | ||
| private val userPasswordPlainText: String, | ||
| private val jiraUserEncryptedPasswordProvider: JiraUserEncryptedPasswordProvider | ||
| ) : Database { | ||
| private val logger: Logger = LogManager.getLogger(this::class.java) | ||
|
Contributor
Author
There was a problem hiding this comment. Nit: Logging responsibility could be split to a decorator on this class.
Contributor
There was a problem hiding this comment. What would be the benefit?
Contributor
Author
There was a problem hiding this comment. There are multiple benefits:
Again I'm not insisting, especially with this one, because our practice was so far to not decouple logger from other business logic. Still I wanted to point it out to see your thoughts on it. I personally would prefer us to follow the decoupling practices, because I see how not following it damages the flexibility of our implementations. I don't believe in popular thinking that everything can be rewritten to match the new requirements - from my experience it never happens. However we can write code that is modular and later one be able to initialise it differently to achieve new requirements.
Contributor
There was a problem hiding this comment. I understand all the benefits, but do we really need them?
Contributor
There was a problem hiding this comment. @pczuj I think these are interesting dev experiments, they deserve to be explored. However, we shouldn't mix them inside a big and overdue feature PR. |
||
|
|
||
| override fun setup(ssh: SshConnection): String = databaseDelegate.setup(ssh) | ||
|
|
||
| override fun start( | ||
| jira: URI, | ||
| ssh: SshConnection | ||
| ) { | ||
| databaseDelegate.start(jira, ssh) | ||
| val password = jiraUserEncryptedPasswordProvider.getEncryptedPassword(ssh) | ||
| sqlClient.runSql(ssh, "UPDATE ${schema}.cwd_user SET credential='$password' WHERE user_name='$username';") | ||
| logger.debug("Password for user '$username' updated to '${userPasswordPlainText}'") | ||
| } | ||
|
|
||
|
|
||
| class Builder( | ||
| private var databaseDelegate: Database, | ||
| private var plainTextPassword: String, | ||
| private var passwordEncrypted: String | ||
| ) { | ||
| private var sqlClient: SshSqlClient = SshMysqlClient() | ||
| private var schema: String = "jiradb" | ||
| private var username: String = "admin" | ||
| private var jiraUserEncryptedPasswordProvider: JiraUserEncryptedPasswordProvider? = null | ||
|
|
||
| fun databaseDelegate(databaseDelegate: Database) = apply { this.databaseDelegate = databaseDelegate } | ||
| fun username(username: String) = apply { this.username = username } | ||
| fun plainTextPassword(userPasswordPlainText: String) = apply { this.plainTextPassword = userPasswordPlainText } | ||
| fun passwordEncrypted(userPasswordEncrypted: String) = apply { this.passwordEncrypted = userPasswordEncrypted } | ||
| fun sqlClient(sqlClient: SshSqlClient) = apply { this.sqlClient = sqlClient } | ||
| fun schema(jiraDatabaseSchemaName: String) = apply { this.schema = jiraDatabaseSchemaName } | ||
| fun jiraUserEncryptedPasswordProvider(jiraUserEncryptedPasswordProvider: JiraUserEncryptedPasswordProvider) = | ||
| apply { this.jiraUserEncryptedPasswordProvider = jiraUserEncryptedPasswordProvider } | ||
|
|
||
| fun build() = JiraUserPasswordOverridingDatabase( | ||
| databaseDelegate = databaseDelegate, | ||
| sqlClient = sqlClient, | ||
| username = username, | ||
| userPasswordPlainText = plainTextPassword, | ||
| schema = schema, | ||
| jiraUserEncryptedPasswordProvider = jiraUserEncryptedPasswordProvider ?: CrowdEncryptedPasswordProvider( | ||
| jiraDatabaseSchemaName = schema, | ||
| passwordPlainText = plainTextPassword, | ||
| passwordEncryptedWithAtlassianSecurityPasswordEncoder = passwordEncrypted, | ||
| sqlClient = sqlClient | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| } | ||
|
|
||
| /** | ||
| * @param adminPasswordEncrypted Based on [retrieving-the-jira-administrator](https://confluence.atlassian.com/jira/retrieving-the-jira-administrator-192836.html) | ||
| * to encode the password in Jira format use [com.atlassian.crowd.password.encoder.AtlassianSecurityPasswordEncoder](https://docs.atlassian.com/atlassian-crowd/4.2.2/com/atlassian/crowd/password/encoder/AtlassianSecurityPasswordEncoder.html) | ||
| * from the [com.atlassian.crowd.crowd-password-encoders](https://mvnrepository.com/artifact/com.atlassian.crowd/crowd-password-encoders/4.2.2). | ||
| * | ||
| */ | ||
| fun Database.overrideAdminPassword(adminPasswordPlainText: String, adminPasswordEncrypted: String): JiraUserPasswordOverridingDatabase.Builder { | ||
|
Contributor
There was a problem hiding this comment. Returning the builder opens up ways to override other params, thanks 👍 |
||
| return JiraUserPasswordOverridingDatabase.Builder( | ||
| databaseDelegate = this, | ||
| plainTextPassword = adminPasswordPlainText, | ||
| passwordEncrypted = adminPasswordEncrypted | ||
| ) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| package com.atlassian.performance.tools.infrastructure.api.database.passwordoverride | ||
|
|
||
| import com.atlassian.performance.tools.infrastructure.mock.MockSshSqlClient | ||
| import com.atlassian.performance.tools.infrastructure.mock.RememberingSshConnection | ||
| import com.atlassian.performance.tools.ssh.api.SshConnection | ||
| import org.assertj.core.api.Assertions.assertThat | ||
| import org.junit.Before | ||
| import org.junit.Test | ||
|
|
||
| class CrowdEncryptedPasswordProviderTest { | ||
| private lateinit var sqlClient: MockSshSqlClient | ||
| private lateinit var sshConnection: RememberingSshConnection | ||
| private lateinit var tested: JiraUserEncryptedPasswordProvider | ||
| private val passwordPlainText = "abcde" | ||
| private val passwordEncrypted = "*****" | ||
|
|
||
| @Before | ||
| fun setup() { | ||
| sqlClient = MockSshSqlClient() | ||
| sshConnection = RememberingSshConnection() | ||
| tested = CrowdEncryptedPasswordProvider( | ||
| jiraDatabaseSchemaName = "jiradb", | ||
| sqlClient = sqlClient, | ||
| passwordPlainText = passwordPlainText, | ||
| passwordEncryptedWithAtlassianSecurityPasswordEncoder = passwordEncrypted | ||
| ) | ||
| } | ||
|
|
||
| @Test | ||
| fun shouldQueryEncryptionMethod() { | ||
| // given | ||
| sqlClient.queueReturnedSqlCommandResult( | ||
| SshConnection.SshResult( | ||
| exitStatus = 0, | ||
| output = """attribute_value | ||
| atlassian-security | ||
| """.trimMargin(), | ||
| errorOutput = "" | ||
| ) | ||
| ) | ||
| // when | ||
| tested.getEncryptedPassword(sshConnection) | ||
| // then | ||
| assertThat(sqlClient.getLog()) | ||
| .`as`("sql queries executed") | ||
| .containsExactly( | ||
| "select attribute_value from jiradb.cwd_directory_attribute where attribute_name = 'user_encryption_method';" | ||
| ) | ||
| } | ||
|
|
||
| @Test | ||
| fun shouldThrowExceptionWhenUnknownEncryption() { | ||
| // when | ||
| var exception: RuntimeException? = null | ||
| try { | ||
| tested.getEncryptedPassword(sshConnection) | ||
| } catch (e: RuntimeException) { | ||
| exception = e | ||
| } | ||
| // then | ||
| assertThat(exception).isNotNull() | ||
| assertThat(exception!!.message).isEqualTo("Unknown jira user password encryption type") | ||
| } | ||
|
|
||
| @Test | ||
| fun shouldReturnEncrypted() { | ||
| // given | ||
| sqlClient.queueReturnedSqlCommandResult( | ||
| SshConnection.SshResult( | ||
| exitStatus = 0, | ||
| output = """attribute_value | ||
| atlassian-security | ||
| """.trimMargin(), | ||
| errorOutput = "" | ||
| ) | ||
| ) | ||
| // when | ||
| val password = tested.getEncryptedPassword(sshConnection) | ||
| // then | ||
| assertThat(password).isEqualTo(passwordEncrypted) | ||
| } | ||
|
|
||
| @Test | ||
| fun shouldReturnPlainText() { | ||
| // given | ||
| sqlClient.queueReturnedSqlCommandResult( | ||
| SshConnection.SshResult( | ||
| exitStatus = 0, | ||
| output = """attribute_value | ||
| plaintext | ||
| """.trimMargin(), | ||
| errorOutput = "" | ||
| ) | ||
| ) | ||
| // when | ||
| val password = tested.getEncryptedPassword(sshConnection) | ||
| // then | ||
| assertThat(password).isEqualTo(passwordPlainText) | ||
| } | ||
|
|
||
| } |
Uh oh!
There was an error while loading. Please reload this page.