Add builder-style setters and remove complex constructors on Approval

[#107504490] https://www.pivotaltracker.com/story/show/107504490 Signed-off-by: Jeremy Coffield <jcoffield@pivotal.io>
cloudfoundry · Nov 23, 2015 · 1a3047a · 1a3047a
1 parent cc4e085
commit 1a3047a
Show file tree

Hide file tree

Showing 16 changed files with 929 additions and 227 deletions.
diff --git a/...on/src/main/java/org/cloudfoundry/identity/uaa/oauth/UserManagedAuthzApprovalHandler.java b/...on/src/main/java/org/cloudfoundry/identity/uaa/oauth/UserManagedAuthzApprovalHandler.java
@@ -126,12 +126,22 @@ public boolean isApproved(AuthorizationRequest authorizationRequest, Authenticat
 
                 for (String requestedScope : requestedScopes) {
                     if (approvedScopes.contains(requestedScope)) {
-                        approvalStore.addApproval(new Approval(getUserId(userAuthentication), authorizationRequest
+                        Approval approval = new Approval()
-                                        .getClientId(), requestedScope, expiry, APPROVED));
+                            .setUserId(getUserId(userAuthentication))
+                            .setClientId(authorizationRequest.getClientId())
+                            .setScope(requestedScope)
+                            .setExpiresAt(expiry)
+                            .setStatus(APPROVED);
+                        approvalStore.addApproval(approval);
                     }
                     else {
-                        approvalStore.addApproval(new Approval(getUserId(userAuthentication), authorizationRequest
+                        Approval approval = new Approval()
-                                        .getClientId(), requestedScope, expiry, DENIED));
+                            .setUserId(getUserId(userAuthentication))
+                            .setClientId(authorizationRequest.getClientId())
+                            .setScope(requestedScope)
+                            .setExpiresAt(expiry)
+                            .setStatus(DENIED);
+                        approvalStore.addApproval(approval);
                     }
                 }
 
@@ -141,8 +151,13 @@ public boolean isApproved(AuthorizationRequest authorizationRequest, Authenticat
 
                 for (String requestedScope : requestedScopes) {
                     if (!autoApprovedScopes.contains(requestedScope)) {
-                        approvalStore.addApproval(new Approval(getUserId(userAuthentication), authorizationRequest
+                        Approval approval = new Approval()
-                                        .getClientId(), requestedScope, expiry, DENIED));
+                            .setUserId(getUserId(userAuthentication))
+                            .setClientId(authorizationRequest.getClientId())
+                            .setScope(requestedScope)
+                            .setExpiresAt(expiry)
+                            .setStatus(DENIED);
+                        approvalStore.addApproval(approval);
                     }
                 }
             }

diff --git a/...n/src/main/java/org/cloudfoundry/identity/uaa/oauth/approval/ApprovalsAdminEndpoints.java b/...n/src/main/java/org/cloudfoundry/identity/uaa/oauth/approval/ApprovalsAdminEndpoints.java
@@ -42,6 +42,7 @@
 import org.springframework.security.oauth2.provider.ClientDetailsService;
 import org.springframework.stereotype.Controller;
 import org.springframework.util.Assert;
+import org.springframework.util.StringUtils;
 import org.springframework.web.bind.annotation.*;
 import org.springframework.web.client.RestTemplate;
 import org.springframework.web.servlet.View;
@@ -153,14 +154,13 @@ public List<Approval> updateApprovals(@RequestBody Approval[] approvals) {
         logger.debug("Updating approvals for user: " + currentUserId);
         approvalStore.revokeApprovals(String.format(USER_FILTER_TEMPLATE, currentUserId));
         for (Approval approval : approvals) {
-            if (approval.getUserId() !=null &&  !isValidUser(approval.getUserId())) {
+            if (StringUtils.hasText(approval.getUserId()) &&  !isValidUser(approval.getUserId())) {
-                logger.warn(String.format("Error[2] %s attemting to update approvals for %s", currentUserId, approval.getUserId()));
+                logger.warn(String.format("Error[2] %s attempting to update approvals for %s", currentUserId, approval.getUserId()));
                 throw new UaaException("unauthorized_operation", "Cannot update approvals for another user. Set user_id to null to update for existing user.",
                                 HttpStatus.UNAUTHORIZED.value());
             } else {
                 approval.setUserId(currentUserId);
             }
-            approval.setLastUpdatedAt(new Date());
             approvalStore.addApproval(approval);
         }
         return approvalStore.getApprovals(String.format(USER_FILTER_TEMPLATE, currentUserId));
@@ -181,7 +181,6 @@ public List<Approval> updateClientApprovals(@PathVariable String clientId, @Requ
             } else {
                 approval.setUserId(currentUserId);
             }
-            approval.setLastUpdatedAt(new Date());
             approvalStore.addApproval(approval);
         }
         return approvalStore.getApprovals(String.format(USER_AND_CLIENT_FILTER_TEMPLATE, currentUserId, clientId));

diff --git a/common/src/main/java/org/cloudfoundry/identity/uaa/oauth/approval/JdbcApprovalStore.java b/common/src/main/java/org/cloudfoundry/identity/uaa/oauth/approval/JdbcApprovalStore.java
@@ -223,7 +223,14 @@ public Approval mapRow(ResultSet rs, int rowNum) throws SQLException {
             String status = rs.getString(5);
             Date lastUpdatedAt = rs.getTimestamp(6);
 
-            return new Approval(userName, clientId, scope, expiresAt, ApprovalStatus.valueOf(status), lastUpdatedAt);
+            Approval approval = new Approval()
+                .setUserId(userName)
+                .setClientId(clientId)
+                .setScope(scope)
+                .setExpiresAt(expiresAt)
+                .setStatus(ApprovalStatus.valueOf(status))
+                .setLastUpdatedAt(lastUpdatedAt);
+            return approval;
         }
     }
 }
diff --git a/...on/src/test/java/org/cloudfoundry/identity/uaa/audit/event/ApprovalModifiedEventTest.java b/...on/src/test/java/org/cloudfoundry/identity/uaa/audit/event/ApprovalModifiedEventTest.java
@@ -7,6 +7,8 @@
 import org.junit.Assert;
 import org.junit.Test;
 
+import java.util.Date;
+
 public class ApprovalModifiedEventTest {
 
     @Test(expected = IllegalArgumentException.class)
@@ -16,7 +18,12 @@ public void testRaisesWithBadSource() throws Exception {
 
     @Test
     public void testAuditEvent() throws Exception {
-        Approval approval = new Approval("mruser", "app", "cloud_controller.read", 1000, Approval.ApprovalStatus.APPROVED);
+        Approval approval = new Approval()
+            .setUserId("mruser")
+            .setClientId("app")
+            .setScope("cloud_controller.read")
+            .setExpiresAt(Approval.timeFromNow(1000))
+            .setStatus(Approval.ApprovalStatus.APPROVED);
 
         ApprovalModifiedEvent event = new ApprovalModifiedEvent(approval, null);
 

diff --git a/common/src/test/java/org/cloudfoundry/identity/uaa/oauth/CheckTokenEndpointTests.java b/common/src/test/java/org/cloudfoundry/identity/uaa/oauth/CheckTokenEndpointTests.java
@@ -230,10 +230,20 @@ public void setUp() {
         Date oneSecondAgo = new Date(System.currentTimeMillis() - 1000);
         Date thirtySecondsAhead = new Date(System.currentTimeMillis() + 30000);
 
-        approvalStore.addApproval(new Approval(userId, "client", "read", thirtySecondsAhead, ApprovalStatus.APPROVED,
+        approvalStore.addApproval(new Approval()
-                        oneSecondAgo));
+            .setUserId(userId)
-        approvalStore.addApproval(new Approval(userId, "client", "write", thirtySecondsAhead, ApprovalStatus.APPROVED,
+            .setClientId("client")
-                        oneSecondAgo));
+            .setScope("read")
+            .setExpiresAt(thirtySecondsAhead)
+            .setStatus(ApprovalStatus.APPROVED)
+            .setLastUpdatedAt(oneSecondAgo));
+        approvalStore.addApproval(new Approval()
+            .setUserId(userId)
+            .setClientId("client")
+            .setScope("write")
+            .setExpiresAt(thirtySecondsAhead)
+            .setStatus(ApprovalStatus.APPROVED)
+            .setLastUpdatedAt(oneSecondAgo));
         tokenServices.setApprovalStore(approvalStore);
         tokenServices.setTokenPolicy(new TokenPolicy(43200, 2592000));
 
@@ -561,8 +571,12 @@ public void testExpiredToken() throws Exception {
     @Test(expected = InvalidTokenException.class)
     public void testUpdatedApprovals() {
         Date thirtySecondsAhead = new Date(System.currentTimeMillis() + 30000);
-        approvalStore.addApproval(new Approval(userId, "client", "read", thirtySecondsAhead, ApprovalStatus.APPROVED,
+        approvalStore.addApproval(new Approval()
-                        new Date()));
+            .setUserId(userId)
+            .setClientId("client")
+            .setScope("read")
+            .setExpiresAt(thirtySecondsAhead)
+            .setStatus(ApprovalStatus.APPROVED));
         Claims result = endpoint.checkToken(accessToken.getValue());
         assertEquals(null, result.getAuthorities());
     }
@@ -571,21 +585,38 @@ public void testUpdatedApprovals() {
     public void testDeniedApprovals() {
         Date oneSecondAgo = new Date(System.currentTimeMillis() - 1000);
         Date thirtySecondsAhead = new Date(System.currentTimeMillis() + 30000);
-        approvalStore.revokeApproval(new Approval(userId, "client", "read", thirtySecondsAhead,
+        approvalStore.revokeApproval(new Approval()
-                        ApprovalStatus.APPROVED,
+            .setUserId(userId)
-                        oneSecondAgo));
+            .setClientId("client")
-        approvalStore.addApproval(new Approval(userId, "client", "read", thirtySecondsAhead, ApprovalStatus.DENIED,
+            .setScope("read")
-                        oneSecondAgo));
+            .setExpiresAt(thirtySecondsAhead)
+            .setStatus(ApprovalStatus.APPROVED)
+            .setLastUpdatedAt(oneSecondAgo));
+        approvalStore.addApproval(new Approval()
+            .setUserId(userId)
+            .setClientId("client")
+            .setScope("read")
+            .setExpiresAt(thirtySecondsAhead)
+            .setStatus(ApprovalStatus.DENIED)
+            .setLastUpdatedAt(oneSecondAgo));
         Claims result = endpoint.checkToken(accessToken.getValue());
         assertEquals(null, result.getAuthorities());
     }
 
     @Test(expected = InvalidTokenException.class)
     public void testExpiredApprovals() {
-        approvalStore.revokeApproval(new Approval(userId, "client", "read", new Date(), ApprovalStatus.APPROVED,
+        approvalStore.revokeApproval(new Approval()
-                        new Date()));
+            .setUserId(userId)
-        approvalStore.addApproval(new Approval(userId, "client", "read", new Date(), ApprovalStatus.APPROVED,
+            .setClientId("client")
-                        new Date()));
+            .setScope("read")
+            .setExpiresAt(new Date())
+            .setStatus(ApprovalStatus.APPROVED));
+        approvalStore.addApproval(new Approval()
+            .setUserId(userId)
+            .setClientId("client")
+            .setScope("read")
+            .setExpiresAt(new Date())
+            .setStatus(ApprovalStatus.APPROVED));
         Claims result = endpoint.checkToken(accessToken.getValue());
         assertEquals(null, result.getAuthorities());
     }