[
https://issues.apache.org/jira/browse/ARTEMIS-4582?focusedWorklogId=908057&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-908057
]
ASF GitHub Bot logged work on ARTEMIS-4582:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 04/Mar/24 12:33
Start Date: 04/Mar/24 12:33
Worklog Time Spent: 10m
Work Description: gemmellr commented on code in PR #4820:
URL: https://github.com/apache/activemq-artemis/pull/4820#discussion_r1510989284
##########
artemis-cli/src/test/java/org/apache/activemq/cli/test/ArtemisTest.java:
##########
@@ -786,7 +786,7 @@ private void
testProperReloadWhenAddingUserViaManagement(boolean basic) throws E
try {
activeMQServerControl.createAddress("myAddress",
RoutingType.ANYCAST.toString());
- activeMQServerControl.addSecuritySettings("myAddress", "myRole",
"myRole", "myRole", "myRole", "myRole", "myRole", "myRole", "myRole", "myRole",
"myRole");
+ activeMQServerControl.addSecuritySettings("myAddress", "myRole",
"myRole", "myRole", "myRole", "myRole", "myRole", "myRole", "myRole", "myRole",
"myRole", "", "");
Review Comment:
Or use the constructor that doesnt have them, if they arent actually being
used?
(Or not add yet another constructor).
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/security/Role.java:
##########
@@ -247,6 +271,12 @@ public String toString() {
if (browse) {
stringReturn.append(" browse ");
}
+ if (update) {
+ stringReturn.append(" update ");
+ }
+ if (view) {
+ stringReturn.append(" view ");
+ }
Review Comment:
Feels weird that 'view' is always the first of the new bits in
args/docs...but then is randomly second in additions here/below in the class.
Would be nicer if things were consistently ordered.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/config/PersistedSecuritySetting.java:
##########
@@ -77,7 +83,7 @@ public PersistedSecuritySetting(final String addressMatch,
final String manageRoles,
final String browseRoles,
final String createAddressRoles,
- final String deleteAddressRoles) {
+ final String deleteAddressRoles, String
viewRoles, String updateRoles) {
Review Comment:
The others are all on their own lines, and final....be consistent.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/LegacyLDAPSecuritySettingPlugin.java:
##########
@@ -463,7 +463,9 @@ private void processSearchResult(Map<String, Set<Role>>
securityRoles,
mapAdminToManage ? admin : false, //
manage - map to admin based on configuration
read, //
browse
admin, //
createAddress
- admin); //
deleteAddress
+ admin, //
deleteAddress
+ read, //
view
+ write); //
edit
Review Comment:
edit -> update?
##########
artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/tools/xml/XmlDataImporter.java:
##########
@@ -337,17 +337,27 @@ private void sendMessage(List<String> queues, Message
message) throws Exception
logger.debug("Requesting ID for: {}", queue);
}
ClientMessage reply = requestor.request(managementMessage);
- Number idObject = (Number) ManagementHelper.getResult(reply);
- queueID = idObject.longValue();
+ if (ManagementHelper.hasOperationSucceeded(reply)) {
+ Number idObject = (Number) ManagementHelper.getResult(reply);
+ queueID = idObject.longValue();
+ } else {
+ if (debugLog) {
+ logger.debug("Failed to get ID for {} is: {}", queue,
ManagementHelper.getResult(reply, String.class));
+ }
Review Comment:
Message doesnt really make sense.
Seems weird that it outputs this...then continues down and immediately logs
the ID it just 'failed to get'.
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/management/ObjectNameBuilder.java:
##########
@@ -161,7 +161,7 @@ private String getActiveMQServerName() {
return String.format("%s:broker=%s", domain, (jmxUseBrokerName &&
brokerName != null) ? ObjectName.quote(brokerName) : "artemis");
}
- public ObjectName getManagementContextObjectName() throws Exception {
- return
ObjectName.getInstance(String.format("hawtio:type=security,area=jmx,name=ArtemisJMXSecurity"));
+ public ObjectName getSecurityObjectName() throws Exception {
Review Comment:
Is this a breaking change, given the package this class is its in? Should a
deprecated method remain with the old name?
##########
artemis-server/src/test/java/org/apache/activemq/artemis/core/security/RoleTest.java:
##########
@@ -91,12 +91,12 @@ public void testManageRole() throws Exception {
@Test
public void testEqualsAndHashcode() throws Exception {
- Role role = new Role("testEquals", true, true, true, false, false,
false, false, false, false, false);
- Role sameRole = new Role("testEquals", true, true, true, false, false,
false, false, false, false, false);
- Role roleWithDifferentName = new Role("notEquals", true, true, true,
false, false, false, false, false, false, false);
- Role roleWithDifferentRead = new Role("testEquals", false, true, true,
false, false, false, false, false, false, false);
- Role roleWithDifferentWrite = new Role("testEquals", true, false, true,
false, false, false, false, false, false, false);
- Role roleWithDifferentCreate = new Role("testEquals", true, true, false,
false, false, false, false, false, false, false);
+ Role role = new Role("testEquals", true, true, true, false, false,
false, false, false, false, false, false, false);
+ Role sameRole = new Role("testEquals", true, true, true, false, false,
false, false, false, false, false, false, false);
+ Role roleWithDifferentName = new Role("notEquals", true, true, true,
false, false, false, false, false, false, false, false, false);
+ Role roleWithDifferentRead = new Role("testEquals", false, true, true,
false, false, false, false, false, false, false, false, false);
+ Role roleWithDifferentWrite = new Role("testEquals", true, false, true,
false, false, false, false, false, false, false, false, false);
+ Role roleWithDifferentCreate = new Role("testEquals", true, true, false,
false, false, false, false, false, false, false, false, false);
Review Comment:
Seems like you should be adding some tests around here
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/AddressControlImpl.java:
##########
@@ -485,7 +473,6 @@ public String sendMessage(final Map<String, String> headers,
try {
return sendMessage(addressInfo.getName(), server, headers, type,
body, durable, user, password, createMessageId);
} catch (Exception e) {
- e.printStackTrace();
Review Comment:
Maybe make it a trace log? Just lost any ability to see the stacktrace at
all (given it isnt passed to the caller) and potentially even the type (if the
exception message doesnt convey it).
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/security/Role.java:
##########
@@ -109,6 +115,22 @@ public Role(final String name,
final boolean browse,
final boolean createAddress,
final boolean deleteAddress) {
+ this(name, send, consume, createDurableQueue, deleteDurableQueue,
createNonDurableQueue, deleteNonDurableQueue, manage, browse, createAddress,
deleteAddress, false, false);
+ }
+
+ public Role(final String name,
+ final boolean send,
+ final boolean consume,
+ final boolean createDurableQueue,
+ final boolean deleteDurableQueue,
+ final boolean createNonDurableQueue,
+ final boolean deleteNonDurableQueue,
+ final boolean manage,
+ final boolean browse,
+ final boolean createAddress,
+ final boolean deleteAddress,
+ final boolean view,
+ final boolean update) {
Review Comment:
What does it mean to have manage=true, but view and update = false (e.g as
they may be by default, looking at the other constructor, which would be used
by all previously existing code) ?
Issue Time Tracking
-------------------
Worklog Id: (was: 908057)
Time Spent: 1h 20m (was: 1h 10m)
> add view and update permissions to augment the manage rbac for control
> resources
> --------------------------------------------------------------------------------
>
> Key: ARTEMIS-4582
> URL: https://issues.apache.org/jira/browse/ARTEMIS-4582
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Components: Broker, Configuration, JMX, Web Console
> Affects Versions: 2.31.0
> Reporter: Gary Tully
> Assignee: Gary Tully
> Priority: Major
> Time Spent: 1h 20m
> Remaining Estimate: 0h
>
> we have the manage permission that allows sending to the management address,
> to access any control resource. We don't however distinguish what a user can
> do.
> We should segment control operations into categories: CRUD provides a basis
> view for get/is (Read)
> update for set or operations that mutate or modify.
> We allow this sort of configuration via management.xml for jmx mbean access
> but using a different model based on object name.
> All of the mbeans delegate to the control resources.
> If we add these two additional permissions then we can have a single rbac
> model (that supports config reload) and more granularity on control resource
> access from the management address.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
