[CA-1258] Add a status check to LdapRegistrationDAO and use to check openDJ #519

s-rubenstein · 2021-04-08T19:55:46Z

Ticket:

PR checklist

I've followed the instructions if I've made any changes to the API, especially if they're breaking changes
I've filled out the Security Risk Assessment and attached the result to the JIRA ticket

src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/LdapRegistrationDAO.scala

coveralls · 2021-04-13T22:36:45Z

Coverage decreased (-0.09%) to 78.448% when pulling 4f5fe60 on sr-status-check-open-dj into c34ff18 on develop.

s-rubenstein · 2021-04-14T13:48:49Z

src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TestSamRoutes.scala

+    val dbRef = TestSupport.dbRef
+    val directoryConfig: DirectoryConfig = TestSupport.directoryConfig
+    val dirURI = new URI(directoryConfig.directoryUrl)
+    val ldapConnectionPool = new LDAPConnectionPool(new LDAPConnection(dirURI.getHost, dirURI.getPort, directoryConfig.user, directoryConfig.password), directoryConfig.connectionPoolSize)


I'm a bit concerned about this, because there's no close on the pool? But also I don't know another way to ensure we are testing the real logic. Is there an afterAll or something hook in this file where cleanup can happen?

gpolumbo-broad

Didn't get a chance to finish reviewing before I had to run to meetings, but I had a comment here that I wanted to get out for discussion at least.

src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/LdapRegistrationDAO.scala

gpolumbo-broad · 2021-04-14T20:11:59Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/service/StatusService.scala

@@ -18,6 +17,7 @@ import scala.concurrent.{ExecutionContext, Future}

 class StatusService(
    val directoryDAO: DirectoryDAO,
+    val registrationDAO: RegistrationDAO,


Wondering if we shouldn't just be explicit here and say:

Suggested change

val registrationDAO: RegistrationDAO,

val ldapRegistrationDAO: LdapRegistrationDAO,

For this class, we don't want a generic RegistrationDAO, we explicitly need it to be an LdapRegistrationDAO in order to satisfy the business requirement. This will require a little refactoring in Boot.scala too I think, but I think it would be worth it. As it is here, I had to trace all the calls back through Boot.scala just to make sure we were passing in the right kind of RegistrationDAO here, which was annoying in itself, and also doesn't prevent anyone from accidentally breaking this again in the future.

I realize this will mess with tests where we use MockRegistrationDAO. I don't know the answer off the top of my head and maybe it's something we can sort out with a discussion/mob/pair to see if we can make this better in some way.

We don't actually have a MockRegistrationDAO, interestingly enough! That said, I think changing it assumes that LDAP is how we communicate to our registration service. I might rather have it be checkRegistrationDBStatus rather than checkOpenDJStatus? Given that things changed with Postgres, I don't want to assume LDAP will be the way forwards forever.

Agreed that we typically don't want to assume that LDAP will be the way forwards forever (I hope the RegistrationDAO goes away entirely one day). However, in this case, I'd argue that we want the StatusService checks to be implementation-specific. Keeping it generic was how we missed this in the first place and didn't check OpenDJ's status for >1 year. If we change the implementation of the RegistrationDAO, we'd want to be forced to change the StatusService to check the new implementation of it so that we couldn't accidentally leave it checking OpenDJ

Ack. That seems reasonable in rationale to me. I will make the change.

I went down a rabbit hole on this, specifying it as LDAPRegistrationDao makes mocking it exceptionally hard, because SBT/Mockito doesn't know how to override a class, only the traits that we use. So the best we could do is spy on it and then override the spy. This is going to be super clunky across our tests. I think the best thing we can do is to rename the variable and make a comment, which will be both simpler, and also provide a check for us against future changes.

src/main/scala/org/broadinstitute/dsde/workbench/sam/service/StatusService.scala

marctalbott · 2021-04-14T17:22:56Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala

+      if (session.connection.isValid((2 seconds).toSeconds.intValue())) {
+        true
+      } else {
+        false
+      }


This can be simplified by just returning session.connection.isValid((2 seconds).toSeconds.intValue())

src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TestSamRoutes.scala

marctalbott · 2021-04-15T15:03:54Z

src/test/scala/org/broadinstitute/dsde/workbench/sam/service/StatusServiceSpec.scala

-  private def newStatusService(directoryDAO: DirectoryDAO) = {
-    new StatusService(directoryDAO, new NoExtensions {
+  // We need to mock this because Postgres does some automatic setup and gets unhappy with multiple instances of its DAO existing at once.
+  private def mockDirectoryDAOWithPostgresStatusCheck(dbReferenceOverride: DbReference = dbReference) = {


Same comment here re: duplicating the checkStatus implementation

So, this one we can't use the real postgres db, because postgres makes some assumptions that it will only get stood up once within the whole test suite, and we don't want to use a persistent postgres across all the suites here, because we want to have different DBRef objects to manage bringing it down or up, so we can't actually override directoryDao itself. (or at least, I couldn't figure out how to)

Could we fully mock out the DAOs here then? It doesn't feel right to be overriding our mocks with the real implementation. I'd prefer that we either use the real implementation with a real PostgresDirectoryDAO or we use the mock rather than relying on this combination. This goes for the MockDirectoryDAO in TestSamRoutes as well

marctalbott · 2021-04-15T15:08:19Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/service/StatusService.scala

@@ -18,6 +17,7 @@ import scala.concurrent.{ExecutionContext, Future}

 class StatusService(
    val directoryDAO: DirectoryDAO,
+    val registrationDAO: RegistrationDAO,


Agreed that we typically don't want to assume that LDAP will be the way forwards forever (I hope the RegistrationDAO goes away entirely one day). However, in this case, I'd argue that we want the StatusService checks to be implementation-specific. Keeping it generic was how we missed this in the first place and didn't check OpenDJ's status for >1 year. If we change the implementation of the RegistrationDAO, we'd want to be forced to change the StatusService to check the new implementation of it so that we couldn't accidentally leave it checking OpenDJ

src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/LdapRegistrationDAO.scala

s-rubenstein · 2021-04-20T19:45:49Z

Jenkins retest

s-rubenstein · 2021-04-22T14:47:02Z

Jenkins retest

s-rubenstein · 2021-04-22T20:05:46Z

Jenkins retest

marctalbott · 2021-04-23T21:34:10Z

.travis.yml

@@ -3,7 +3,7 @@ jdk:
 dist: trusty
 language: scala
 scala:
- 2.13.5
+- 2.12.8


Is this related to the Github actions stuff?

Yes, will revert.

marctalbott · 2021-04-26T19:36:28Z

src/test/scala/org/broadinstitute/dsde/workbench/sam/service/StatusServiceSpec.scala

-  private def newStatusService(directoryDAO: DirectoryDAO) = {
-    new StatusService(directoryDAO, new NoExtensions {
+  // We need to mock this because Postgres does some automatic setup and gets unhappy with multiple instances of its DAO existing at once.
+  private def mockDirectoryDAOWithPostgresStatusCheck(dbReferenceOverride: DbReference = dbReference) = {


Could we fully mock out the DAOs here then? It doesn't feel right to be overriding our mocks with the real implementation. I'd prefer that we either use the real implementation with a real PostgresDirectoryDAO or we use the mock rather than relying on this combination. This goes for the MockDirectoryDAO in TestSamRoutes as well

gpolumbo-broad

One method naming nit, but it's fine.

.travis.yml

gpolumbo-broad · 2021-05-03T17:34:06Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/RegistrationDAO.scala

@@ -11,6 +12,7 @@ import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext
  * away from a solution that requires that the Apache proxies query this group, we can remove the RegistrationDAO.
  */
 trait RegistrationDAO {
+  def getConnectionTarget(): ConnectionType


Why not getConnectionType?

Because I am bad at naming :D

s-rubenstein · 2021-05-06T17:25:34Z

Jenkins retest

s-rubenstein · 2021-05-06T20:08:55Z

Jenkins retest

Add a status check to LdapRegistrationDAO and use to check openDJ

9041852

s-rubenstein requested review from andy7i and marctalbott April 8, 2021 19:55

Compiling, tests not passing

daaa728

gpolumbo-broad suggested changes Apr 12, 2021

View reviewed changes

src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/LdapRegistrationDAO.scala Outdated Show resolved Hide resolved

s-rubenstein added 2 commits April 13, 2021 17:56

Working StatusServiceSpec tests

c7d6ad0

Fix mocks to work successfully for status checks

bb5f321

s-rubenstein commented Apr 14, 2021

View reviewed changes

gpolumbo-broad suggested changes Apr 14, 2021

View reviewed changes

marctalbott requested changes Apr 15, 2021

View reviewed changes

s-rubenstein added 2 commits April 15, 2021 15:59

[NOT WORKING] Mock registration DAO

9ccca46

Move to enums, add mockregistrationdao

7002099

s-rubenstein added 4 commits April 21, 2021 10:46

Change a string

bdf92ea

Testing

4e7461b

More testing

f5bfdb8

Try scala version different

89f8833

Remove version commands

a6b8ef6

PR Feedback

4e4d78c

marctalbott reviewed Apr 26, 2021

View reviewed changes

PR Feedback

64e80f6

marctalbott approved these changes Apr 28, 2021

View reviewed changes

gpolumbo-broad approved these changes May 3, 2021

View reviewed changes

Change name to connectionType

4f5fe60

s-rubenstein merged commit 80f3066 into develop May 6, 2021

s-rubenstein deleted the sr-status-check-open-dj branch May 6, 2021 20:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CA-1258] Add a status check to LdapRegistrationDAO and use to check openDJ #519

[CA-1258] Add a status check to LdapRegistrationDAO and use to check openDJ #519

s-rubenstein commented Apr 8, 2021

coveralls commented Apr 13, 2021 •

edited

Loading

s-rubenstein Apr 14, 2021

gpolumbo-broad left a comment

gpolumbo-broad Apr 14, 2021

s-rubenstein Apr 15, 2021

marctalbott Apr 15, 2021

s-rubenstein Apr 15, 2021

s-rubenstein Apr 26, 2021

marctalbott Apr 14, 2021

marctalbott Apr 15, 2021

s-rubenstein Apr 26, 2021

marctalbott Apr 26, 2021

marctalbott Apr 15, 2021

s-rubenstein commented Apr 20, 2021

s-rubenstein commented Apr 22, 2021

s-rubenstein commented Apr 22, 2021

marctalbott Apr 23, 2021

s-rubenstein Apr 27, 2021

marctalbott Apr 26, 2021

gpolumbo-broad left a comment

gpolumbo-broad May 3, 2021

s-rubenstein May 3, 2021

s-rubenstein commented May 6, 2021

s-rubenstein commented May 6, 2021

	val registrationDAO: RegistrationDAO,
	val ldapRegistrationDAO: LdapRegistrationDAO,

[CA-1258] Add a status check to LdapRegistrationDAO and use to check openDJ #519

[CA-1258] Add a status check to LdapRegistrationDAO and use to check openDJ #519

Conversation

s-rubenstein commented Apr 8, 2021

coveralls commented Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

gpolumbo-broad left a 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

Choose a reason for hiding this comment

s-rubenstein commented Apr 20, 2021

s-rubenstein commented Apr 22, 2021

s-rubenstein commented Apr 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpolumbo-broad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-rubenstein commented May 6, 2021

s-rubenstein commented May 6, 2021

coveralls commented Apr 13, 2021 •

edited

Loading