Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add gRPC definitions for participant user management service #11818

Merged
merged 2 commits into from
Nov 25, 2021

Conversation

meiersi-da
Copy link
Contributor

@meiersi-da meiersi-da commented Nov 22, 2021

This PR introduces the gRPC definitions for an experimental user management service to manage users and their rights for interacting with the Ledger API served by a participant node. The PR does not change the set of actually exposed services of a Ledger API server.

I expect to implement the user management service in a later PR behind a feature flag such that we can start a PoC phase to evaluate whether all the Daml SDK tools can be made to work well (or hopefully better) building on this user management service. I expect that we'll iterate further on the service's gRPC definitions as part of this PoC phase.

@bame-da @cocreature @stefanobaghino-da could I ask one of you to approve the PR so we can start the PoC implementation work?

FYI: @da-tanabe @adriaanm-da @gerolf-da and others, feel free to review the code directly on this PR and add comments here. I will then take care of addressing the comments on a new PR in case this one is already merged.

Pull Request Checklist

  • Read and understand the contribution guidelines
  • Include appropriate tests
  • Set a descriptive title and thorough description
  • Add a reference to the issue this PR will solve, if appropriate
  • Include changelog additions in one or more commit message bodies between the CHANGELOG_BEGIN and CHANGELOG_END tags
  • Normal production system change, include purpose of change in description
  • If you mean to change the status of a component, please make sure you keep the Component Status page up to date.

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.

@meiersi-da meiersi-da requested review from a team as code owners November 22, 2021 16:20
Comment on lines 24 to 23
// 1. List methods do not (yet) support pagination,
// as we expect to have fewer than 10k users and 1k rights per user.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I largely agree with these numbers, but given #10283 this could be a tough API to change later…

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My aim would be to add pagination support as per https://cloud.google.com/apis/design/design_patterns#list_pagination

This can be done in a backwards compatible way, by assuming a default limit in case none is specified.

Copy link
Contributor

@bame-da bame-da left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All makes sense to me. My main question is why you are designing this as extra services rather than just keeping users and rights on-ledger.

template User
  with
    participant : Party
    userParty : Party
    userName : Text
  where
    ensure (valid userName)
    signatory participant
    observer userParty

data Right
  = Admin
  | ReadAs Party
  | ActAs Party

template UserRight
  with
    participant : Party
    user : Party
    right : Right
  where
    signatory participant
    observer user

Now the API is given by the transaction, acs, and command APIs, and the authorization is dealt with in the Daml Model.

@bame-da
Copy link
Contributor

bame-da commented Nov 23, 2021

If we do this in Daml there's a question of causality and transactionality. Ie I could imagine two models:

  1. The user rights are resolved once at the beginning of command processing. So the following sequence of events is valid
    A. Admin submits TX1: Delete User Alice
    B. Alice Submits TX2
    C. TX1 completes
    D. TX2 completes
  2. The user rights are resolved as part of validation and can change during a transaction. So the above is invalid, but the following is valid, allowing for "transient" user rights:
    A. Alice submits a transaction with events
    1. Exercise delegated choice to add ReadAs right for user Alice on Party Bob
    2. Fetch contract known only to Bob
    3. Exercise delegated choice to remove ReadAs right for user Alice on Party Bob

When I talk about modelling users in Daml, I have model 1 in mind for now.

@meiersi-da
Copy link
Contributor Author

meiersi-da commented Nov 23, 2021

All makes sense to me. My main question is why you are designing this as extra services rather than just keeping users and rights on-ledger.

@bame-da: I agree that we ultimately want to end up with using Daml for managing the participant-local state as well. The main motivation for introducing this API is to manage technical risk. The API proposed here is simple and boring, and thus has a low chance of unforeseen implementation problems.

When building user management directly in Daml, we for example have to consider how to upgrade the User contracts in the future to add new features without breaking all clients interacting with the current version. Likewise, there is a potential bootstrapping problem for creating the first user contract that allocates the 'admin' user with admin rights. I believe we can and should discover and solve these problems, but not on a tight deadline.

Instead, I propose that we introduce a simple gRPC API that

  1. covers the needs on which we have a tight deadline, and
  2. can be implemented both with direct DB access and also using on-ledger Daml contracts.
    This way we can first deliver the API using direct DB access, and then iterate on it in a backwards-compatible way using on-ledger Daml contracts as the backing storage.

What do you think?

@meiersi-da meiersi-da marked this pull request as draft November 23, 2021 15:45
@bame-da
Copy link
Contributor

bame-da commented Nov 23, 2021

What do you think?

Makes sense, you are almost certainly right that this is the lower-risk route.

@meiersi-da meiersi-da changed the title Draft: participant user management api Add gRPC definitions for participant user management service Nov 25, 2021
@meiersi-da meiersi-da marked this pull request as ready for review November 25, 2021 08:13
Only gRPC definitions are added. No new service is exposed by the Ledger
API server.

CHANGELOG_BEGIN

- [Ledger API] Introduce gRPC definitions for experimental user
  managament service to manage users and their rights for interacting
  with the Ledger API served by a participant node.

CHANGELOG_END
@meiersi-da meiersi-da force-pushed the meiersi-da/user-account-management-ledger-api branch from a6657b5 to 72ef34d Compare November 25, 2021 08:25
Copy link
Contributor

@cocreature cocreature left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

The API looks very reasonable. So far, I think I’m fairly confident we can implement this for 2.0.

My main concern is the amount of breakage we might cause for our users and our examples if we try to change e.g., Daml Script to work with users by default. But that definitely doesn’t need to block this PR and I also haven’t thought too deeply about it so far.

@stefanobaghino-da
Copy link
Contributor

I'll make sure to find time to review the design and this PR by the end of the day.

@meiersi-da meiersi-da merged commit 026b92a into main Nov 25, 2021
@meiersi-da meiersi-da deleted the meiersi-da/user-account-management-ledger-api branch November 25, 2021 11:26
@gerolf-da
Copy link
Contributor

cc @mziolekda

rpc CreateUser (CreateUserRequest) returns (User);

// Get the user data of a specific user or the authenticated user.
rpc GetUser (GetUserRequest) returns (User);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a chance that we want to transmit other metadata than what is contained in User?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gerolf-da : I'm following the Google API Style guidelines here: https://cloud.google.com/apis/design/standard_methods

If we really require more info to be transmitted, then we should introduce a custom method: https://cloud.google.com/apis/design/custom_methods

repeated Right rights = 2;
}

message GrantUserRightsResponse {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's useful to also include all rights (or just the set of already existing rights) in the response. If I think about the usage of this in a GUI, I'd probably want to refresh the user's rights with a definitive authoritative answer about all rights the user has.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason why these rights are not included is that this won't scale to large sets of rights.

@stefanobaghino-da
Copy link
Contributor

Sorry, couldn't review today, I'll need to do it tomorrow.

}

message ListUserRightsResponse {
repeated Right rights = 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Daml script, the natural thing to do seems to be to flip this around immediately and turn it into:

data Rights = Rights
  { actAs : [Party]
  , readAs : [Party]
   , admin : Bool
   }

I could see other clients wanting to do the same thing (the JSON API for example). Have you considered emitting the result in something closer to that format?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried it, and it was not easy to present that way. The flat set of rights is a simple, well-defined structure that lends itself well to extensions.

azure-pipelines bot pushed a commit that referenced this pull request Dec 1, 2021
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.

@SamirTalwar-DA is in charge of this release.

Commit log:
```
683ab87 Move ghc-lib{,-parser} to bazel-haskell-deps (#11775)
9350632 Fix releasing of resources in case connection initialization failed (#11915)
e1559af Update `ModelConformanceValidator` comments and prevent them from getting outdated easily (#11924)
16a41f7 Avoid package validation in speedy compilation benchmark. (#11927)
16135e6 Limit supported input versions in damlc to >= LF 1.8 (#11905)
0ee4154 Use Absolute-indexes as keys for the Env-mapping during closure-conversion (#11912)
1d7bca8 Add optional typerep argument in UExerciseInterface. (#11910)
c2c22f8 kvutils: Protos no longer depend on the Daml-LF transaction proto [KVL-1166] (#11909)
5641948 [Docs] Add labels to error codes to support references to them (#11913)
0e77676 Update protobuf docs template to handle oneOf (#11887)
5a9481f unify heading markup according to README.md (#11919)
61334cf kvutils - Add Writer which can handle deduplication periods as offsets [KVL-1172] (#11900)
0b9d57b Add ContractDoesntImplementInterface error. (#11884)
49e5d41 align index.rst files for HTML and PDF (#11907)
dbbb05f Split daml-lf encode/decode Haskell libraries (#11906)
e5d3902 iface: support for fixed choices in TS codegen (#11630)
31cc540 Turn package name & version warnings into an error (#11859)
4e50060 self-service compat: set branch name to not main (#11902)
2f4aa47 refactor to avoid impossible code path (#11901)
a81995c switch dev images to Temurin (#11895)
f3a0e2e Set scalafmt dialect explicitly (#11898)
60e372d Don't run pruning tests on H2, they time-out (#11897)
58e69ad LF: replace "dev" LF version by "1.dev" in bazel files (#11894)
8ef348d Use absolute stack locations in SExpr1 (#11877)
071bcf7 update NOTICES file (#11892)
a1705d6 participant-state - Add an implicit logging context to the write service [kvl-1072] (#11838)
9ff64f7 Change daml script’s sleep to sleep for a minimum amount of time (#11886)
132c277 Add a Canton sandbox to the SDK (#11881)
68a2343 Only run self-service compat job on PRs (#11893)
c27406c [DPP-762][Self-service error codes] Automate generation of inventory of error categories. #11879
1379722 Adapt the compatibility exclusions (#11872)
d66ecc9 LF: Drop Archive Snapshot for LF < 1.14 (#11820)
abc141b Increase pruning tests timeout (#11891)
66b4074 Update protobuf docs plugin (#11880)
b0dda53 LF check stable proto with buf and md5sum. (#11888)
056fc52 Log while processing base64 encoded server key [DPP-761] (#11835)
dbda67b bump JVM in Docker image (#11883)
f69bd68 ledger-api-bench-tool: Fix flaky `MetricsCollectorSpec` (#11750)
cb758e8 Fix call to experimental interface signatory builtin (#11882)
024400b Error when fetching the wrong template id (via fetch by interface). (#11862)
0852c8f Make DA.List.Total return Optional instead (#11878)
df37346 [JSON-API] Add query store metrics (#11809)
2f8f69e Drop DA.Next.Set and DA.Next.Map (#11864)
5f3a4d2 [Self-service error codes] Fix section numbering in pdf for error codes section by moving it a level higher. (#11867)
cf3ac01 [Self-service error codes] Do not return error code id and definite_answer in metadata for security sensitive errors (#11828)
026b92a Add gRPC definitions for participant user management service (#11818)
2fde30d Disable writing volatile bits in Scala statsfile (#11875)
4ed9ded Remove xxd from dev-env (#11876)
eaded41 remove mergify (#11866)
3cd5028 fix a few more things in the daml-lf spec (#11851)
beca0ee Refactor StandaloneApiServer factory (#11842)
6356f13 Properly upgrade gRPC to 1.41.0 (#11858)
f6accd3 Release 1.18 RC2 (#11869)
d858873 fix main (#11868)
da8dd7e rotate release duty after 1.18.0-snapshot.20211123.8463.0.bd2a6852 (#11845)
066da4f [Self-service error codes] Small fixes for docs/scripts/live-preview.sh (#11856)
258fb65 Document how to deal with HTTP JSON API schema changes (#11336)
b8937ad ci: self-service compat test start (#11853)
de8d15f fix Nix install on macOS nodes (#11696)
b3d1d40 Expose submissionId via the Java bindings (#11839) (#11847)
86da6e8 LF: Test scala interface type checking (#11833)
5f52f00 increase linux cluster size (#11860)
5c12d75 Add a guard when exercising by interface. (#11836)
7c3a2a7 Add a new KV submission failure error (#11854)
aebc5a7 All packages must be valid (#11850)
0374843 speedy compilation benchmark (#11852)
393893a LF encoder: make package validation optional (#11849)
25b476f DPP-726 Add string interning unit tests (#11841)
59eb0d2 kvutils - For duplicate command rejections, add the submission id as metadata [KVL-1175] (#11848)
970243d Ensure stack-safety during closure-conversion. (#11778)
e63c80d update LATEST (#11846)
db42521 libs-scala: Change `SourceQueueResourceOwner` to `BoundedSourceQueueResourceOwner` [KVL-1177] (#11832)
109b606 Make the `InstrumentedSource.queue` use the `BoundedSourceQueue` [KVL-1177] (#11807)
```
Changelog:
```

- [Daml Compiler] The supported input LF versions for
  data-dependencies are now limited to LF 1.8 and newer.

- [Daml2js] DARs with LF version < 1.8 are no longer supported.

- [Integration Kit] kvutils protos no longer depend on the Daml-LF transaction proto

- [Daml Standard Library] DA.List.Total functions now return Optional
  instead of being polymorphic in the return type. DA.Optional.Total
  has been removed.

- [JSON-API] added metrics to separately track:
    - time taken to update query-store ACS (from ledger)
    - lookup times for the query store

- [Daml Standard Library] DA.Next.Map and DA.Next.Set have been removed
  after being deprecated since Daml-LF 1.11

- [Ledger API] Introduce gRPC definitions for experimental user
  managament service to manage users and their rights for interacting
  with the Ledger API served by a participant node.

[HTTP JSON API] [Docs] Document lack of data continuity guarantees and how to deal with schema changes
[Java Bindings] submissionId is now exposed via the bindings, see issue #11705
[Integration Kit] Add a new SUBMISSION_FAILED internal error
kvutils - For duplicate command rejections, the submission id of the already accepted transaction is returning as part of the gRPC metadata. The submission id will be included under the key `existing_submission_id`.

- [Integration Kit] `SourceQueueResourceOwner` has been renamed to `BoundedSourceQueueResourceOwner` and takes a `BoundedSourceQueue` from now on

- [Integration Kit] InstrumentedSource.queue.offer no longer returns a Future

```

CHANGELOG_BEGIN
CHANGELOG_END
cocreature added a commit that referenced this pull request Dec 1, 2021
* release 2.0.0-snapshot.20211130.8536.0.683ab871

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.

@SamirTalwar-DA is in charge of this release.

Commit log:
```
683ab87 Move ghc-lib{,-parser} to bazel-haskell-deps (#11775)
9350632 Fix releasing of resources in case connection initialization failed (#11915)
e1559af Update `ModelConformanceValidator` comments and prevent them from getting outdated easily (#11924)
16a41f7 Avoid package validation in speedy compilation benchmark. (#11927)
16135e6 Limit supported input versions in damlc to >= LF 1.8 (#11905)
0ee4154 Use Absolute-indexes as keys for the Env-mapping during closure-conversion (#11912)
1d7bca8 Add optional typerep argument in UExerciseInterface. (#11910)
c2c22f8 kvutils: Protos no longer depend on the Daml-LF transaction proto [KVL-1166] (#11909)
5641948 [Docs] Add labels to error codes to support references to them (#11913)
0e77676 Update protobuf docs template to handle oneOf (#11887)
5a9481f unify heading markup according to README.md (#11919)
61334cf kvutils - Add Writer which can handle deduplication periods as offsets [KVL-1172] (#11900)
0b9d57b Add ContractDoesntImplementInterface error. (#11884)
49e5d41 align index.rst files for HTML and PDF (#11907)
dbbb05f Split daml-lf encode/decode Haskell libraries (#11906)
e5d3902 iface: support for fixed choices in TS codegen (#11630)
31cc540 Turn package name & version warnings into an error (#11859)
4e50060 self-service compat: set branch name to not main (#11902)
2f4aa47 refactor to avoid impossible code path (#11901)
a81995c switch dev images to Temurin (#11895)
f3a0e2e Set scalafmt dialect explicitly (#11898)
60e372d Don't run pruning tests on H2, they time-out (#11897)
58e69ad LF: replace "dev" LF version by "1.dev" in bazel files (#11894)
8ef348d Use absolute stack locations in SExpr1 (#11877)
071bcf7 update NOTICES file (#11892)
a1705d6 participant-state - Add an implicit logging context to the write service [kvl-1072] (#11838)
9ff64f7 Change daml script’s sleep to sleep for a minimum amount of time (#11886)
132c277 Add a Canton sandbox to the SDK (#11881)
68a2343 Only run self-service compat job on PRs (#11893)
c27406c [DPP-762][Self-service error codes] Automate generation of inventory of error categories. #11879
1379722 Adapt the compatibility exclusions (#11872)
d66ecc9 LF: Drop Archive Snapshot for LF < 1.14 (#11820)
abc141b Increase pruning tests timeout (#11891)
66b4074 Update protobuf docs plugin (#11880)
b0dda53 LF check stable proto with buf and md5sum. (#11888)
056fc52 Log while processing base64 encoded server key [DPP-761] (#11835)
dbda67b bump JVM in Docker image (#11883)
f69bd68 ledger-api-bench-tool: Fix flaky `MetricsCollectorSpec` (#11750)
cb758e8 Fix call to experimental interface signatory builtin (#11882)
024400b Error when fetching the wrong template id (via fetch by interface). (#11862)
0852c8f Make DA.List.Total return Optional instead (#11878)
df37346 [JSON-API] Add query store metrics (#11809)
2f8f69e Drop DA.Next.Set and DA.Next.Map (#11864)
5f3a4d2 [Self-service error codes] Fix section numbering in pdf for error codes section by moving it a level higher. (#11867)
cf3ac01 [Self-service error codes] Do not return error code id and definite_answer in metadata for security sensitive errors (#11828)
026b92a Add gRPC definitions for participant user management service (#11818)
2fde30d Disable writing volatile bits in Scala statsfile (#11875)
4ed9ded Remove xxd from dev-env (#11876)
eaded41 remove mergify (#11866)
3cd5028 fix a few more things in the daml-lf spec (#11851)
beca0ee Refactor StandaloneApiServer factory (#11842)
6356f13 Properly upgrade gRPC to 1.41.0 (#11858)
f6accd3 Release 1.18 RC2 (#11869)
d858873 fix main (#11868)
da8dd7e rotate release duty after 1.18.0-snapshot.20211123.8463.0.bd2a6852 (#11845)
066da4f [Self-service error codes] Small fixes for docs/scripts/live-preview.sh (#11856)
258fb65 Document how to deal with HTTP JSON API schema changes (#11336)
b8937ad ci: self-service compat test start (#11853)
de8d15f fix Nix install on macOS nodes (#11696)
b3d1d40 Expose submissionId via the Java bindings (#11839) (#11847)
86da6e8 LF: Test scala interface type checking (#11833)
5f52f00 increase linux cluster size (#11860)
5c12d75 Add a guard when exercising by interface. (#11836)
7c3a2a7 Add a new KV submission failure error (#11854)
aebc5a7 All packages must be valid (#11850)
0374843 speedy compilation benchmark (#11852)
393893a LF encoder: make package validation optional (#11849)
25b476f DPP-726 Add string interning unit tests (#11841)
59eb0d2 kvutils - For duplicate command rejections, add the submission id as metadata [KVL-1175] (#11848)
970243d Ensure stack-safety during closure-conversion. (#11778)
e63c80d update LATEST (#11846)
db42521 libs-scala: Change `SourceQueueResourceOwner` to `BoundedSourceQueueResourceOwner` [KVL-1177] (#11832)
109b606 Make the `InstrumentedSource.queue` use the `BoundedSourceQueue` [KVL-1177] (#11807)
```
Changelog:
```

- [Daml Compiler] The supported input LF versions for
  data-dependencies are now limited to LF 1.8 and newer.

- [Daml2js] DARs with LF version < 1.8 are no longer supported.

- [Integration Kit] kvutils protos no longer depend on the Daml-LF transaction proto

- [Daml Standard Library] DA.List.Total functions now return Optional
  instead of being polymorphic in the return type. DA.Optional.Total
  has been removed.

- [JSON-API] added metrics to separately track:
    - time taken to update query-store ACS (from ledger)
    - lookup times for the query store

- [Daml Standard Library] DA.Next.Map and DA.Next.Set have been removed
  after being deprecated since Daml-LF 1.11

- [Ledger API] Introduce gRPC definitions for experimental user
  managament service to manage users and their rights for interacting
  with the Ledger API served by a participant node.

[HTTP JSON API] [Docs] Document lack of data continuity guarantees and how to deal with schema changes
[Java Bindings] submissionId is now exposed via the bindings, see issue #11705
[Integration Kit] Add a new SUBMISSION_FAILED internal error
kvutils - For duplicate command rejections, the submission id of the already accepted transaction is returning as part of the gRPC metadata. The submission id will be included under the key `existing_submission_id`.

- [Integration Kit] `SourceQueueResourceOwner` has been renamed to `BoundedSourceQueueResourceOwner` and takes a `BoundedSourceQueue` from now on

- [Integration Kit] InstrumentedSource.queue.offer no longer returns a Future

```

CHANGELOG_BEGIN
CHANGELOG_END

* bump to include fix for damlc package validation

changelog_begin
changelog_end

Co-authored-by: Azure Pipelines Daml Build <[email protected]>
Co-authored-by: Moritz Kiefer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants