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

JwtUtils: support subs, data, payload & default to no limit / -1 #909

Merged
merged 3 commits into from
May 13, 2023
Merged

JwtUtils: support subs, data, payload & default to no limit / -1 #909

merged 3 commits into from
May 13, 2023

Conversation

MauriceVanVeen
Copy link
Collaborator

The JwtUtils didn't include the subs, data and payload. This made the server default those limits to 0, not allowing for any subs to be created, etc.

Looking at the NATS JWT Go implementation:

// NewUserClaims creates a user JWT with the specific subject/public key
func NewUserClaims(subject string) *UserClaims {
	if subject == "" {
		return nil
	}
	c := &UserClaims{}
	c.Subject = subject
	c.Limits = Limits{
		UserLimits{CIDRList{}, nil, ""},
		NatsLimits{NoLimit, NoLimit, NoLimit},
	}
	return c
}

These fields are defaulted to NoLimit, so -1, in the NatsLimits (which contains the subs, data, payload).

To support the Java client I've added support for overwriting these fields in the UserClaim, as well as defaulting them to NO_LIMIT = -1 to ensure these are set explicitly in the created JWT, instead of defaulted by the server to 0.

This fixes an issue when creating a JWT without these fields explicitly defined, and the server logging about it when trying to create subs and publishing data:

[115788] 2023/05/10 23:45:32.652619 [ERR] 127.0.0.1:48848 - cid:7 - maximum subscriptions exceeded
[115788] 2023/05/10 23:45:32.657450 [ERR] 127.0.0.1:48848 - cid:7 - maximum payload exceeded: 29 vs 0

Copy link
Contributor

@scottf scottf left a comment

Choose a reason for hiding this comment

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

LGTM

@netesh3
Copy link

netesh3 commented May 13, 2023

It's look fine now, but we need to add JavaDoc and more unit tests to cover all scenarios.

@netesh3
Copy link

netesh3 commented May 13, 2023

It would be better to avoid displaying "-1" in the JSON when the LIMIT value for subscriptions, data, and payload is not explicitly set. This can cause confusion as "-1" may be interpreted as indicating no subscription. Therefore, it is recommended to omit the "-1" value in the JSON if the user has not set the limit explicitly.

{
  "exp": 1684092852,
  "iat": 1684006452,
  "jti": "HMWH4X6SARWN4GDJCSXOO4I67LFJ6MJAHUVZKEJFZPWQ4DG26UCA",
  "iss": "ADNLAFFCSB73PV3LA3JLSBCXSGMTE2DZNBIB2DWHD7YMQCJ2ZUO6OGJF",
  "name": "UBLGKWI7N76L5UKJ3QT7UFQHMRZWSNFWVI5W7JB4GZCI462DE3OMURXR",
  "nats": {
    "issuer_account": "ADNLAFFCSB73PV3LA3JLSBCXSGMTE2DZNBIB2DWHD7YMQCJ2ZUO6OGJF",
    "type": "user",
    "version": 2,
    "pub": {
      "allow": [
        "subject.foo"
      ]
    },
    "sub": {
      "allow": [
        "subject.foo"
      ]
    },
    **"subs": -1,
    "data": -1,
    "payload": -1**
  },
  "sub": "UBLGKWI7N76L5UKJ3QT7UFQHMRZWSNFWVI5W7JB4GZCI462DE3OMURXR"
}

@MauriceVanVeen
Copy link
Collaborator Author

@netesh3 the -1 is needed in this case and can't be omitted. -1 means infinite, while 0 would mean no subs allowed for example, in that case it could be omitted since it'd be defaulted to 0 anyhow.

If you'd omit the -1 value it will be defaulted by the server to 0, which is not what you'd want. The -1 is required to be explicitly set to have this fixed.

@netesh3
Copy link

netesh3 commented May 13, 2023

@netesh3 the -1 is needed in this case and can't be omitted. -1 means infinite, while 0 would mean no subs allowed for example, in that case it could be omitted since it'd be defaulted to 0 anyhow.

If you'd omit the -1 value it will be defaulted by the server to 0, which is not what you'd want. The -1 is required to be explicitly set to have this fixed.

My intention was to conceal the "subs," "data," and "payload" fields from the JWT. If the user does not explicitly set these fields, they should not be included in the JWT. However, internally, we will still set these fields behind the scenes. ignore it, just document it out the reason why -1.

@scottf scottf merged commit 3306c48 into nats-io:main May 13, 2023
1 check passed
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.

None yet

3 participants