----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44265/#review122532 -----------------------------------------------------------
This patch is way too big. It should have been split into multiple patches. ambari-server/pom.xml (lines 1269 - 1273) <https://reviews.apache.org/r/44265/#comment184585> It seems that this is overkill to just create and carry around a timestamp value. Also, looking at the information for Joda Time (http://www.joda.org/joda-time/) a highlighed portion of it reads: ``` Note that from Java SE 8 onwards, users are asked to migrate to java.time (JSR-310) - a core part of the JDK which replaces this project. ``` If we add this now, we probably will forget to get rid of in when we no longer support JDK 1.7. Do we really need this? ambari-server/src/main/java/org/apache/ambari/server/actionmanager/ActionDBAccessorImpl.java (lines 486 - 490) <https://reviews.apache.org/r/44265/#comment184582> I don't see the need to move this code from inside the `if` clause. Was this really necessary? ambari-server/src/main/java/org/apache/ambari/server/api/services/BaseRequest.java (line 138) <https://reviews.apache.org/r/44265/#comment184681> Since you already have the HttpHeaders, wouldn't it be quicker to add a method to `RequestUtils` to get the remote address from that structure than to get the request from the context? ambari-server/src/main/java/org/apache/ambari/server/api/services/BaseService.java (line 129) <https://reviews.apache.org/r/44265/#comment184589> Is this intended? Maybe it should be documented for clarity. It looks like you are loosing information that should be logged: - The initial request, which may be _lost_ if an exception isn't thrown. - all but the last _internal_ request and its result if more than one _request_ is found in the body of the original request ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/DefaultEventCreator.java (lines 49 - 57) <https://reviews.apache.org/r/44265/#comment184621> This is interesting syntax. Why not use a constructor or initialized the same way other Creator classes do it: ``` requestTypes = ImmutableSet.<Request.Type>builder().add(Request.Type.POST, Request.Type.DELETE, ...).build(); ``` Or maybe use ``` requestTypes = Collections.unmodifiableSet(EnumSet.complementOf(EnumSet.of(Request.Type.GET)); ``` ambari-server/src/main/java/org/apache/ambari/server/security/authentication/AmbariAuthenticationFilter.java (line 42) <https://reviews.apache.org/r/44265/#comment184630> JavaDoc ambari-server/src/main/java/org/apache/ambari/server/security/authorization/AuthorizationHelper.java (line 285) <https://reviews.apache.org/r/44265/#comment184671> This is incorrect. The ResourceType id is not the cluster id. You need to use ``` ClusterDAO.findByResourceID(privilegeEntity.getResource().getId()) ``` ambari-server/src/main/java/org/apache/ambari/server/security/authorization/AuthorizationHelper.java (line 295) <https://reviews.apache.org/r/44265/#comment184672> At least a message should be logged so we aren't confused later. ambari-server/src/main/java/org/apache/ambari/server/serveraction/AbstractServerAction.java (line 199) <https://reviews.apache.org/r/44265/#comment184674> Seems like an AuditLogger factory would be better than this... Then it could be reused rahter than trying to pass around the injector. ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosServerAction.java (line 22) <https://reviews.apache.org/r/44265/#comment184675> This should probably be reverted.. its the only change in the file.. ambari-server/src/main/java/org/apache/ambari/server/utils/RequestUtils.java (line 27) <https://reviews.apache.org/r/44265/#comment184677> Javadoc ambari-server/src/main/java/org/apache/ambari/server/utils/RequestUtils.java (line 32) <https://reviews.apache.org/r/44265/#comment184678> Javadoc - Robert Levas On March 8, 2016, 8:36 a.m., Daniel Gergely wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44265/ > ----------------------------------------------------------- > > (Updated March 8, 2016, 8:36 a.m.) > > > Review request for Ambari, Laszlo Puskas, Oliver Szabo, Robert Levas, Sandor > Magyari, and Sebastian Toader. > > > Bugs: AMBARI-15241 > https://issues.apache.org/jira/browse/AMBARI-15241 > > > Repository: ambari > > > Description > ------- > > Entry points for logging events: > - Login and logout: AmbariAuthenticationFilter, AmbariAuthorizationFilter, > LogoutService > - Operation and tasks: ActionDBAAccessorImpl > - Kerberos related events: CreateKeytabFilesServerAction, > CreatePrincipalServerAction, DestroyPrincipalsServerAction, > FinalizeKerberosServerAction > - REST api: BaseService > > Architecture: > AuditLogger injectable is created and wired in by Guice. The default > implementation (AuditLoggerDefaultImpl) for this interface logs to a file. > Considering performance impact, there is a buffered audit logger > implementation (BufferedAuditLogger) that is a wrapper for an audit logger > implementation and does the logging asynchronously. > > Audit loggers have a single log method that accepts an AuditEvent object. At > the entry points an AuditEvent is assembled and log method is called (except > for the REST api, see below...) > > REST Api: > In BaseService, there is a RequestAuditLogger, that is responsible for > handling and assembling AuditEvents. It also has a log method, but the > parameters for that is the Request and Result objects. > RequestAuditLogger is a small framework for that plugins can be implemented > to handle different type of requests. > Plugins implements RequestAuditEventCreator interface and can be set to > handle one or multiple request types (PUT, POST, DELETE, etc...), resource > types (HOST_COMPONENT, BLUEPRINT, etc..) and results statuses (200 OK, 202 > ACCEPTED, 404 NOT_FOUND, etc). If null is returned by these getters, it means > "all". If there are more than one RequestAuditEventCreators can handle a > request, then the most specific is called. (Priority order: resourceType > > resultStatus > requestType) > For example it is possible to define a plugin for all the POST requests, but > if there is an other plugin for POST request and for resource type > HOST_COMPONENT, then this latter is used (because it is more specific). The > method createAuditEvent is responsible for creating the AuditEvent object for > the RequestAuditEventLogger. If null is returned, then request is not logged > (can be used for ignoring events). > Plugins are registered in ControllerModule and wired by guice to the > RequestAuditLogger. If a new plugin is needed, then implementing the > RequestAuditEventCreator interface and registering it in Guice is the only > things to do. (open-closed principle) > > > Diffs > ----- > > ambari-project/pom.xml ed94004 > ambari-server/conf/unix/log4j.properties 2ee32d4 > ambari-server/conf/windows/log4j.properties cc40f74 > ambari-server/pom.xml e3409b9 > ambari-server/src/main/conf/log4j.properties 8e6652d > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/ActionDBAccessorImpl.java > 23686c3 > > ambari-server/src/main/java/org/apache/ambari/server/api/services/BaseRequest.java > a33e8a7 > > ambari-server/src/main/java/org/apache/ambari/server/api/services/BaseService.java > 7945599 > > ambari-server/src/main/java/org/apache/ambari/server/api/services/LogoutService.java > df8faaa > > ambari-server/src/main/java/org/apache/ambari/server/api/services/Request.java > ff9b6c7 > ambari-server/src/main/java/org/apache/ambari/server/audit/AuditLogger.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/AuditLoggerDefaultImpl.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/AuditLoggerModule.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/BufferedAuditLogger.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/AbstractAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/AbstractUserAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/AccessUnauthorizedAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/AuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/LoginAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/LogoutAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/OperationStatusAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/TaskStatusAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/kerberos/AbstractKerberosAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/kerberos/ChangeSecurityStateKerberosAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/kerberos/CreateKeyTabKerberosAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/kerberos/CreatePrincipalKerberosAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/kerberos/DestroyPrincipalKerberosAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/ActivateUserRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/AddAlertGroupRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/AddAlertTargetRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/AddBlueprintRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/AddComponentToHostRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/AddCredentialRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/AddHostRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/AddRepositoryRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/AddRepositoryVersionRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/AddRequestRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/AddUpgradeRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/AddUserToGroupRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/AddViewInstanceRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/AdminUserRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/BlueprintExportRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/ChangeAlertGroupRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/ChangeAlertTargetRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/ChangeRepositoryVersionRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/ChangeViewInstanceRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/ClientConfigDownloadRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/ClusterNameChangeRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/ClusterPrivilegeChangeRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/ConfigurationChangeRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/CreateGroupRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/CreateUserRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/DeleteAlertGroupRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/DeleteAlertTargetRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/DeleteBlueprintRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/DeleteGroupRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/DeleteHostRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/DeleteRepositoryVersionRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/DeleteServiceRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/DeleteUserRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/DeleteViewInstanceRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/MembershipChangeRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/PrivilegeChangeRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/RemoveUserFromGroupRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/StartOperationRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/UpdateRepositoryRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/UpdateUpgradeItemRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/UserPasswordChangeRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/event/request/ViewPrivilegeChangeRequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/RequestAuditEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/RequestAuditEventCreator.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/RequestAuditLogger.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/RequestAuditLoggerImpl.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/AlertGroupEventCreator.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/AlertTargetEventCreator.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/BlueprintEventCreator.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/BlueprintExportEventCreator.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/ComponentEventCreator.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/ConfigurationChangeEventCreator.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/CredentialEventCreator.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/DefaultEventCreator.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/GroupEventCreator.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/HostEventCreator.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/MemberEventCreator.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/PrivilegeEventCreator.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/RecommendationIgnoreEventCreator.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/RepositoryEventCreator.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/RepositoryVersionEventCreator.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/RequestEventCreator.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/ServiceConfigDownloadEventCreator.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/ServiceEventCreator.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/UnauthorizedEventCreator.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/UpgradeEventCreator.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/UpgradeItemEventCreator.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/UserEventCreator.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/ValidationIgnoreEventCreator.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/ViewInstanceEventCreator.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/audit/request/eventcreator/ViewPrivilegeEventCreator.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java > 882adb2 > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java > 4695990 > > ambari-server/src/main/java/org/apache/ambari/server/controller/ControllerModule.java > daca64d > > ambari-server/src/main/java/org/apache/ambari/server/security/authentication/AmbariAuthenticationFilter.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/security/authorization/AmbariAuthorizationFilter.java > e2a28d0 > > ambari-server/src/main/java/org/apache/ambari/server/security/authorization/AuthorizationHelper.java > b136182 > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/AbstractServerAction.java > 33191bf > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/CreateKeytabFilesServerAction.java > cadfe28 > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/CreatePrincipalsServerAction.java > 8009ae1 > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/DestroyPrincipalsServerAction.java > 93daae8 > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/FinalizeKerberosServerAction.java > c710b8e > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosServerAction.java > 90d9414 > > ambari-server/src/main/java/org/apache/ambari/server/utils/RequestUtils.java > PRE-CREATION > ambari-server/src/main/resources/webapp/WEB-INF/spring-security.xml 3bbc785 > > ambari-server/src/test/java/org/apache/ambari/server/api/services/BaseServiceTest.java > 19eeffb > > ambari-server/src/test/java/org/apache/ambari/server/audit/AccessUnauthorizedAuditEventTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/audit/BufferedAuditLoggerTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/audit/LoginAuditEventTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/audit/LogoutAuditEventTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/audit/OperationStatusAuditEventTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/audit/StartOperationRequestAuditEventTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/audit/request/AbstractBaseCreator.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/audit/request/AllGetCreator.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/audit/request/AllPostAndPutCreator.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/audit/request/DefaultEventCreatorTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/audit/request/PutHostComponentCreator.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/audit/request/RequestAuditLogModule.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/audit/request/RequestAuditLoggerTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/checks/UpgradeCheckOrderTest.java > d4ff566 > > ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerTest.java > 498fddf > > ambari-server/src/test/java/org/apache/ambari/server/notifications/DispatchFactoryTest.java > 6834d5c > > ambari-server/src/test/java/org/apache/ambari/server/orm/InMemoryDefaultTestModule.java > 3ecfe14 > > ambari-server/src/test/java/org/apache/ambari/server/orm/JdbcPropertyTest.java > c07382b > > ambari-server/src/test/java/org/apache/ambari/server/security/authentication/AmbariAuthenticationFilterTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/security/authorization/AmbariAuthorizationFilterTest.java > b30bff3 > > ambari-server/src/test/java/org/apache/ambari/server/security/authorization/AmbariLdapAuthenticationProviderForDNWithSpaceTest.java > da8d9bc > > ambari-server/src/test/java/org/apache/ambari/server/security/authorization/AmbariLdapAuthenticationProviderTest.java > d48be85 > > ambari-server/src/test/java/org/apache/ambari/server/security/authorization/AmbariLocalUserDetailsServiceTest.java > c410f5b > > ambari-server/src/test/java/org/apache/ambari/server/security/authorization/LdapServerPropertiesTest.java > 0797239 > > ambari-server/src/test/java/org/apache/ambari/server/utils/RequestUtilsTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/44265/diff/ > > > Testing > ------- > > Unit tests are added to cover functionality. > > All tests passed on local machine. > > > Thanks, > > Daniel Gergely > >
