[User management] Terminate ongoing streams when user state has changed [DPP-830] #12437

pbatko-da · 2022-01-17T14:48:33Z

No description provided.

...-api-auth/src/main/scala/com/digitalasset/ledger/api/auth/OngoingAuthorizationObserver.scala

pbatko-da · 2022-01-19T08:52:53Z

...-api-auth/src/main/scala/com/digitalasset/ledger/api/auth/OngoingAuthorizationObserver.scala

+    * if any change in compared the original user claims has been detected
+    * or if user claims change check times out.
+    */
+  private def validateClaimsResolvedFromUser(now: Instant): Unit = {


Core logic here

CHANGELOG_BEGIN CHANGELOG_END

meiersi-da

Good stuff @pbatko-da ! This goes in exactly the right direction. I think we do though want to revisit the core a logic a bit wrt how it deals with quiescent streams. See my comments.

ledger/error/src/main/scala/com/daml/error/definitions/LedgerApiErrors.scala

...-api-auth/src/main/scala/com/digitalasset/ledger/api/auth/OngoingAuthorizationObserver.scala

meiersi-da · 2022-01-21T13:08:32Z

...-api-auth/src/main/scala/com/digitalasset/ledger/api/auth/OngoingAuthorizationObserver.scala

+  private def authorize: Either[AuthorizationError, Unit] = {
+    val now = nowF()
+    for {
+      _ <- originalClaims.notExpired(now)
+      _ = validateClaimsResolvedFromUser(now)
+    } yield {
+      ()
+    }
+  }


Inline the two calls into the onNext case distinction above for clarity.

meiersi-da · 2022-01-21T13:13:37Z

...-api-auth/src/main/scala/com/digitalasset/ledger/api/auth/OngoingAuthorizationObserver.scala

+  /** Aborts the stream by throwing an exception
+    * if any change in compared the original user claims has been detected
+    * or if user claims change check times out.
+    */


The code in here works with the limitation that a user rights check is only initiatiated on seeing a new stream element, which means that quiet streams won't be terminated in a timely fashion on user right changes or expiry.

If it's not too much effort, then I'd prefer to not have this problem and terminate in a timely fashion. This will require running the user rights check independently of the stream in a periodic fashion. That would also allow to adapt the tests to not require sending an extra element.

@nmarton-da : what is the recommended way to do a check periodically in Scala? If found this https://stackoverflow.com/a/39938355, but I'm sure we already have place where we do that.

My initial question would be which and/or how many threads do want to spare with these checks.

If one new thread per all streams then java.util.Timer looks like a good fit.
If we predict needing more than one dedicated thread then should it be a dedicated thread pool or one that we already use?

Will probably start by looking at scheduling with Akka

If it's not too much effort, then I'd prefer to not have this problem and terminate in a timely fashion.

Ok

ledger/sandbox-classic/src/test/suite/scala/platform/sandbox/auth/OngoingStreamAuthIT.scala

meiersi-da · 2022-01-21T13:21:23Z

ledger/sandbox-classic/src/test/suite/scala/platform/sandbox/auth/OngoingStreamAuthIT.scala

+                  "definite_answer" -> "false",
+                ),
+              ),
+              ErrorDetails.RetryInfoDetail(retryDelayInSeconds = 1),


Why do we have a 1 second delay here? I'd expect we want to recommend an immediate retry wouldn't we?

This is a default from ContentionOnSharedResources error category.

What do you think is better: 0.seconds or 1.millisecond

Changed to 0.seconds

…piErrors.scala Co-authored-by: Simon Meier <[email protected]>

Co-authored-by: Simon Meier <[email protected]>

ledger/error/src/main/scala/com/daml/error/definitions/LedgerApiErrors.scala

…piErrors.scala Co-authored-by: tudor-da <[email protected]>

mziolekda · 2022-01-26T16:32:32Z

...-api-auth/src/main/scala/com/digitalasset/ledger/api/auth/OngoingAuthorizationObserver.scala

+    if (
+      originalClaims.resolvedFromUser &&
+      lastUserRightsCheckTime.isAfter(
+        now.plusSeconds(2 * userRightsCheckIntervalInSeconds.toLong)


I think the 2 * bit is a misunderstanding. We should check every userRightsCheckIntervalInSeconds. While doing so we guarantee that in the worst case we will be 2 * cacheExpiryAfterWriteInSeconds behind the update

Makes sense!

Discussed offline as staying with 2 *

mziolekda · 2022-01-26T16:45:50Z

...-api-auth/src/main/scala/com/digitalasset/ledger/api/auth/OngoingAuthorizationObserver.scala

-    throwOnFailure: AuthorizationError => Throwable,
-) extends ServerCallStreamObserver[A] {
+    originalClaims: ClaimSet.Claims,
+    nowF: () => Instant,


I see how this could become useful, but AFAICT it is not set to a "fake" clock in any of the tests, is it?

It would be useful in a unit test for OngoingAuthorizationObserver but such test is missing from this PR

language-support/java/bindings-rxjava/src/test/scala/com/daml/ledger/rxjava/package.scala

nmarton-da · 2022-01-27T08:07:11Z

...-api-auth/src/main/scala/com/digitalasset/ledger/api/auth/OngoingAuthorizationObserver.scala

+      val delay = userRightsCheckIntervalInSeconds.seconds
+      // Note: https://doc.akka.io/docs/akka/2.6.13/scheduler.html states that:
+      // "All scheduled task will be executed when the ActorSystem is terminated, i.e. the task may execute before its timeout."
+      val c = akkaScheduler.scheduleWithFixedDelay(initialDelay = delay, delay = delay)(runnable =


Akka scheduler is a good approach, if you need it scalable, and don't care much about precision (I think this is the case here)

nmarton-da · 2022-01-27T08:09:56Z

...-api-auth/src/main/scala/com/digitalasset/ledger/api/auth/OngoingAuthorizationObserver.scala

+        case Success(Right(userRights)) =>
+          val updatedClaims = AuthorizationInterceptor.convertUserRightsToClaims(userRights)
+          if (updatedClaims.toSet != originalClaims.claims.toSet) {
+            self.synchronized(observer.onError(staleStreamAuthError))


why do you need to sync for this? looks suspicious 🤔
(btw self.synchronized is just synchronized, no? or if you want to make it explicit porbably it would be better to pick the observer for synchronization)
also: if you need that synchronized, probably would be better to wrap the observer into a synchronized observer or something, and not worry about synchronization code in here (hard to prove every call is synchronized)

why do you need to sync for this?

I synchronized all calls to onNext, onError, onComplete and setMessageCompression. Other methods either are documented to be safe to be called from multiplethreads or have to be called early in the lifecycle.
From io.grpc.stub.ServerCallStreamObserver:

Like StreamObserver, implementations are not required to be thread-safe; if multiple threads will be writing to an instance concurrently, the application must synchronize its calls.

Synchronized observer sounds great

Changed approach to synchronize methods of OngoingAuthorizationObserver rather than just mehtods on the delegate instance

mziolekda · 2022-01-28T10:27:52Z

ledger/ledger-api-auth/src/main/scala/com/digitalasset/ledger/api/auth/Authorizer.scala

@@ -324,8 +331,21 @@ object Authorizer {
      ledgerId: String,
      participantId: String,
      errorCodesVersionSwitcher: ErrorCodesVersionSwitcher,
+      userManagementStore: UserManagementStore,


I can see that apply is only ever used in the tests. Would it not make sense to hide it behind a method called testAuthorizer and then to encapsulate the repetitious boiler plate in this method? WDYT?

mziolekda

Feel free to address my remaining comment in a follow up PR or challenge it

@meiersi-da

All comments from @meiersi-da have been addressed

kamil-da · 2022-01-28T15:19:32Z

...src/test/suite/scala/com/digitalasset/ledger/api/auth/OngoingAuthorizationObserverSpec.scala

+
+  private val loggingContext = LoggingContext.ForTesting
+
+  it should "signal onError aborting the stream when user rights state hasn't been refreshed in a timely manner" in {


Shouldn't we have a test case for claims changing over time?

That's tested in OngoingStreamAuthIT

That's correct but the comments of the OngoingAuthorizationObserver class state that the class will ensure stream termination on changing user rights state. Even if that would be a test duplication, I'd still add such test case to OngoingAuthorizationObserverSpec.

@adriaanm-da

This PR has been created by a script, which is not very smart and does not have all the context. Please do double-check that the version prefix is correct before merging. @adriaanm-da is in charge of this release. Commit log: ``` 5390505 Remove participant-side command deduplication [DPP-848] (#12677) 85f2690 split release (#12700) ed1bf24 LF: Check activeness of cached contracts inside FetchInterface (#12698) aa2494f Speedy: check contract type after checking if they are consumed (#12691) 183f936 LF: Imporve safety of the Serialization of proto message. (#12686) b4ed15b Changed product names in the documentation (#12668) 5caaf1f Switch github urls from zip to tar.gz (#12692) 6cdda6f Metering report [DPP-817] (#12604) 716cc22 Deduplication offset period errors handled in Sandbox-on-X [DPP-872] (#12652) e4764cc Upgrade to GHC 9.0.2 (#12300) 1238e1b Update profiling documentation to Canton sandbox (#12680) f1c7e9e Drop `daml ledger navigator` in favor of `daml navigator` (#12669) 7731324 Drop profile-dir validation for sandbox-kv (#12671) 1fa095a match Foldable.foldl1's argument order to DA.List.foldl1's order (#12685) 06b63e9 update NOTICES file (#12688) 2ac76fd LF: Gracefully handle serialization error of Values beyond 2GB (#12638) 0494731 Adjust response type for script JSON API user mgmt requests (#12683) 7aabc49 Update OpenTelemetry from 0.16.0 to 1.1.0 [KVL-1256] (#12568) 117c920 Bump sandbox-on-x-it-tests size (#12681) 100c59f [JSON-API] Docs for user management endpoints (#12432) 92938d5 Moved com.daml.platform.sandbox to com.daml.ledger.sandbox in SoX (#12675) 8d5d3bd [JSON-API] User management endpoint adjustments (#12578) 54c427f Expose documentation for user-management functions in daml-script. (#12674) c1afabe [participant-state] Add earliest_offset metadata to pruned data error [kvl-1270] (#12546) ef18bf4 DEV all caps needed to exclude dev dependencies (#12679) 3d3d84f User management in daml-script over json API (#12646) 182edde [compatiblity tests] Limit the exclusion to versions that have the CommandDeduplication:Participant* tests (#12660) f08dfa3 Bump terraform (#12670) 366cd89 Add new interface serializability tests (#12666) 84cec38 Upgrade ghc-lib (#12664) d215a01 Enable user management for the Canton sandbox (#12667) bb5722c Move sandbox-classic test lib and IT tests to Sandbox-on-x (#12641) c9a65d1 update NOTICES file (#12659) 1fa7f61 ci: pin workdirs on Windows (#12645) dcbb398 Typecheck experimental primitives in damlc (#12650) 0d5443f Drop direct dependencies on system-filepath (#12658) 49f37b8 Allow defining ledger-begin and ledger-end offset conditions in ledger-api-bench-tool [DPP-836] (#12521) f1560ce Support `implements` qualified interfaces (#12644) 20836b1 Address CVE-2022-0355 alert, resolve `simple-get` to 4.0.1 (#12655) 49e6646 Clean up unstable-types test (#12648) 214951e Fix log string interpolation in AkkaExecutionSequencer (#12647) c72c27c [User management] Terminate ongoing streams when user state has changed [DPP-830] (#12437) 35eae89 Compiler: expose LF builtin ExerciseByKey (#12615) dfdb7ce Remove DA.Generics (#12634) 14f43e4 update NOTICES file (#12642) cfa8d30 [Sandbox-on-X] Ledger-side in-memory command deduplication [DPP-872] (#12596) 7567cf5 Add scala serializability checks for interfaces (#12631) 39a421e Drop unused silencer variable (#12640) 4ec336d [User management] Enforce 1k user rights limit [DPP-833] (#12558) 16a13e2 Remove redundant conformance tests from ledger-on-sql (#12639) 0610a44 daml-react: add an useUser hook (#12622) 579fb20 Reenable Canton contract id tests (#12637) 60a0f03 Hide primitiveInterface from docs (#12635) 536e493 Bump canton to latest snapshot (#12633) 613e070 Wrap client instead of runner in script test (#12630) d5ede55 Turn name collision warnings for virtual modules into errors (#12627) d5ae82d Bump cached-path-relative from 1.0.2 to 1.1.0 (#12632) f4c2862 Split release 2022-01-27 (#12624) 9e5425e update NOTICES file (#12613) fcb6124 Simplify Nix GHC derivation (#12620) 5a47223 ledger-on-memory: Do not create iterators on a mutable log. (#12626) d2216b3 Clean up unused test and unnecessary flags in ledger-on-sql tests (#12623) 345e267 create-daml-app: Use alias templates for display names (#12390) 2e3ae0d Disable ghcide snapshot on Windows (#12621) e55622e Remove a bunch of ledger-on-memory tests (#12618) f5c9a67 Remove DA.Experimental.Interfaces (#12619) 5810c25 release 2.0.0-snapshot.20220127.9042.0.4038d0a7 (#12614) f396da5 Run ledger-api-test-tool tests on ledger-on-sql with PostgreSQL (#12616) 89ce7d0 Remove conflict-checking logic from JdbcLedgerDao [DPP-808] (#12555) ``` Changelog: ``` with experimental support for non-aggregated metering reporting - [Daml Standard Library] An argument order in the default implementation of ``Foldable.foldl1`` was reversed from that of ``DA.List.foldl1``; this incompatibly changes the former to match the latter. - [HTTP-JSON] Added documentation for the new user management endpoints - [HTTP-JSON] Adjusted the response for the createUser & deleteUser endpoint to an empty object instead of "true". This was done to allow possible future extensions to the return type without applying breaking changes. - [HTTP-JSON] Made the field rights optional for the request body of the createUser endpoint. Now it is possible to create a user with just the userId (i.e. { "userId": "nice.user" }). - [Integration Kit] - ledger-api-bench-tool allow to define ledger-begin and ledger-end offset boundaries for streams. Ledger API Specification: When using user management based authorization streams will now get aborted on authenticated user's rights change. - [Daml Standard Library] The DA.Generics module has been removed. Ledger API Specification: Maximum number of user rights per user is now limited to 1000 and is added to UserManagementFeature in VersionService. getLedgerApiVersion endpoint. [daml-react] A `useUser` react hook is added to the daml-react TypeScript library. It allows for easy access to the currently logged in user of a ledger participant node for ledgers supporting user management. ``` CHANGELOG_BEGIN CHANGELOG_END

...-api-auth/src/main/scala/com/digitalasset/ledger/api/auth/OngoingAuthorizationObserver.scala

pbatko-da commented Jan 17, 2022

View reviewed changes

...-api-auth/src/main/scala/com/digitalasset/ledger/api/auth/OngoingAuthorizationObserver.scala Outdated Show resolved Hide resolved

pbatko-da force-pushed the pbatko/persistent-user-management-stream-auth branch from 2e6e48f to 54a7efc Compare January 18, 2022 14:55

Base automatically changed from pbatko/persistent-user-management to main January 18, 2022 14:59

pbatko-da force-pushed the pbatko/persistent-user-management-stream-auth branch from 54a7efc to 3bbc731 Compare January 18, 2022 15:20

pbatko-da commented Jan 19, 2022

View reviewed changes

pbatko-da force-pushed the pbatko/persistent-user-management-stream-auth branch from 3bbc731 to fb23f07 Compare January 20, 2022 19:31

[User management] Terminate ongoing streams when user state has changed.

16c30e1

CHANGELOG_BEGIN CHANGELOG_END

pbatko-da force-pushed the pbatko/persistent-user-management-stream-auth branch from fb23f07 to 16c30e1 Compare January 20, 2022 19:33

pbatko-da marked this pull request as ready for review January 20, 2022 19:33

pbatko-da requested review from stefanobaghino-da and a team as code owners January 20, 2022 19:33

pbatko-da changed the title ~~[User management] Ongoing stream authorization~~ [User management] Terminate ongoing streams when user state has changed Jan 20, 2022

pbatko-da changed the title ~~[User management] Terminate ongoing streams when user state has changed~~ [User management] Terminate ongoing streams when user state has changed [DPP-830] Jan 21, 2022

pbatko-da added 2 commits January 21, 2022 10:18

Fix deps

e075198

Merge branch 'main' into pbatko/persistent-user-management-stream-auth

be2e631

meiersi-da previously requested changes Jan 21, 2022

View reviewed changes

pbatko-da and others added 8 commits January 21, 2022 14:31

Update ledger/error/src/main/scala/com/daml/error/definitions/LedgerA…

a20663b

…piErrors.scala Co-authored-by: Simon Meier <[email protected]>

Update ledger/error/src/main/scala/com/daml/error/definitions/LedgerA…

81e9a12

…piErrors.scala Co-authored-by: Simon Meier <[email protected]>

Apply suggestions from code review

fae4d5d

Co-authored-by: Simon Meier <[email protected]>

Use Akka's scheduler

1f8473a

Cancel streams in a timely manner even with quiescent streams

641c406

Address outstanding PR comments and remove 'aborted' boolean

744b267

Merge branch 'main' into pbatko/persistent-user-management-stream-auth

b211153

local renames

8933568

pbatko-da requested a review from meiersi-da January 24, 2022 15:24

small fix

49b9219

tudor-da reviewed Jan 24, 2022

View reviewed changes

ledger/error/src/main/scala/com/daml/error/definitions/LedgerApiErrors.scala Outdated Show resolved Hide resolved

pbatko-da and others added 2 commits January 25, 2022 09:52

Merge branch 'main' into pbatko/persistent-user-management-stream-auth

63f03e7

Update ledger/error/src/main/scala/com/daml/error/definitions/LedgerA…

ffe5840

…piErrors.scala Co-authored-by: tudor-da <[email protected]>

pbatko-da added 2 commits January 25, 2022 10:40

Merge branch 'main' into pbatko/persistent-user-management-stream-auth

857bb87

fix tests

2f7fe4d

pbatko-da requested a review from tudor-da January 25, 2022 16:33

mziolekda reviewed Jan 26, 2022

View reviewed changes

nmarton-da reviewed Jan 27, 2022

View reviewed changes

uni test for OngoingAuthorizationObserver

502b1f7

pbatko-da requested review from nmarton-da and mziolekda January 27, 2022 15:28

mziolekda reviewed Jan 28, 2022

View reviewed changes

mziolekda approved these changes Jan 28, 2022

View reviewed changes

pbatko-da added 2 commits January 28, 2022 14:31

Increase task tiemout to 2*scheduleDealy and add more docs

72f6ecd

fix doc -> comment

c7b3260

kamil-da reviewed Jan 28, 2022

View reviewed changes

pbatko-da merged commit c72c27c into main Jan 28, 2022

pbatko-da deleted the pbatko/persistent-user-management-stream-auth branch January 28, 2022 15:44

nmarton-da reviewed Feb 9, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[User management] Terminate ongoing streams when user state has changed [DPP-830] #12437

[User management] Terminate ongoing streams when user state has changed [DPP-830] #12437

pbatko-da commented Jan 17, 2022

pbatko-da Jan 19, 2022

meiersi-da left a comment

meiersi-da Jan 21, 2022

pbatko-da Jan 24, 2022

meiersi-da Jan 21, 2022

pbatko-da Jan 21, 2022 •

edited

Loading

pbatko-da Jan 21, 2022

pbatko-da Jan 24, 2022 •

edited

Loading

meiersi-da Jan 21, 2022

pbatko-da Jan 21, 2022

pbatko-da Jan 24, 2022

mziolekda Jan 26, 2022

pbatko-da Jan 26, 2022

pbatko-da Jan 28, 2022

mziolekda Jan 26, 2022

pbatko-da Jan 26, 2022

nmarton-da Jan 27, 2022

nmarton-da Jan 27, 2022

pbatko-da Jan 27, 2022

pbatko-da Jan 27, 2022

mziolekda Jan 28, 2022

mziolekda left a comment •

edited

Loading

kamil-da Jan 28, 2022

pbatko-da Jan 28, 2022

kamil-da Jan 28, 2022


		private val loggingContext = LoggingContext.ForTesting

		it should "signal onError aborting the stream when user rights state hasn't been refreshed in a timely manner" in {

[User management] Terminate ongoing streams when user state has changed [DPP-830] #12437

[User management] Terminate ongoing streams when user state has changed [DPP-830] #12437

Conversation

pbatko-da commented Jan 17, 2022

Choose a reason for hiding this comment

meiersi-da 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

pbatko-da Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbatko-da Jan 24, 2022 • edited Loading

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

mziolekda left a comment • edited Loading

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

pbatko-da Jan 21, 2022 •

edited

Loading

pbatko-da Jan 24, 2022 •

edited

Loading

mziolekda left a comment •

edited

Loading