Alon Bar-Lev has posted comments on this change. Change subject: core, engine: Services for providing info to the Console Proxy ......................................................................
Patch Set 4: (10 comments) http://gerrit.ovirt.org/#/c/35887/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/aaa/GetAllUserProfilesQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/aaa/GetAllUserProfilesQuery.java: this is not aaa Line 1: package org.ovirt.engine.core.bll.aaa; Line 2: Line 3: import org.ovirt.engine.core.bll.QueriesCommandBase; Line 4: import org.ovirt.engine.core.common.queries.VdcQueryParametersBase; http://gerrit.ovirt.org/#/c/35887/4/backend/manager/modules/services/src/main/java/org/ovirt/engine/core/services/ConsoleProxyServlet.java File backend/manager/modules/services/src/main/java/org/ovirt/engine/core/services/ConsoleProxyServlet.java: VMConsoleProxyServlet will match the ovirt-vmconsole package :) Line 1: package org.ovirt.engine.core.services; Line 2: Line 3: import org.apache.commons.lang.StringUtils; Line 4: import org.codehaus.jackson.map.ObjectMapper; Line 49: List<UserProfile> users = v.getReturnValue(); Line 50: Line 51: for (UserProfile user : users) { Line 52: if (StringUtils.isNotEmpty(user.getSshPublicKey())) { Line 53: jsonUsers.put(user.getId().toString(), user.getSshPublicKey()); I would like to have user name as well for auditing purposes Line 54: } Line 55: } Line 56: } Line 57: Line 83: Line 84: for (VM vm : vmsList) { Line 85: IdQueryParameters vmParam = new IdQueryParameters(vm.getId()); Line 86: Line 87: VdcQueryReturnValue retAddr = backend.runInternalQuery(VdcQueryType.GetManagementInterfaceAddressByVmId, vmParam); probably better to have one query to perform the entire sequence instead of call it over and over. Line 88: Line 89: if (retAddr != null && retAddr.getReturnValue() != null) { Line 90: String vdsAddress = (String) retAddr.getReturnValue(); Line 91: Line 111: return buffer.toString(); Line 112: } Line 113: Line 114: private String validateTicket(String ticket) throws GeneralSecurityException, IOException { Line 115: TicketDecoder ticketDecoder = new TicketDecoder(EngineEncryptionUtils.getTrustStore(), null, null, 10000); we will use the ticket validation based on EKU so that a specific EKU will be able to be authenticated without the need to present a specific certificate. Line 116: return ticketDecoder.decode(ticket); Line 117: } Line 118: Line 119: @Override Line 119: @Override Line 120: protected void doPost(HttpServletRequest request, HttpServletResponse response) Line 121: throws ServletException, IOException { Line 122: Line 123: response.setContentType("application/json"); set content type only when success and you actually sending content. Line 124: Line 125: OutputStream outputStream = response.getOutputStream(); Line 126: Line 127: String stringParameters = null; Line 126: Line 127: String stringParameters = null; Line 128: Line 129: try { Line 130: stringParameters = validateTicket(readBody(request.getReader())); much better to use inputstream and copy byte by byte instead of using reader and assume lines. Line 131: } catch (GeneralSecurityException e) { Line 132: log.error("Error validating ticket: ", e); Line 133: response.setStatus(HttpURLConnection.HTTP_FORBIDDEN); Line 134: } catch (IOException e) { Line 133: response.setStatus(HttpURLConnection.HTTP_FORBIDDEN); Line 134: } catch (IOException e) { Line 135: log.error("Error decoding ticket: ", e); Line 136: response.setStatus(HttpURLConnection.HTTP_INTERNAL_ERROR); Line 137: } this try/catch should be for entire function, make sure you do not continue to normal logic if failed. Line 138: Line 139: ObjectMapper mapper = new ObjectMapper(); Line 140: Line 141: Map<String, String> parameters = mapper.readValue( Line 157: result = availableConsoles(backend, userId); Line 158: } else if ("public_keys".equals(command)) { Line 159: result = publicKeys(backend); Line 160: } Line 161: } else? Line 162: Line 163: mapper.writeValue(outputStream, result); Line 164: Line 165: } catch (Exception e) { Line 165: } catch (Exception e) { Line 166: response.setStatus(HttpURLConnection.HTTP_INTERNAL_ERROR); Line 167: log.error("Error processing request: ", e); Line 168: } finally { Line 169: outputStream.close(); try with resources? Line 170: } Line 171: } -- To view, visit http://gerrit.ovirt.org/35887 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I53c721da21cefcf4069d14c7016b6f7d97f9eac9 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vitor de Lima <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Ravi Nori <[email protected]> Gerrit-Reviewer: Shahar Havivi <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
