From c12cb181978d21d1865db6ce91ebef1d202dbfcb Mon Sep 17 00:00:00 2001 From: Carling Knight Date: Mon, 3 Dec 2018 16:18:48 +0000 Subject: [PATCH 1/3] DWN-27040: Changes when the client secret is given to the UI --- ...faultOAuth2ClientDetailsEntityService.java | 6 ++++-- .../mitre/openid/connect/web/ClientAPI.java | 21 ++++++++++++++++--- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/openid-connect-server/src/main/java/org/mitre/oauth2/service/impl/DefaultOAuth2ClientDetailsEntityService.java b/openid-connect-server/src/main/java/org/mitre/oauth2/service/impl/DefaultOAuth2ClientDetailsEntityService.java index f02e773979..ddaa0435a0 100644 --- a/openid-connect-server/src/main/java/org/mitre/oauth2/service/impl/DefaultOAuth2ClientDetailsEntityService.java +++ b/openid-connect-server/src/main/java/org/mitre/oauth2/service/impl/DefaultOAuth2ClientDetailsEntityService.java @@ -432,9 +432,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); } diff --git a/openid-connect-server/src/main/java/org/mitre/openid/connect/web/ClientAPI.java b/openid-connect-server/src/main/java/org/mitre/openid/connect/web/ClientAPI.java index 01edb6b55f..86e359f8e6 100644 --- a/openid-connect-server/src/main/java/org/mitre/openid/connect/web/ClientAPI.java +++ b/openid-connect-server/src/main/java/org/mitre/openid/connect/web/ClientAPI.java @@ -278,6 +278,8 @@ public String apiAddClient(@RequestBody String jsonString, Model m, Authenticati client = clientService.generateClientId(client); } + String plaintextSecret = client.getClientSecret(); + if (client.getTokenEndpointAuthMethod() == null || client.getTokenEndpointAuthMethod().equals(AuthMethod.NONE)) { // we shouldn't have a secret for this client @@ -292,6 +294,7 @@ public String apiAddClient(@RequestBody String jsonString, Model m, Authenticati if (json.has("generateClientSecret") && json.get("generateClientSecret").getAsBoolean() || Strings.isNullOrEmpty(client.getClientSecret())) { client = clientService.generateClientSecret(client); + plaintextSecret = client.getClientSecret(); } } else if (client.getTokenEndpointAuthMethod().equals(AuthMethod.PRIVATE_KEY)) { @@ -320,6 +323,10 @@ 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 + newClient.setClientSecret(plaintextSecret); + m.addAttribute(JsonEntityView.ENTITY, newClient); if (AuthenticationUtilities.isAdmin(auth)) { @@ -385,6 +392,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 +416,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 +446,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 +509,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)) { From 5abebb7c3692dda14ba249365046b92cc8e8e1a4 Mon Sep 17 00:00:00 2001 From: Carling Knight Date: Tue, 4 Dec 2018 15:33:50 +0000 Subject: [PATCH 2/3] DWN-27040: Adds the same secret key limiting to client registration Also removes the client secrets from the client listing --- .../java/org/mitre/openid/connect/web/ClientAPI.java | 3 +++ .../connect/web/DynamicClientRegistrationEndpoint.java | 9 +++++++++ 2 files changed, 12 insertions(+) diff --git a/openid-connect-server/src/main/java/org/mitre/openid/connect/web/ClientAPI.java b/openid-connect-server/src/main/java/org/mitre/openid/connect/web/ClientAPI.java index 86e359f8e6..94c3952867 100644 --- a/openid-connect-server/src/main/java/org/mitre/openid/connect/web/ClientAPI.java +++ b/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 clients = clientService.getAllClients(); + + clients.forEach(client -> client.setClientSecret(null)); + model.addAttribute(JsonEntityView.ENTITY, clients); if (AuthenticationUtilities.isAdmin(auth)) { diff --git a/openid-connect-server/src/main/java/org/mitre/openid/connect/web/DynamicClientRegistrationEndpoint.java b/openid-connect-server/src/main/java/org/mitre/openid/connect/web/DynamicClientRegistrationEndpoint.java index 89e0418ad8..74c106b661 100644 --- a/openid-connect-server/src/main/java/org/mitre/openid/connect/web/DynamicClientRegistrationEndpoint.java +++ b/openid-connect-server/src/main/java/org/mitre/openid/connect/web/DynamicClientRegistrationEndpoint.java @@ -167,6 +167,8 @@ public String registerNewClient(@RequestBody String jsonString, Model m) { if (newClient != null) { // it parsed! + String plaintextSecret = newClient.getClientSecret(); + // // Now do some post-processing consistency checks on it // @@ -201,6 +203,7 @@ public String registerNewClient(@RequestBody String jsonString, Model m) { // we need to generate a secret newClient = clientService.generateClientSecret(newClient); + plaintextSecret = newClient.getClientSecret(); } // set some defaults for token timeouts @@ -242,6 +245,9 @@ 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")); + + registered.setClientSecret(plaintextSecret); + m.addAttribute("client", registered); m.addAttribute(HttpCodeView.CODE, HttpStatus.CREATED); // http 201 @@ -377,6 +383,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 From 2008404afdcb99b781959d14b87fba3c5c12de5c Mon Sep 17 00:00:00 2001 From: Carling Knight Date: Wed, 5 Dec 2018 14:36:38 +0000 Subject: [PATCH 3/3] DWN-27040: Bit of refactoring, Protected Resources now protected --- .../impl/DefaultOAuth2ClientDetailsEntityService.java | 4 ++++ .../main/java/org/mitre/openid/connect/web/ClientAPI.java | 5 ----- .../connect/web/DynamicClientRegistrationEndpoint.java | 5 ----- 3 files changed, 4 insertions(+), 10 deletions(-) diff --git a/openid-connect-server/src/main/java/org/mitre/oauth2/service/impl/DefaultOAuth2ClientDetailsEntityService.java b/openid-connect-server/src/main/java/org/mitre/oauth2/service/impl/DefaultOAuth2ClientDetailsEntityService.java index ddaa0435a0..005db69b54 100644 --- a/openid-connect-server/src/main/java/org/mitre/oauth2/service/impl/DefaultOAuth2ClientDetailsEntityService.java +++ b/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; diff --git a/openid-connect-server/src/main/java/org/mitre/openid/connect/web/ClientAPI.java b/openid-connect-server/src/main/java/org/mitre/openid/connect/web/ClientAPI.java index 94c3952867..a3943fba5a 100644 --- a/openid-connect-server/src/main/java/org/mitre/openid/connect/web/ClientAPI.java +++ b/openid-connect-server/src/main/java/org/mitre/openid/connect/web/ClientAPI.java @@ -281,8 +281,6 @@ public String apiAddClient(@RequestBody String jsonString, Model m, Authenticati client = clientService.generateClientId(client); } - String plaintextSecret = client.getClientSecret(); - if (client.getTokenEndpointAuthMethod() == null || client.getTokenEndpointAuthMethod().equals(AuthMethod.NONE)) { // we shouldn't have a secret for this client @@ -297,7 +295,6 @@ public String apiAddClient(@RequestBody String jsonString, Model m, Authenticati if (json.has("generateClientSecret") && json.get("generateClientSecret").getAsBoolean() || Strings.isNullOrEmpty(client.getClientSecret())) { client = clientService.generateClientSecret(client); - plaintextSecret = client.getClientSecret(); } } else if (client.getTokenEndpointAuthMethod().equals(AuthMethod.PRIVATE_KEY)) { @@ -328,8 +325,6 @@ public String apiAddClient(@RequestBody String jsonString, Model m, Authenticati ClientDetailsEntity newClient = clientService.saveNewClient(client); //Set the client secret to the plaintext from the request - newClient.setClientSecret(plaintextSecret); - m.addAttribute(JsonEntityView.ENTITY, newClient); if (AuthenticationUtilities.isAdmin(auth)) { diff --git a/openid-connect-server/src/main/java/org/mitre/openid/connect/web/DynamicClientRegistrationEndpoint.java b/openid-connect-server/src/main/java/org/mitre/openid/connect/web/DynamicClientRegistrationEndpoint.java index 74c106b661..a36d539d01 100644 --- a/openid-connect-server/src/main/java/org/mitre/openid/connect/web/DynamicClientRegistrationEndpoint.java +++ b/openid-connect-server/src/main/java/org/mitre/openid/connect/web/DynamicClientRegistrationEndpoint.java @@ -167,8 +167,6 @@ public String registerNewClient(@RequestBody String jsonString, Model m) { if (newClient != null) { // it parsed! - String plaintextSecret = newClient.getClientSecret(); - // // Now do some post-processing consistency checks on it // @@ -203,7 +201,6 @@ public String registerNewClient(@RequestBody String jsonString, Model m) { // we need to generate a secret newClient = clientService.generateClientSecret(newClient); - plaintextSecret = newClient.getClientSecret(); } // set some defaults for token timeouts @@ -246,8 +243,6 @@ public String registerNewClient(@RequestBody String jsonString, Model m) { RegisteredClient registered = new RegisteredClient(savedClient, token.getValue(), config.getIssuer() + "register/" + UriUtils.encodePathSegment(savedClient.getClientId(), "UTF-8")); - registered.setClientSecret(plaintextSecret); - m.addAttribute("client", registered); m.addAttribute(HttpCodeView.CODE, HttpStatus.CREATED); // http 201