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] Account default permissions #1398

Merged
merged 2 commits into from
Jun 2, 2020
Merged

[ADDED] Account default permissions #1398

merged 2 commits into from
Jun 2, 2020

Conversation

matthiashanel
Copy link
Contributor

Adding default permissions to account (config and jwt).
This simplifies config of the common user.
This happens in the authorization block already.

This change corresponds to nats-io/jwt#75

server/accounts.go Outdated Show resolved Hide resolved
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

A left over debug print it seems and question about use or not of Permissions.clone().

server/accounts.go Outdated Show resolved Hide resolved
if p == nil && acc.default_perms != nil {
json, _ := json.Marshal(*acc.default_perms)
log.Printf("apply default_perms %+v to %s\n", string(json), uc.Subject)
p = acc.default_perms
Copy link
Member

Choose a reason for hiding this comment

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

We have to ask ourselves: would there be a case where we alter this user's permissions after that. If so, that would change the account's default user. It exists a Permissions.clone() that makes a copy of permissions. If need be, we may need to use that here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went back and forth on this. Maybe it's safest to clone them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kozlovic the reason for not calling clone was that auth.defaultPermissions was not cloned either. Change it for both I assume?

Copy link
Member

Choose a reason for hiding this comment

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

I would clone, unless we are sure they will never be altered (I mean at the user-level, consequently altering the default values).

Copy link
Member

Choose a reason for hiding this comment

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

A precision to that: if we start to have lots and lots of users, and we are sure that if there are default perms we don't alter them, then we could save mem allocs. So I guess it is more a question of ensuring that if a user uses default perms, then we never ever modify them. Or if we have to, then that's when we do a clone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kozlovic I noticed that when user are copied from options to the server, their permissions are cloned. This is why I don't call clone in that code. However when generating an nkey user on the fly I did add clone to arrive at the same data structure as when configured.

server/opts.go Show resolved Hide resolved
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@aricart aricart left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Matthias Hanel <mh@synadia.com>
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

Small changes so that we report the user dup from the "users" section.

server/config_check_test.go Outdated Show resolved Hide resolved
server/opts.go Show resolved Hide resolved
server/opts.go Show resolved Hide resolved
server/opts.go Outdated Show resolved Hide resolved
server/opts.go Outdated Show resolved Hide resolved
Signed-off-by: Matthias Hanel <mh@synadia.com>
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM

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

4 participants