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

feat: support conditional policies #110

Merged
merged 36 commits into from Feb 27, 2020
Merged

Conversation

frankyn
Copy link
Member

@frankyn frankyn commented Dec 20, 2019

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

PTAL: @broady @jkwlui @JesseLovelace @chingor13

@googlebot googlebot added the cla: yes label Dec 20, 2019
@chingor13 chingor13 changed the title Support conditional policies feat: support conditional policies Jan 6, 2020
@frankyn frankyn changed the title feat: support conditional policies fix: support conditional policies Jan 7, 2020
@frankyn frankyn changed the title fix: support conditional policies feat: support conditional policies Jan 7, 2020
@codecov
Copy link

@codecov codecov bot commented Jan 7, 2020

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Member Author

@frankyn frankyn commented Jan 9, 2020

Hi @chingor13 @JesseLovelace @jkwlui @broady,

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

Copy link
Member

@jkwlui jkwlui left a comment

a few question, but looks good in general!

private Condition condition;

public static class Builder {
private List<String> members = new ArrayList();
Copy link
Member

@jkwlui jkwlui Jan 9, 2020

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@frankyn frankyn Jan 13, 2020

Choose a reason for hiding this comment

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

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

@@ -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>();
Copy link
Member

@jkwlui jkwlui Jan 9, 2020

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@frankyn frankyn Jan 13, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@chingor13 chingor13 Jan 28, 2020

Choose a reason for hiding this comment

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

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

setCondition(binding.condition);
}

public final Binding.Builder setRole(String role) {
Copy link
Member

@jkwlui jkwlui Jan 9, 2020

Choose a reason for hiding this comment

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

is there a reason to use String instead of Role?

Copy link
Member Author

@frankyn frankyn Jan 13, 2020

Choose a reason for hiding this comment

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

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

private List<String> members;
private Condition condition;

public static class Builder {
Copy link
Collaborator

@chingor13 chingor13 Jan 10, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@frankyn frankyn Jan 13, 2020

Choose a reason for hiding this comment

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

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.

@frankyn frankyn requested review from jkwlui and chingor13 Jan 23, 2020
@jkwlui
Copy link
Member

@jkwlui jkwlui commented Jan 24, 2020

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

@frankyn
Copy link
Member Author

@frankyn frankyn commented Jan 28, 2020

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

import javax.annotation.Nullable;

@AutoValue
abstract class Condition {
Copy link
Collaborator

@chingor13 chingor13 Jan 28, 2020

Choose a reason for hiding this comment

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

This should probably be public?

import javax.annotation.Nullable;

@AutoValue
abstract class Binding {
Copy link
Collaborator

@chingor13 chingor13 Jan 28, 2020

Choose a reason for hiding this comment

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

This should probably be public?

abstract class Binding {
abstract String getRole();

abstract ImmutableList<String> getMembers();
Copy link
Collaborator

@chingor13 chingor13 Jan 28, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@frankyn frankyn Feb 21, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@frankyn frankyn Feb 21, 2020

Choose a reason for hiding this comment

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

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

@@ -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>();
Copy link
Collaborator

@chingor13 chingor13 Jan 28, 2020

Choose a reason for hiding this comment

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

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


package com.google.cloud;

import static org.junit.Assert.*;
Copy link
Collaborator

@chingor13 chingor13 Jan 28, 2020

Choose a reason for hiding this comment

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

nit: avoid * imports

* 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) {
Copy link
Collaborator

@chingor13 chingor13 Jan 28, 2020

Choose a reason for hiding this comment

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

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
jkwlui approved these changes Jan 30, 2020
Copy link
Member

@jkwlui jkwlui left a comment

LGTM

@frankyn frankyn requested a review from elharo Feb 25, 2020
@kolea2 kolea2 dismissed their stale review Feb 25, 2020

comments addressed, lgtm

@codecov
Copy link

@codecov codecov bot commented Feb 25, 2020

Codecov Report

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

Impacted file tree graph

@@            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
Copy link

@codecov codecov bot commented Feb 25, 2020

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Member Author

@frankyn frankyn commented Feb 25, 2020

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
Copy link
Member Author

@frankyn frankyn commented Feb 26, 2020

cc: @netdpb (David)

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

@kolea2 kolea2 Feb 26, 2020

Choose a reason for hiding this comment

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

sorry, missed this. Should this be returning something?

Copy link
Member Author

@frankyn frankyn Feb 26, 2020

Choose a reason for hiding this comment

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

oh, I missed this as well. Will fix.

Copy link

@netdpb netdpb left a comment

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).


public abstract ImmutableList<String> getMembers();

@Nullable
Copy link

@netdpb netdpb Feb 26, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@frankyn frankyn Feb 26, 2020

Choose a reason for hiding this comment

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

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.

Copy link

@netdpb netdpb Feb 27, 2020

Choose a reason for hiding this comment

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

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

public abstract static class Builder {
public abstract Builder setRole(String role);

public abstract Builder setMembers(List<String> members);
Copy link

@netdpb netdpb Feb 26, 2020

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@frankyn frankyn Feb 26, 2020

Choose a reason for hiding this comment

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

Thanks, addressing.


public abstract String getRole();

public abstract ImmutableList<String> getMembers();
Copy link

@netdpb netdpb Feb 26, 2020

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@frankyn frankyn Feb 26, 2020

Choose a reason for hiding this comment

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

No, good point.


public abstract Builder setCondition(Condition condition);

public abstract String getRole();
Copy link

@netdpb netdpb Feb 26, 2020

Choose a reason for hiding this comment

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

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

Same for getCondition().

Copy link
Member Author

@frankyn frankyn Feb 26, 2020

Choose a reason for hiding this comment

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

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.

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

@netdpb netdpb Feb 26, 2020

Choose a reason for hiding this comment

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

Why are these nullable?

Copy link
Member Author

@frankyn frankyn Feb 26, 2020

Choose a reason for hiding this comment

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

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

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();
Copy link

@netdpb netdpb Feb 26, 2020

Choose a reason for hiding this comment

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

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().

}

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

@netdpb netdpb Feb 26, 2020

Choose a reason for hiding this comment

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

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

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

@netdpb netdpb Feb 26, 2020

Choose a reason for hiding this comment

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

This whole block can be just

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

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());
Copy link

@netdpb netdpb Feb 26, 2020

Choose a reason for hiding this comment

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

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.

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

@netdpb netdpb Feb 26, 2020

Choose a reason for hiding this comment

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

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)?

@frankyn frankyn requested a review from kolea2 Feb 26, 2020
kolea2
kolea2 approved these changes Feb 27, 2020
Copy link
Collaborator

@chingor13 chingor13 left a comment

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
Copy link
Member Author

@frankyn frankyn commented Feb 27, 2020

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

@frankyn frankyn merged commit 61e2d19 into googleapis:master Feb 27, 2020
15 checks passed
@frankyn frankyn deleted the conditional-policy branch Feb 27, 2020

public abstract ImmutableList<String> getMembers();

@Nullable
Copy link

@netdpb netdpb Feb 27, 2020

Choose a reason for hiding this comment

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

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

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

@netdpb netdpb Feb 27, 2020

Choose a reason for hiding this comment

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

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

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

@netdpb netdpb Feb 27, 2020

Choose a reason for hiding this comment

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

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

*
* @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(
Copy link

@netdpb netdpb Feb 27, 2020

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants