[GitHub] activemq-artemis pull request #2423: ARTEMIS-2168 Fix "populate-validated-us...

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2423: ARTEMIS-2168 Fix "populate-validated-us...

jbertram-2
GitHub user michaelandrepearce opened a pull request:

    https://github.com/apache/activemq-artemis/pull/2423

    ARTEMIS-2168 Fix "populate-validated-user" feature for AMQP messages

    When populate-validated-user is true, force conversion to CORE as AMQP messages are immutable.
    Note, this fixes feature: https://issues.apache.org/jira/browse/ARTEMIS-584 for AMQP produced messages.
    Add additional test cases to cover AMQP for feature.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/michaelandrepearce/activemq-artemis ARTEMIS-2168-v2

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/activemq-artemis/pull/2423.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #2423
   
----
commit cce891325f72fc921ff964440861432b2f320fb3
Author: Michael André Pearce <michael.andre.pearce@...>
Date:   2018-11-06T21:30:52Z

    ARTEMIS-2142 Fix ServerJMSMessage
   
    Fix ServerJMSMessage so it correctly transposes JMSX headers.
    Push common JMS to Message Interface mappings into MessageUtil

commit 213f32d70f7f1c2e14686341a581727875e40dc3
Author: Michael André Pearce <michael.andre.pearce@...>
Date:   2018-11-08T22:18:29Z

    ARTEMIS-2168 Fix "populate-validated-user" feature for AMQP messages
   
    When populate-validated-user is true, force conversion to CORE as AMQP messages are immutable.
    Note, this fixes feature: https://issues.apache.org/jira/browse/ARTEMIS-584 for AMQP produced messages.
    Add additional test cases to cover AMQP for feature.

----


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2423: ARTEMIS-2168 Fix "populate-validated-user" fea...

jbertram-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2423
 
    @tabish121 one of the alternative approaches, where when this feature is enabled, we simply convert to CORE, this then avoids making any changes to AMQPMessage (keeping untouched)


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2423: ARTEMIS-2168 Fix "populate-validated-user" fea...

jbertram-2
In reply to this post by jbertram-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2423
 
    im going to close this, and re-base and re-open once the fixes for group seq are merged, so its a cleaner PR.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2423: ARTEMIS-2168 Fix "populate-validated-us...

jbertram-2
In reply to this post by jbertram-2
Github user michaelandrepearce closed the pull request at:

    https://github.com/apache/activemq-artemis/pull/2423


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2423: ARTEMIS-2168 Fix "populate-validated-user" fea...

jbertram-2
In reply to this post by jbertram-2
Github user tabish121 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2423
 
    -1
   
    This is actually worse that before because now you've not only violated the AMQP spec in regards to changing the User ID portion of the properties section but also opened it up to even more spec abuse and probably lost fidelity on the message such that what was sent will not be faithfully reproduced to an AMQP consumer reading the message.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2423: ARTEMIS-2168 Fix "populate-validated-user" fea...

jbertram-2
In reply to this post by jbertram-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2423
 
    So on the options/ideas i put on the other pr which do you think is the best.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2423: ARTEMIS-2168 Fix "populate-validated-user" fea...

jbertram-2
In reply to this post by jbertram-2
Github user tabish121 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2423
 
    I'd say none of them are really the right way to go.  I'd suggest your organization create a custom broker plugin that does whatever protocol violating things are acceptable to you and your team and not allow for the inevitable issues that arise from violating the specification from being dropped directly into the broker.  You could also (if you really care about the message user id no matching) reject incoming messages there are close the connection of the offending client.  


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2423: ARTEMIS-2168 Fix "populate-validated-user" fea...

jbertram-2
In reply to this post by jbertram-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2423
 
    @tabish121 id disagree here, the message is already modified when broker features like enforced expiry-delay are being used.
   
   
    ```  public AMQPMessage setExpiration(long expiration) {
          if (properties != null) {
             if (expiration <= 0) {
                properties.setAbsoluteExpiryTime(null);
             } else {
                properties.setAbsoluteExpiryTime(new Date(expiration));
             }
          } else if (expiration > 0) {
             properties = new Properties();
             properties.setAbsoluteExpiryTime(new Date(expiration));
          }
   
          // We are overriding expiration with an Absolute expiration time so any
          // previous Header based TTL also needs to be removed.
          if (header != null) {
             header.setTtl(null);
          }
   
          this.expiration = Math.max(0, expiration);
   
          return this;
       }```
   
   



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2423: ARTEMIS-2168 Fix "populate-validated-user" fea...

jbertram-2
In reply to this post by jbertram-2
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2423
 
    Amqp headers cannot be changed per spec. I am kind of confused on what’s going on
   
    I’m a bit lost on a week or vacation.  
   
   
    Can I have a TL;dr


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2423: ARTEMIS-2168 Fix "populate-validated-user" fea...

jbertram-2
In reply to this post by jbertram-2
Github user tabish121 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2423
 
    @michaelandrepearce I agree, that expiry bit qualifies as a bug and should also be fixed to disallow modification


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2423: ARTEMIS-2168 Fix "populate-validated-user" fea...

jbertram-2
In reply to this post by jbertram-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2423
 
    Id disagree thats a very importand broker feature.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2423: ARTEMIS-2168 Fix "populate-validated-user" fea...

jbertram-2
In reply to this post by jbertram-2
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2423
 
    Any modification has to be on extra properties.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2423: ARTEMIS-2168 Fix "populate-validated-user" fea...

jbertram-2
In reply to this post by jbertram-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2423
 
    But then that would mean it would not show on JMS properties correctly . Which it does today


---