feat: support conditional policies #110

frankyn · 2019-12-20T17:56:40Z

fixes: #118
Add support for conditional policies based on design document.

PTAL: @broady @jkwlui @JesseLovelace @chingor13

codecov · 2020-01-07T18:38:23Z

Codecov Report

❗ No coverage uploaded for pull request base (master@d2fab21). Click here to learn what that means.
The diff coverage is 94.59%.

@@            Coverage Diff            @@
##             master     #110   +/-   ##
=========================================
  Coverage          ?   67.98%           
  Complexity        ?      377           
=========================================
  Files             ?       36           
  Lines             ?     1968           
  Branches          ?      258           
=========================================
  Hits              ?     1338           
  Misses            ?      526           
  Partials          ?      104

Impacted Files	Coverage Δ	Complexity Δ
...core/src/main/java/com/google/cloud/Condition.java	`100% <ø> (ø)`	`2 <0> (?)`
...d-core/src/main/java/com/google/cloud/Binding.java	`100% <100%> (ø)`	`2 <0> (?)`
...ud-core/src/main/java/com/google/cloud/Policy.java	`94.47% <93.1%> (ø)`	`20 <0> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2fab21...505f9bc. Read the comment docs.

frankyn · 2020-01-09T00:07:31Z

Hi @chingor13 @JesseLovelace @jkwlui @broady,

I resolved presubmit checks, please review when you have a moment. Thank you!

jkwlui

a few question, but looks good in general!

jkwlui · 2020-01-08T19:24:07Z

google-cloud-core/src/main/java/com/google/cloud/Binding.java

+  private Condition condition;
+
+  public static class Builder {
+    private List<String> members = new ArrayList();


Did we end up deciding to use a List instead of a Set for the list of members?

I updated it to List<> instead of Set<> to get the conversation moving. I can revert back to Set<>.

jkwlui · 2020-01-09T21:18:57Z

google-cloud-core/src/main/java/com/google/cloud/Policy.java

@@ -85,22 +96,25 @@ public String apply(Identity identity) {

    @Override
    protected Policy fromPb(com.google.iam.v1.Policy policyPb) {
-      Map<Role, Set<Identity>> bindings = new HashMap<>();
+      List<Binding> bindingsList = new ArrayList<Binding>();


Do we want to allow users to modify the binding in-place?

I'd need to allow mutations outside of builders. That's okay but it does allow for mutation directly of the policy when it wasn't allowed beforehand.

This could use ImmutableList.Builder if we want to keep it immutable

jkwlui · 2020-01-09T21:23:18Z

google-cloud-core/src/main/java/com/google/cloud/Binding.java

+      setCondition(binding.condition);
+    }
+
+    public final Binding.Builder setRole(String role) {


is there a reason to use String instead of Role?

I'm trying to remove helpers from the new usage patterns. I could add them back in though.

chingor13 · 2020-01-09T18:12:27Z

google-cloud-core/src/main/java/com/google/cloud/Binding.java

+  private List<String> members;
+  private Condition condition;
+
+  public static class Builder {


Should we be using auto-value for these classes and builders if they are meant to be value classes?

It might be tricky if we're trying to enforce that the list of members is immutable on the actual value, but it's probably worthwhile to have these classes fully immutable.

good call I didn't think about using auto-value and I think you're right in that it can be helpful to be fully immutable reducing the number of builders.

Please correct me but I don't think I should update the Policy class but only Binding and Condition to use Auto-Value.

jkwlui · 2020-01-24T23:04:01Z

I notice there are no documentation on the fields/getter/setter for auto-value classes. @chingor13 how do we usually document these?

frankyn · 2020-01-28T17:56:55Z

I can add documentation to autovalue implementation after I get a review of the surface.

chingor13 · 2020-01-28T19:15:41Z

google-cloud-core/src/main/java/com/google/cloud/Condition.java

+import javax.annotation.Nullable;
+
+@AutoValue
+abstract class Condition {


This should probably be public?

chingor13 · 2020-01-28T19:16:24Z

google-cloud-core/src/main/java/com/google/cloud/Binding.java

+import javax.annotation.Nullable;
+
+@AutoValue
+abstract class Binding {


This should probably be public?

chingor13 · 2020-01-28T19:17:51Z

google-cloud-core/src/main/java/com/google/cloud/Binding.java

+abstract class Binding {
+  abstract String getRole();
+
+  abstract ImmutableList<String> getMembers();


Assuming this class is public, we want to keep guava types off the surface of the classes. This should be a List<String> and we'd document that the implementation is immutable so they shouldn't try to modify it.

I missed this comment a while back. The reason I'm using ImmutableList<> is based on recommendation from AutoValue docs https://github.com/google/auto/blob/master/value/userguide/builders-howto.md#-use-a-collection-valued-property. I want getMembers() to support a builder helper as well in case it's easier for users.

WDYT?

Removed ImmutableList and now using List in the surface while still supporting helper methods addMembers and removeMembers.

chingor13 · 2020-01-28T19:21:07Z

google-cloud-core/src/main/java/com/google/cloud/Policy.java

@@ -85,22 +96,25 @@ public String apply(Identity identity) {

    @Override
    protected Policy fromPb(com.google.iam.v1.Policy policyPb) {
-      Map<Role, Set<Identity>> bindings = new HashMap<>();
+      List<Binding> bindingsList = new ArrayList<Binding>();


This could use ImmutableList.Builder if we want to keep it immutable

google-cloud-core/src/main/java/com/google/cloud/Policy.java

chingor13 · 2020-01-28T19:24:05Z

google-cloud-core/src/test/java/com/google/cloud/PolicyV3Test.java

+
+package com.google.cloud;
+
+import static org.junit.Assert.*;


nit: avoid * imports

chingor13 · 2020-01-28T19:25:13Z

google-cloud-core/src/main/java/com/google/cloud/Policy.java

+   * Return false if Policy is version 3 OR bindings has a conditional binding.
+   * Return true if Policy is version 1 AND bindings does not have a conditional binding.
+   */
+  private static boolean checkVersion(int version, List<Binding> bindingsList) {


I might call this isVersion. The check* methods in guava throw exceptions when conditions are not met, whereas this method is returning a boolean

jkwlui

LGTM

comments addressed, lgtm

codecov · 2020-02-25T17:27:45Z

Codecov Report

❗ No coverage uploaded for pull request base (master@d2fab21). Click here to learn what that means.
The diff coverage is 94.59%.

@@            Coverage Diff            @@
##             master     #110   +/-   ##
=========================================
  Coverage          ?   67.98%           
  Complexity        ?      377           
=========================================
  Files             ?       36           
  Lines             ?     1968           
  Branches          ?      258           
=========================================
  Hits              ?     1338           
  Misses            ?      526           
  Partials          ?      104

Impacted Files	Coverage Δ	Complexity Δ
...core/src/main/java/com/google/cloud/Condition.java	`100% <ø> (ø)`	`2 <0> (?)`
...d-core/src/main/java/com/google/cloud/Binding.java	`100% <100%> (ø)`	`2 <0> (?)`
...ud-core/src/main/java/com/google/cloud/Policy.java	`94.47% <93.1%> (ø)`	`20 <0> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2fab21...505f9bc. Read the comment docs.

codecov · 2020-02-25T17:28:46Z

Codecov Report

Merging #110 into master will not change coverage.
The diff coverage is 83.33%.

@@            Coverage Diff            @@
##             master     #110   +/-   ##
=========================================
  Coverage     67.94%   67.94%           
  Complexity      377      377           
=========================================
  Files            36       36           
  Lines          1959     1959           
  Branches        254      254           
=========================================
  Hits           1331     1331           
  Misses          524      524           
  Partials        104      104

Impacted Files	Coverage Δ	Complexity Δ
...ud-core/src/main/java/com/google/cloud/Policy.java	`94.84% <0%> (ø)`	`19 <0> (ø)`	⬇️
...core/src/main/java/com/google/cloud/Timestamp.java	`90.62% <100%> (ø)`	`25 <2> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f48a15...2b56641. Read the comment docs.

frankyn · 2020-02-25T23:24:44Z

Updated based on discussion with @chingor13, @kolea2, @elharo, @jkwlui, and David (not sure of their Github handle).

Please take a look when you have a moment. Thank you for your help.

frankyn · 2020-02-26T15:23:17Z

cc: @netdpb (David)

kolea2 · 2020-02-26T17:20:15Z

google-cloud-core/src/main/java/com/google/cloud/Policy.java

+      return false;
+    }
+    for (int i = 0; i < bindingsList.size(); i++) {
+      bindingsList.get(i).equals(other.getBindingsList().get(i));


sorry, missed this. Should this be returning something?

oh, I missed this as well. Will fix.

netdpb

I haven't finished reviewing the test code yet, but I have to run.

Feel free not to address all (or any!) of these issues in this PR if they require further discussion.

Some are pure style nits, which you should feel free to ignore anyway (like deleting the final period at the end of a Javadoc @throws clause).

netdpb · 2020-02-26T18:33:04Z

google-cloud-core/src/main/java/com/google/cloud/Binding.java

+
+  public abstract ImmutableList<String> getMembers();
+
+  @Nullable


Why @Nullable instead of a non-nullable Optional<Condition>?

Either is okay, but it's interesting that you're using both patterns in the same class.

I picked Nullable because it made more sense to me in this case. I'm not well versed in Optional's. Both patterns? Not sure I understand.

Sorry, I thought for some reason that you were also using Optional. Don't know why I thought that.

netdpb · 2020-02-26T18:33:38Z

google-cloud-core/src/main/java/com/google/cloud/Binding.java

+  public abstract static class Builder {
+    public abstract Builder setRole(String role);
+
+    public abstract Builder setMembers(List<String> members);


This could even take Iterable<String> if you want to be most general.

Thanks, addressing.

netdpb · 2020-02-26T18:39:33Z

google-cloud-core/src/main/java/com/google/cloud/Binding.java

+
+    public abstract String getRole();
+
+    public abstract ImmutableList<String> getMembers();


Do you expect this to be called other than from addMembers(String...) and removeMembers(String...)? If not, make this have default access.

No, good point.

netdpb · 2020-02-26T18:40:22Z

google-cloud-core/src/main/java/com/google/cloud/Binding.java

+
+    public abstract Builder setCondition(Condition condition);
+
+    public abstract String getRole();


Is this used from anywhere? Do you need it? Generally, builders don't need getters.

Same for getCondition().

removed to reduce surface size. At the moment they were convenience but I'd like to get more developer input before adding unnecessary methods to the public surface.

netdpb · 2020-02-26T18:41:05Z

google-cloud-core/src/main/java/com/google/cloud/Condition.java

+@BetaApi("This is a Beta API is not stable yet and may change in the future.")
+@AutoValue
+public abstract class Condition {
+  @Nullable


Why are these nullable?

Good point, no reason other than my first attempt with AutoValue. removing.

netdpb · 2020-02-26T19:18:18Z

google-cloud-core/src/main/java/com/google/cloud/Policy.java

+    checkArgument(
+        !isConditional(this.version, this.bindingsList),
+        "getBindings() is only supported with version 1 policies and non-conditional policies");
+    ImmutableMap.Builder<Role, Set<Identity>> bindingsV1Builder = ImmutableMap.builder();


You can use an ImmutableSetMultimap instead, regardless of whether you change the method return type. You can always get the map out with Multimaps.asMap().

netdpb · 2020-02-26T19:18:44Z

google-cloud-core/src/main/java/com/google/cloud/Policy.java

+  }
+
+  /** Returns the list of bindings that comprises the policy for version 3. */
+  public List<Binding> getBindingsList() {


Suggested change

public List<Binding> getBindingsList() {

public ImmutableList<Binding> getBindingsList() {

netdpb · 2020-02-26T19:20:23Z

google-cloud-core/src/main/java/com/google/cloud/Policy.java

-    return Objects.equals(bindings, other.getBindings())
-        && Objects.equals(etag, other.getEtag())
-        && Objects.equals(version, other.getVersion());
+    if (bindingsList.size() != other.getBindingsList().size()) {


This whole block can be just

if (!bindingsList.equals(other.getBindingsList())) { return false; }

netdpb · 2020-02-26T19:21:17Z

google-cloud-core/src/main/java/com/google/cloud/Policy.java

+    for (int i = 0; i < bindingsList.size(); i++) {
+      bindingsList.get(i).equals(other.getBindingsList().get(i));
+    }
+    return Objects.equals(etag, other.getEtag()) && Objects.equals(version, other.getVersion());


Can etag ever be null? If not, just call etag.equals(other.getEtag()).

version is an int, so you can just do version == other.getVersion(). Otherwise you're boxing the int.

netdpb · 2020-02-26T19:25:53Z

google-cloud-core/src/test/java/com/google/cloud/PolicyV3Test.java

+          Binding.newBuilder().setRole(EDITOR).setMembers(MEMBERS_LIST_2).build());
+  private static final List<Binding> BINDINGS_WITH_CONDITIONS =
+      ImmutableList.copyOf(BINDINGS_NO_CONDITIONS)
+          .of(


You're calling a static method on an instance, and throwing the instance away. That's not what you want to do. (If the project used Error-Prone, this would have generated a warning.)

It looks like you want a variant of BINDINGS_NO_CONDITIONS where the viewer role has a condition. What about this?

ImmutableList.of( BINDINGS_NO_CONDITIONS.get(0).toBuilder().setCondition(...).build(), BINDINGS_NO_CONDITIONS.get(1))

Although it might be nice to have a constant for each element of BINDINGS_NO_CONDITIONS (VIEWER and EDITOR)?

chingor13

We should still clean up a bunch of the review comments, but they are minor and can be done separately. We can merge, but let's not lose the work items to clean up.

frankyn · 2020-02-27T18:33:43Z

I'll make a separate list and file an issue to track it. Thanks @chingor13!

netdpb · 2020-02-27T20:35:43Z

google-cloud-core/src/main/java/com/google/cloud/Binding.java

+
+  public abstract ImmutableList<String> getMembers();
+
+  @Nullable


Sorry, I thought for some reason that you were also using Optional. Don't know why I thought that.

netdpb · 2020-02-27T20:43:21Z

google-cloud-core/src/main/java/com/google/cloud/Binding.java

  public static Builder newBuilder() {
-    return new AutoValue_Binding.Builder();
+    List<String> emptyMembers = ImmutableList.of();


You could just inline emptyMembers into setMembers(ImmutableList.of()).

netdpb · 2020-02-27T20:44:21Z

google-cloud-core/src/main/java/com/google/cloud/Binding.java

+    /**
+     * Set IAM Members for Policy Binding
+     *
+     * @throws NullPointerException if a member is null.


You don't really need to repeat this everywhere. It's better to use @Nullable for the hopefully few cases where null is accepted.

netdpb · 2020-02-27T20:46:56Z

google-cloud-core/src/main/java/com/google/cloud/Policy.java

+     *
+     * @throws IllegalArgumentException if policy version is equal to 3 or has conditional bindings
+     *     because conditional policies are not supported
+     */
    public final Builder removeRole(Role role) {
      checkArgument(


I still think all of these should use checkState instead of checkArgument so they throw IllegalStateException.

Throw IllegalArgumentException when the caller should have known that the arguments to this method are invalid.
Throw IllegalStateException when the caller should have known that this method cannot be called given the state of the object.

frankyn added 3 commits December 17, 2019 12:15

Base implementation

67e52ce

Update with unit tests

84fcfea

lint

145f7a0

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 20, 2019

chingor13 changed the title ~~Support conditional policies~~ feat: support conditional policies Jan 6, 2020

frankyn changed the title ~~feat: support conditional policies~~ fix: support conditional policies Jan 7, 2020

frankyn changed the title ~~fix: support conditional policies~~ feat: support conditional policies Jan 7, 2020

correct copyright date

ac1fd6e

frankyn added 2 commits January 7, 2020 10:50

lint

2c8724b

Revert removal of helper functions

c90dbb0

jkwlui reviewed Jan 9, 2020

View reviewed changes

chingor13 reviewed Jan 10, 2020

View reviewed changes

frankyn added 6 commits January 21, 2020 18:30

use auto-value

b0a617f

Merge branch 'master' into conditional-policy

9dbfc2d

reformat Binding.java and Condition.java

54ad076

remove unnecessary dep

ff49620

code format

298b2be

add dep on com.google.code.findbugs in google-cloud-core

aaebba4

frankyn requested review from jkwlui and chingor13 January 23, 2020 17:19

chingor13 reviewed Jan 28, 2020

View reviewed changes

jkwlui approved these changes Jan 30, 2020

View reviewed changes

address comments

a5f63ea

frankyn mentioned this pull request Feb 6, 2020

feat: add IAM Conditions support googleapis/java-storage#120

Merged

2 tasks

frankyn and others added 2 commits February 10, 2020 20:49

Merge branch 'master' into conditional-policy

4873b73

Clean up

c853a84

frankyn added 2 commits February 24, 2020 20:33

address feedback

108faec

split up unit test

79126b5

frankyn requested a review from elharo February 25, 2020 05:42

use guava beta annotation

505f9bc

frankyn added 2 commits February 25, 2020 15:15

surface ImmutableList<> for Binding class.

a89ef0c

use BetaApi from api.core

f0b5085

kolea2 reviewed Feb 26, 2020

View reviewed changes

netdpb reviewed Feb 26, 2020

View reviewed changes

frankyn added 6 commits February 26, 2020 12:12

return as expected

9f5e600

partial addressing of feedback

8580f5a

address feedback pt2

9fe4358

address remaining feedback

8f48a15

Merge branch 'master' into conditional-policy

615ba06

address one last feedback

2b56641

frankyn requested a review from kolea2 February 26, 2020 21:10

kolea2 approved these changes Feb 27, 2020

View reviewed changes

chingor13 approved these changes Feb 27, 2020

View reviewed changes

frankyn merged commit 61e2d19 into googleapis:master Feb 27, 2020

frankyn deleted the conditional-policy branch February 27, 2020 18:34

frankyn mentioned this pull request Feb 27, 2020

Clean up tests for IAM Condition Support #169

Closed

netdpb reviewed Feb 27, 2020

View reviewed changes

chingor13 mentioned this pull request Mar 10, 2020

Policy equality fails for binding list order #173

Closed

release-please bot mentioned this pull request Jan 4, 2022

chore(main): release java-core 2.0.0 #653

Closed


		public abstract ImmutableList<String> getMembers();

		@Nullable


		public abstract String getRole();

		public abstract ImmutableList<String> getMembers();


		public abstract Builder setCondition(Condition condition);

		public abstract String getRole();

	public List<Binding> getBindingsList() {
	public ImmutableList<Binding> getBindingsList() {

feat: support conditional policies #110

feat: support conditional policies #110

Conversation

frankyn commented Dec 20, 2019 • edited

codecov bot commented Jan 7, 2020 • edited

Codecov Report

frankyn commented Jan 9, 2020

jkwlui 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

frankyn Jan 13, 2020 • edited

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

jkwlui commented Jan 24, 2020

frankyn commented Jan 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chingor13 Jan 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankyn Feb 21, 2020 • edited

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

jkwlui left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 25, 2020

Codecov Report

codecov bot commented Feb 25, 2020 • edited

Codecov Report

frankyn commented Feb 25, 2020

frankyn commented Feb 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netdpb 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

chingor13 left a comment

Choose a reason for hiding this comment

frankyn commented Feb 27, 2020

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

frankyn commented Dec 20, 2019 •

edited

codecov bot commented Jan 7, 2020 •

edited

frankyn Jan 13, 2020 •

edited

chingor13 Jan 28, 2020 •

edited

frankyn Feb 21, 2020 •

edited

codecov bot commented Feb 25, 2020 •

edited