cshannon commented on PR #2071: URL: https://github.com/apache/activemq/pull/2071#issuecomment-4614854382
> One consequence of this change seems to be that there isn't a means of preventing a client from generating advisories which could be an uncommon thing to want to do but seems like it was possible under the existing model as those users could be assigned a role that didn't allow then write access. Have you given any thought to whether there is some consequence to not having this ability now? So this is very confusing but this is actually a non issue because of how the AdvisoryBroker is configured and these changes also help clarify this. The AdvisoryBroker is set up to bypass authorization checks for publish. The advisory broker is [created ](https://github.com/apache/activemq/blob/f84eb3968a02bb5a9698da508d66c56c72ccdaaf/activemq-broker/src/main/java/org/apache/activemq/broker/BrokerService.java#L2450) before any broker plugins are applied, so it always delegates to the region broker which does not include the authorization plugins which get initialized later [here](https://github.com/apache/activemq/blob/f84eb3968a02bb5a9698da508d66c56c72ccdaaf/activemq-broker/src/main/java/org/apache/activemq/broker/BrokerService.java#L2488). This means that any advisory message sent by the AdvisoryBroker _never_ has permissions checked and is always allowed to be sent as it bypasses auth checks. This is fine and expected because its the broker itself. So trying to block advisories being sent with permissions wouldn't work anyways. So what are we fixing here? The main problem this fixes is destination creation when messages are published to topics that don't exist yet. On creation there are interceptors that cause the creation to go back through the full broker plugin stack and it _does_ get processed by any authorization plugins using the provided connection context. This means creation would get blocked if the user isn't given permission. So by using the broker admin context the creation won't get blocked either and we can take away the requirement to grant create permissions. With these changes it would be possible if we wanted to refactor to include the authorization plugin in the broker stack that is passed to AdvisoryBroker and it would work fine (because it uses the admin context) but there isn't any good reason to do that because we are an admin anyways so we don't need to check. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected] For further information, visit: https://activemq.apache.org/contact
