Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added OPAQUE text encoding, using base64 #676

Merged

Conversation

davideicardi
Copy link
Contributor

Signed-off-by: Davide Icardi davide.icardi@gmail.com

See #675

@davideicardi
Copy link
Contributor Author

davideicardi commented Apr 5, 2019

Some doubts:

  • Do we need to handle null values?
  • It is correct to assume that the value is a byte[]? Casting ((byte[])val) is safe?

Copy link
Contributor

@sbernard31 sbernard31 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to handle null values?

This is not possible to create a LwM2mSingleResource with a null value : see code

It is correct to assume that the value is a byte[]? Casting ((byte[])val) is safe?

The LwM2mResource javadoc says : "If getType() returns Type.OPAQUE, the value is a byte array."

So I think we're good :)

Thx for this, did you plan to push the decoder in this PR or in another one ?

@davideicardi davideicardi force-pushed the feature/opaque-text-encoding branch from b35dddf to 76ef3b4 Compare April 8, 2019 13:25
@davideicardi
Copy link
Contributor Author

davideicardi commented Apr 8, 2019

I will now have a look at the decoder. I think to add it here if for you is ok.

@sbernard31
Copy link
Contributor

I think to add it here if for you is ok.

👍

@davideicardi davideicardi force-pushed the feature/opaque-text-encoding branch from 9f4a9e5 to 5dc6242 Compare April 8, 2019 15:05
@davideicardi
Copy link
Contributor Author

Added decoder and test. I have squashed all commits ...

Do you think that we need a try/catch inside the decoder?

@sbernard31
Copy link
Contributor

Do you think that we need a try/catch inside the decoder?

I would say we should handle the case where the string is not a valid base64 payload and so raise a CodecException !

But the Base64 javadoc is not so verbose about that ...
(this is a copy paste from apache commons codec, we didn't want to depends to this jar only for 2 methods)

So reading the code, it seems that IllegalStateException could be raised ?
There is also a isBase64 method ...
So several solutions :

  • if isBase64() ?
  • try catch IllegalStateException ?
  • try catch Exception ? (probably not a so good idea)

We should add some test with bad payload, we will see how base64 behave and you could try several solutions and choose the best one ?

@davideicardi
Copy link
Contributor Author

I have tried to write a test with an input that cause the decode to throw but without success. It seems that when an invalid characters is found it simply ignore it or trasform it to a +.

I have added a test to ensure this behaviour, see text_decode_opaque_should_ignore_not_base64_chars.

We can probably use isBase64 and throw but not sure if it is a good idea, maybe the validation should be performed at an higher level...

In any case I'm open to suggestions, I will follow you ;-)

@sbernard31
Copy link
Contributor

It seems that when an invalid characters is found it simply ignore it or transform it to a +

With this implementation, Invalid characters is ignored.
The "-" transform in "+" is about url safe variant. see rfc4648#section-5, by default this implementation is able to decode both standard and url safe alphabet. But we encode with standard alphabet.

As I was a bit lost with all of this ... I read the RFC4648.
And my understanding is that rfc recommends to :

By the way the LWM2M specification does not talk about url safe alphabet.

So a perfect solution should be to have a default base64 decoder which :

  1. rejects url safe alphabet : A-BB should be rejected (the good one is A+BB)
  2. rejects invalid character : 'A+B;B! should be rejected
  3. rejects 1 bit in padding : ABC= should be rejected (the canonical form is ABA=)
  4. rejects missing padding character : ABA should be rejected (missing pad char ABA=)

by raising a CodecException.
And if user want a different behavior it could change it. (we will see later how to do that)

  1. It seems we can not do that with this implementation ...
  2. is doable with isBase64 check...
  3. it seems we can not do that with this implementation ...
  4. it seems we can not do that with this implementation ...

So, we can :

  • go for a first shot implementation with just 2. and searching to improve this in a second time by searching a more appropriate implementation.
  • or we search a good implementations for our needs now.

@davideicardi
Copy link
Contributor Author

Thanks! Good investigation!

If for you is ok I will proceed with:

go for a first shot implementation with just 2. and searching to improve this in a second time by searching a more appropriate implementation.

I will first call isBase64 and in case of false I will throw a CodecException.

Ok?

@sbernard31
Copy link
Contributor

I will first call isBase64 and in case of false I will throw a CodecException.
Ok?

👍

Signed-off-by: Davide Icardi <davide.icardi@gmail.com>
@davideicardi davideicardi force-pushed the feature/opaque-text-encoding branch from 96b3108 to bff1ab4 Compare April 9, 2019 13:16
@davideicardi
Copy link
Contributor Author

Ok done! let met know if you see problems ...

@sbernard31
Copy link
Contributor

Sounds good, I will integrate this in master. (I create a new issue about issue we discover here, see #678 )

@sbernard31 sbernard31 merged commit bff1ab4 into eclipse-leshan:master Apr 9, 2019
@sbernard31
Copy link
Contributor

sbernard31 commented Apr 9, 2019

Thx @davideicardi ! 🙏

@davideicardi
Copy link
Contributor Author

Great! Thank you @sbernard31 !
There is a nightly build or something similar available? In other words there is an easy way to get the master/unstable version from a maven repo?

@sbernard31
Copy link
Contributor

We have a jenkins build : https://hudson.eclipse.org/leshan/job/leshan/, but jars are not deployed ... 😞

But building jars locally is pretty easy, doing this you will have a leshan-*-SNAPSHOT available in your local maven repository and so you could test it easily just adding SNAPSHOT version to you dependencies.

Last point, if you need a release just let me know :)

@davideicardi
Copy link
Contributor Author

The problem is that we have some build pipelines running with the Leshan dependency ... so building it locally can be a little difficult ... Taking it from a maven repo is easier...
If you are able to release a new version in one week or two I will appreciate, otherwise no problem, I will pubish it in some temporary repo 😉

@sbernard31
Copy link
Contributor

If you are able to release a new version in one week or two I will appreciate

I will try, but maybe doing some validation before could be a good idea 🤔
(I would like to integrate this PR #657 too but I wait for a teammate review :) )

@sbernard31
Copy link
Contributor

@davideicardi did you made some tests ?
I plan to release a 1.0.0-M11 today ?

@davideicardi
Copy link
Contributor Author

Good news! I have used a local compiled version during these days and for now it is working fine. Green light for me!

@sbernard31
Copy link
Contributor

You probably don't care but finally #657 will not be integrated in this release. (next one)

@davideicardi
Copy link
Contributor Author

Yes, no problem for me.

I think that now we can close #675 issue right? we can leave #678 open for the enhancements ..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants