Skip to content

Commit

Permalink
Update IAM helper methods to protect against IAM conditions
Browse files Browse the repository at this point in the history
The methods throw an exception if:

- The policy version is greater than 1
- Any bindings have conditions

The exception message will need to be changed to include a link with more information.
  • Loading branch information
jskeet committed Nov 4, 2019
1 parent af47eb8 commit f8f09a8
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 0 deletions.
38 changes: 38 additions & 0 deletions apis/Google.Cloud.Iam.V1/Google.Cloud.Iam.V1.Tests/PolicyTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

using System;
using Xunit;

namespace Google.Cloud.Iam.V1.Tests
Expand Down Expand Up @@ -154,5 +155,42 @@ public void RemoveRoleMember_NoRole()
Assert.False(policy.RemoveRoleMember("target", "d"));
Assert.Equal(expected, policy);
}

[Fact]
public void AddRoleMember_LaterVersion()
{
var policy = new Policy { Version = 2 };
Assert.Throws<InvalidOperationException>(() => policy.AddRoleMember("role", "member"));
}

[Fact]
public void RemoveRoleMember_LaterVersion()
{
var policy = new Policy { Version = 2 };
Assert.Throws<InvalidOperationException>(() => policy.RemoveRoleMember("role", "member"));
}

[Fact]
public void AddRoleMember_ContainsBinding()
{
var policy = new Policy
{
Bindings = { new Binding { Role = "role", Members = { "member" }, Condition = new Type.Expr { Description = "condition" } } }
};
Assert.Throws<InvalidOperationException>(() => policy.AddRoleMember("role", "member2"));
Assert.Throws<InvalidOperationException>(() => policy.AddRoleMember("role2", "member2"));
}

[Fact]
public void RemoveRoleMember_ContainsBinding()
{
var policy = new Policy
{
Bindings = { new Binding { Role = "role", Members = { "member" }, Condition = new Type.Expr { Description = "condition" } } }
};
Assert.Throws<InvalidOperationException>(() => policy.RemoveRoleMember("role", "member"));
Assert.Throws<InvalidOperationException>(() => policy.RemoveRoleMember("role", "member2"));
Assert.Throws<InvalidOperationException>(() => policy.RemoveRoleMember("role2", "member"));
}
}
}
18 changes: 18 additions & 0 deletions apis/Google.Cloud.Iam.V1/Google.Cloud.Iam.V1/PolicyPartial.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,21 @@ public partial class Policy
/// <summary>
/// Adds the specified member to the specified role. If the role does
/// not already exist, it is created.
/// This method will fail with an <see cref="InvalidOperationException"/>
/// if it is called on a Policy with a <see cref="Version"/> greater than 1,
/// or if any of the bindings contain conditions,
/// as that indicates a more complicated policy than this method is prepared
/// to handle. Changes to such policies must be made manually.
/// </summary>
/// <param name="role">The role to add the member to. Must not be null or empty.</param>
/// <param name="member">The member to add to the role. Must not be null or empty.</param>
/// <returns><c>true</c> if the policy was changed; <c>false</c> if the member already existed in the role.</returns>
/// <exception cref="InvalidOperationException">The <see cref="Version"/> is greater than 1.</exception>
public bool AddRoleMember(string role, string member)
{
GaxPreconditions.CheckNotNullOrEmpty(role, nameof(role));
GaxPreconditions.CheckNotNullOrEmpty(member, nameof(member));
ValidatePolicyVersion();
var binding = FindRole(role);
if (binding == null)
{
Expand All @@ -51,15 +58,22 @@ public bool AddRoleMember(string role, string member)
/// <summary>
/// Removes the specified member to the specified role, if they belong to it. If the role becomes empty after
/// removing the member, it is removed from the policy.
/// This method will fail with an <see cref="InvalidOperationException"/>
/// if it is called on a Policy with a <see cref="Version"/> greater than 1,
/// or if any of the bindings contain conditions,
/// as that indicates a more complicated policy than this method is prepared
/// to handle. Changes to such policies must be made manually.
/// </summary>
/// <param name="role">The role to remove the member from. Must not be null or empty.</param>
/// <param name="member">The member to remove from the role. Must not be null or empty.</param>
/// <returns><c>true</c> if the policy was changed; <c>false</c> if the member didn't exist in the role
/// or the role didn't exist.</returns>
/// <exception cref="InvalidOperationException">The <see cref="Version"/> is greater than 1.</exception>
public bool RemoveRoleMember(string role, string member)
{
GaxPreconditions.CheckNotNullOrEmpty(role, nameof(role));
GaxPreconditions.CheckNotNullOrEmpty(member, nameof(member));
ValidatePolicyVersion();
var binding = FindRole(role);
if (binding == null)
{
Expand All @@ -77,5 +91,9 @@ public bool RemoveRoleMember(string role, string member)
}

private Binding FindRole(string role) => Bindings.FirstOrDefault(binding => binding.Role == role);

private void ValidatePolicyVersion() =>
GaxPreconditions.CheckState(Version <= 1 && Bindings.All(b => b.Condition == null),
"Helper methods cannot be invoked on policies with version {0} or with conditional bindings. For more information, see https://cloud.google.com/iam/docs/policies.", Version);
}
}

0 comments on commit f8f09a8

Please sign in to comment.