Unboundid #167

dvoet · 2018-06-11T02:55:27Z

implemented new version of directory and access policy daos using unboundid. I did not touch the schema dao, it still uses jndi. But I did turn off the jndi connection pool (schema dao is the only one to use it now and it just runs at the beginning)

Have you read CONTRIBUTING.md lately? If not, do that first.

I, the developer opening this PR, do solemnly pinky swear that:

I've followed the instructions if I've made any changes to the API, especially if they're breaking changes
I've updated the RC_XXX release ticket with any manual steps required to release this change
I've updated the FISMA documentation if I've made any security-related changes, including auth, encryption, or auditing

In all cases:

Get two thumbsworth of review and PO signoff if necessary
Verify all tests go green
Squash and merge; you can delete your branch after this unless it's for a hotfix. In that case, don't delete it!
Test this change deployed correctly and works on dev environment after deployment

dvoet · 2018-06-11T02:56:29Z

...main/scala/org/broadinstitute/dsde/workbench/sam/directory/DirectorySubjectNameSupport.scala


 /**
  * Created by dvoet on 6/6/17.
  */
-trait DirectorySubjectNameSupport extends JndiSupport {


the additions to this trait moved from JndiSupport

dvoet · 2018-06-11T02:59:04Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/Boot.scala

@@ -40,8 +45,17 @@ object Boot extends App with LazyLogging {
    implicit val materializer = ActorMaterializer()
    import scala.concurrent.ExecutionContext.Implicits.global

-    val accessPolicyDAO = new JndiAccessPolicyDAO(directoryConfig)
-    val directoryDAO = new JndiDirectoryDAO(directoryConfig)
+    val dirURI = new URI(directoryConfig.directoryUrl)


jndi supports a ldap url which is what we have in conifg. Unboundid does not so here is a little tap dance to figure it out.

dvoet · 2018-06-11T02:59:45Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/config/package.scala

@@ -88,7 +88,8 @@ package object config {
      config.getString("user"),
      config.getString("password"),
      config.getString("baseDn"),
-      config.getString("enabledUsersGroupDn")
+      config.getString("enabledUsersGroupDn"),
+      config.as[Option[Int]]("connectionPoolSize").getOrElse(20)


default value for backwards compatibility

gpolumbo-broad

I did not go over LdapDirectoryDAO and LdapPolicyDAO in terribly close detail. I scanned them quickly, but otherwise am counting on the tests and the coverage results from Coveralls to verify these classes. @dvoet if you want me to go over them in closer detail, just let me know.

gpolumbo-broad · 2018-06-11T13:24:28Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/Boot.scala

+      case unsupported => throw new WorkbenchException(s"unsupported directory url scheme: $unsupported")
+    }
+    val port = if (dirURI.getPort > 0) dirURI.getPort else defaultPort
+    val ldapConnectionPool = new LDAPConnectionPool(new LDAPConnection(socketFactory, dirURI.getHost, port, directoryConfig.user, directoryConfig.password), directoryConfig.connectionPoolSize)


Maybe we don't want to do anything now, but going off of @MatthewBemis's observation last week about setting the TTL on the connections, we might consider throwing in a call to https://docs.ldap.com/ldap-sdk/docs/javadoc/com/unboundid/ldap/sdk/LDAPConnectionPool.html#setMaxConnectionAgeMillis() at some point.

I am not ready to do anything yet... let's keep with current behavior and see how it goes

gpolumbo-broad · 2018-06-12T13:58:51Z

...main/scala/org/broadinstitute/dsde/workbench/sam/directory/DirectorySubjectNameSupport.scala

+  protected def dnMatcher(matchAttributeNames: Seq[String], baseDn: String): Regex = {
+    val partStrings = matchAttributeNames.map { attrName => s"$attrName=([^,]+)" }
+    partStrings.mkString("(?i)", ",", s",$baseDn").r
+  }


Are there tests for this?

this code was just moved, not changed. I don't know if there are specific tests but it is tested.

gpolumbo-broad · 2018-06-12T14:02:39Z

...main/scala/org/broadinstitute/dsde/workbench/sam/directory/DirectorySubjectNameSupport.scala

@@ -62,4 +80,7 @@ trait DirectorySubjectNameSupport extends JndiSupport {
      case _ => throw new WorkbenchException(s"not a group dn [$dn]")
    }
  }
+
+  protected def formattedDate(date: Date) = new SimpleDateFormat("yyyyMMddHHmmss.SSSZ").format(date)
+  protected def parseDate(date: String) = new SimpleDateFormat("yyyyMMddHHmmss.SSSZ").parse(date)


Nit, but extract string. Maybe add just a couple of basic tests.

gpolumbo-broad · 2018-06-12T14:21:03Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/directory/JndiDirectoryDAO.scala

  } recover {
-    case _: NoSuchAttributeException => ()  // don't fail if the member is already not in the group
+    case _: NoSuchAttributeException => false


Is this what we want? If I am trying to remove User1 from group foo and the user is already not present in the group, maybe that should be OK and just return true. In other words, I'm interpreting the method contract as: "When I return, the subject specified will no longer be a member of the group and I will return True. I will throw an exception if the group doesn't exist or some other error occurs while trying to remove the user from the group."

Alternatively, instead of just returning false, should we return an ErrorReport?

look in the DirectoryDAO trait for the docs of how this should work. In general though, if at the end of the call the user is not in the group the function is successful. The boolean output signifies whether or not the user as actually removed during the course of the function call. This is a common pattern (https://docs.oracle.com/javase/7/docs/api/java/util/Set.html#remove(java.lang.Object)).

It is not an error so no ErrorReport.

gpolumbo-broad · 2018-06-12T14:26:43Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/directory/JndiDirectoryDAO.scala

+      case Success(_) => true
+      case Failure(_: AttributeInUseException) => false
+      case Failure(regrets) => throw regrets
+    }


This method uses a Try block, whereas removeGroupMember uses just a withContext{...}recover{...}. Is there a reason why the alternative approaches are used? Since these methods are a matching pair, I think it makes sense for them to be consistent in implementation.

this class is on the chopping block anyway, the change was only made because it made sense to change the return type

gpolumbo-broad · 2018-06-12T15:17:14Z

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

-    directoryDAO.addGroupMember(resourceAndPolicyName, subject) recover {
-      case _: AttributeInUseException => // subject is already there
-    } andThen {
+    directoryDAO.addGroupMember(resourceAndPolicyName, subject).map(_ => ()) andThen {
      case Success(_) => fireGroupUpdateNotification(resourceAndPolicyName)


It is possible that addGroupMember returns Success(false), in which case, we probably don't need to call fireGroupUpdateNotification because nothing changed. Furthermore, the fact that the user wasn't added, is not bubbled up in any way here, which means addSubjectToPolicy did not successfully perform its task and that's not reported out in any way.

at the end of this function the subject is in the policy which is the desired result so everything is ok. I think you misunderstand what Success(false) means, it is not an error, it means the user was already there.

Technically we don't need to fireGroupUpdateNotification on Success(false) but I think that is needless complexity

gpolumbo-broad · 2018-06-12T15:19:34Z

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

-    directoryDAO.removeGroupMember(resourceAndPolicyName, subject) recover {
-      case _: NoSuchAttributeException => // subject already gone
-    } andThen {
+    directoryDAO.removeGroupMember(resourceAndPolicyName, subject).map(_ => ()) andThen {
      case Success(_) => fireGroupUpdateNotification(resourceAndPolicyName)


See comment for addSubjectToPolicy above. I'm not sure we want to treat Success(true) and Success(false) the same way.

we do, none of this is new functionality

gpolumbo-broad · 2018-06-12T15:28:08Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/util/JndiSupport.scala

@@ -58,20 +58,6 @@ trait JndiSupport {
    t.get
  }


I think we should we mark this method and/or class as deprecated and identify that it is still permitted to be used by the JndiSchemaDAO only.

Do we have a longer term task to refactor the JndiSchemaDAO to use unboundid? Or are we just going to leave it using JNDI?

gpolumbo-broad · 2018-06-12T16:49:44Z

src/test/scala/org/broadinstitute/dsde/workbench/sam/directory/LdapDirectoryDAOSpec.scala

@@ -48,6 +53,11 @@ class JndiDirectoryDAOSpec extends FlatSpec with Matchers with TestSupport with
      runAndWait(dao.createGroup(group))
    }

+    val conflict = intercept[WorkbenchExceptionWithErrorReport] {


The name of the spec is "JndiGroupDirectoryDAO" should "create... should be renamed to LdapGroupDirectoryDAO

dvoet · 2018-06-13T18:10:40Z

jenkins retest with leo fix merged

dvoet · 2018-06-14T13:23:10Z

jenkins retest

dvoet added 5 commits June 10, 2018 08:58

unboundid

1cbf9a3

wip

a320af0

wip

997d349

final removal of jndi

034eb9c

doc

83dee13

dvoet requested review from MatthewBemis and gpolumbo-broad June 11, 2018 02:55

dvoet commented Jun 11, 2018

View reviewed changes

dvoet added 3 commits June 11, 2018 08:34

test fix

2eb0048

test fix

c862977

add coverageReport to .travis

36e00c5

gpolumbo-broad suggested changes Jun 12, 2018

View reviewed changes

code review

552ae74

MatthewBemis approved these changes Jun 12, 2018

View reviewed changes

gpolumbo-broad approved these changes Jun 13, 2018

View reviewed changes

Merge branch 'develop' into unboundid

eafee5f

dvoet merged commit 99af22d into develop Jun 14, 2018

dvoet deleted the unboundid branch June 14, 2018 19:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unboundid #167

Unboundid #167

dvoet commented Jun 11, 2018

dvoet Jun 11, 2018

dvoet Jun 11, 2018

dvoet Jun 11, 2018

gpolumbo-broad left a comment

gpolumbo-broad Jun 11, 2018

dvoet Jun 12, 2018

gpolumbo-broad Jun 12, 2018

dvoet Jun 12, 2018

gpolumbo-broad Jun 12, 2018

gpolumbo-broad Jun 12, 2018

dvoet Jun 12, 2018

gpolumbo-broad Jun 12, 2018

dvoet Jun 12, 2018

gpolumbo-broad Jun 12, 2018

dvoet Jun 12, 2018

gpolumbo-broad Jun 12, 2018

dvoet Jun 12, 2018

gpolumbo-broad Jun 12, 2018

dvoet Jun 12, 2018

gpolumbo-broad Jun 12, 2018

dvoet Jun 12, 2018

dvoet commented Jun 13, 2018

dvoet commented Jun 14, 2018

Unboundid #167

Unboundid #167

Conversation

dvoet commented Jun 11, 2018

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

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

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

dvoet commented Jun 13, 2018

dvoet commented Jun 14, 2018