Merge pull request #2 from gresham-computing/client-secret-security

DWN-27040: Changes when the client secret is given to the UI

Author: gresham-Carling

openid-connect-server/src/main/java/org/mitre/oauth2/service/impl/DefaultOAuth2ClientDetailsEntityService.java

@@ -150,12 +150,16 @@ public ClientDetailsEntity saveNewClient(ClientDetailsEntity client) {
 
 		ensureNoReservedScopes(client);
 
+		String plaintextSecret = client.getClientSecret();
+
 		if(!Strings.isNullOrEmpty(client.getClientSecret())) {
 		    client.setClientSecret(this.passwordEncoder.encode(client.getClientSecret()));
         }
 
 		ClientDetailsEntity c = clientRepository.saveClient(client);
 
+		c.setClientSecret(plaintextSecret);
+
 		statsService.resetCache();
 
 		return c;
@@ -432,9 +436,11 @@ public ClientDetailsEntity updateClient(ClientDetailsEntity oldClient, ClientDet
 			// make sure a client doesn't get any special system scopes
 			ensureNoReservedScopes(newClient);
 
-            if(!Strings.isNullOrEmpty(newClient.getClientSecret())) {
+			if (Strings.isNullOrEmpty(newClient.getClientSecret())){
+				newClient.setClientSecret(oldClient.getClientSecret());
+			}else{
                 newClient.setClientSecret(this.passwordEncoder.encode(newClient.getClientSecret()));
-            }
+			}
 
 			return clientRepository.updateClient(oldClient.getId(), newClient);
 		}

openid-connect-server/src/main/java/org/mitre/openid/connect/web/ClientAPI.java

@@ -229,6 +229,9 @@ public PKCEAlgorithm deserialize(JsonElement json, Type typeOfT, JsonDeserializa
 	public String apiGetAllClients(Model model, Authentication auth) {
 
 		Collection<ClientDetailsEntity> clients = clientService.getAllClients();
+
+		clients.forEach(client -> client.setClientSecret(null));
+
 		model.addAttribute(JsonEntityView.ENTITY, clients);
 
 		if (AuthenticationUtilities.isAdmin(auth)) {
@@ -320,6 +323,8 @@ public String apiAddClient(@RequestBody String jsonString, Model m, Authenticati
 
 		try {
 			ClientDetailsEntity newClient = clientService.saveNewClient(client);
+
+			//Set the client secret to the plaintext from the request
 			m.addAttribute(JsonEntityView.ENTITY, newClient);
 
 			if (AuthenticationUtilities.isAdmin(auth)) {
@@ -385,6 +390,7 @@ public String apiUpdateClient(@PathVariable("id") Long id, @RequestBody String j
 		}
 
 		ClientDetailsEntity oldClient = clientService.getClientById(id);
+		String plaintextSecret = client.getClientSecret();
 
 		if (oldClient == null) {
 			logger.error("apiUpdateClient failed; client with id " + id + " could not be found.");
@@ -408,10 +414,10 @@ public String apiUpdateClient(@PathVariable("id") Long id, @RequestBody String j
 				|| client.getTokenEndpointAuthMethod().equals(AuthMethod.SECRET_POST)
 				|| client.getTokenEndpointAuthMethod().equals(AuthMethod.SECRET_JWT)) {
 
-			// if they've asked for us to generate a client secret (or they left it blank but require one), do so here
-			if (json.has("generateClientSecret") && json.get("generateClientSecret").getAsBoolean()
-					|| Strings.isNullOrEmpty(client.getClientSecret())) {
+			// Once a client has been created, we only update the secret when asked to
+			if (json.has("generateClientSecret") && json.get("generateClientSecret").getAsBoolean()) {
 				client = clientService.generateClientSecret(client);
+				plaintextSecret = client.getClientSecret();
 			}
 
 		} else if (client.getTokenEndpointAuthMethod().equals(AuthMethod.PRIVATE_KEY)) {
@@ -438,6 +444,10 @@ public String apiUpdateClient(@PathVariable("id") Long id, @RequestBody String j
 
 		try {
 			ClientDetailsEntity newClient = clientService.updateClient(oldClient, client);
+
+			//Set the client secret to the plaintext from the request
+			newClient.setClientSecret(plaintextSecret);
+
 			m.addAttribute(JsonEntityView.ENTITY, newClient);
 
 			if (AuthenticationUtilities.isAdmin(auth)) {
@@ -497,6 +507,9 @@ public String apiShowClient(@PathVariable("id") Long id, Model model, Authentica
 			return JsonErrorView.VIEWNAME;
 		}
 
+		//We don't want the UI to get the secret
+		client.setClientSecret(null);
+
 		model.addAttribute(JsonEntityView.ENTITY, client);
 
 		if (AuthenticationUtilities.isAdmin(auth)) {

openid-connect-server/src/main/java/org/mitre/openid/connect/web/DynamicClientRegistrationEndpoint.java

@@ -242,6 +242,7 @@ public String registerNewClient(@RequestBody String jsonString, Model m) {
 				// send it all out to the view
 
 				RegisteredClient registered = new RegisteredClient(savedClient, token.getValue(), config.getIssuer() + "register/" + UriUtils.encodePathSegment(savedClient.getClientId(), "UTF-8"));
+
 				m.addAttribute("client", registered);
 				m.addAttribute(HttpCodeView.CODE, HttpStatus.CREATED); // http 201
 
@@ -377,6 +378,9 @@ public String updateClient(@PathVariable("id") String clientId, @RequestBody Str
 
 				RegisteredClient registered = new RegisteredClient(savedClient, token.getValue(), config.getIssuer() + "register/" + UriUtils.encodePathSegment(savedClient.getClientId(), "UTF-8"));
 
+				// We don't want the UI to receive the client secret
+				registered.setClientSecret(null);
+
 				// send it all out to the view
 				m.addAttribute("client", registered);
 				m.addAttribute(HttpCodeView.CODE, HttpStatus.OK); // http 200