JwsDescriptor.Payload setter weird behavior.

[olivier-spinelli]

This unit test fails:

        [Fact]
        public void DescriptorPayloadIsAStandardReferenceObject()
        {
            var descriptor = new JwsDescriptor(Jwk.None, SignatureAlgorithm.None);

            var p1 = new JwtPayload { { "One", "Member" } };
            var p1Content = p1.ToString();
            descriptor.Payload = p1;

            var p2 = new JwtPayload { { "Something", "else" } };
            var p2Content = p2.ToString();            
            descriptor.Payload = p2;

            Assert.Equal(p1.ToString(),p1Content);
            Assert.Equal(p2.ToString(),p2Content);
        }

With this:

    Assert.Equal() Failure
                                     ↓ (pos 24)
    Expected: ··· "Something": "else",\r\n  "One": "Member"\r\n}
    Actual:   ··· "Something": "else"\r\n}
                                     ↑ (pos 24)

This is a rather surprising side effect. Considering the implementation (see below _payload.CopyTo(value);), I'm wondering if this is intentional or not...

public override JwtPayload? Payload
{
    get => _payload;
    set
    {
        if (value is null)
        {
            ThrowHelper.ThrowArgumentNullException(ExceptionArgument.value);
        }

        _payload.CopyTo(value);
        _payload = value;
    }
}

If its not, I can make a PR to fix this (including the above test).

[olivier-spinelli]

Please note that the JwtDescriptor.Header implements the exact same behavior.

It seems that the intent was (for the JwtDescriptor.Header at least) to support both:

• Preconfiguring headers from constructors (typically from JwsDescriptor ctor).
• using the init pattern.

[ycrumeyrolle]

This is "by design". These objects are designed for append-only, so

The expected usage is:

    var descriptor = new JwsDescriptor(<any>, SignatureAlgorithm.HS256)
    {
        Header = new JwtHeader { { "One", "Header" } }
        Payload = new JwtPayload { { "One", "Member" } }
    }

The goal is to keep the values of the predefined members, mostly for the header (like "alg"), so the JwtHeader & the JwtPayload are merging with the existing values.

I do understand this is surprising, and the merging might be at another place. We will take a look at this. Maybe keep a reference of 2 differents JwtPayload, or throw an exception when the Payload property is set twice.

[olivier-spinelli]

Ok. I understand... I think I have good news: thanks to the new C# 9 "init" keyword, there is a really clean solution that I pushed here: https://github.com/uruk-project/Jwt/pull/539/commits/fd09a2a0d8376ef0ce6dda603f1460f70ee0595c

That's a miserable failure! I forgot to change my branch before investigating the init idea. Regardless of me being very bad at git, could you have a look at this commit?

If you want I can close the current PR and recreate 2 different ones.

[ycrumeyrolle]

Yes can please keep the current PR for the bug fix, and another PR for the init feature. I had an issue with c# 9 on MacOS, and currently MacOS CI/CD is also disabled (due to another OS issue...). I have a PR in preparation for reenabling the MacOS. Please wait this PR before to upgrade the c# version.

[ycrumeyrolle]

Added init-only for Payload & Header in #543

[olivier-spinelli]

Hello,

I've seen that you have integrated the Payloald and Header setter.

I have 2 remarks:

• It seems to me that the Payload is a very independent beast (as opposed to the Header). This is why I considered it a standard get/set property. This allows to easily reuse the descriptor with different payloads.

• Following this choice (useless if the Payload init is kept): the JwsDescriptor.Payload cannot be null but the base JwtDescriptor is. I can see 3 options:

  • Changing the generic base class (to be investigated).

or:

• Comments.

      /// <inheritdoc />
      /// <remarks>This payload is never null.</remarks>
  public override JwtPayload? Payload

or:

• Removing the warning.

      /// <inheritdoc />
      public override JwtPayload Payload
      {
          get => _payload;
  #pragma warning disable CS8765 // Nullability of type of parameter doesn't match overridden member (possibly because of nullability attributes).
          set
          {
              if (value is null)
              {
                  ThrowHelper.ThrowArgumentNullException(ExceptionArgument.value);
              }
              _payload = value;
          }
  #pragma warning restore CS8765 // Nullability of type of parameter doesn't match overridden member (possibly because of nullability attributes).
      }

[ycrumeyrolle]

Init-only has been removed from the JwtDescriptor.Header & JwtDescriptor.Payload. #566 There is now a check when setting the Header, it can be done only once. This allow the have a similar behavior of init-only, without the requirement of C# 9.0.

JwtDescriptor.Payload is now not nullable as suggested.