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

fix(cli): updates 'grants add' command #1474

Merged
merged 19 commits into from
Apr 8, 2022
Merged

fix(cli): updates 'grants add' command #1474

merged 19 commits into from
Apr 8, 2022

Conversation

kimskimchi
Copy link
Contributor

@kimskimchi kimskimchi commented Apr 5, 2022

Summary

  1. updates grants add command to take in user rather than destination as the command argument.2.
  2. redefines flags
    a) -ito be consistent with our term identity, as well as the identity command
    b) -ican be user, machine, or group. However, if it is a group, you must mark it manually as -g to override detecting the type (user or machine) automatically
    c) flag role is optional
    d) flag provider is situationally required: if more than two providers, and identity is a group or user, then it is required. (machines are local only, so no need to provide flag if identity is a machine)
    e) you can specify identity as a cmd argument OR flag. same behaviour as login's SERVER
    f) removes machine and user flags.
  3. Gives extensive explanation on the flags. 4.
  4. Fixes bug where where it wasn't asking for provider when there are multiple rpoviders5.
  5. Marks flag destination as required, Marks identity value as required (either flag or cmd argument)6.
  6. Role is optional. Use if you want control. Otherwise, it will default to connect. 7.
  7. Adds panic in areas where it is a programming error state - eg, it should never arrive at that state.

Checklist:

  • Wrote appropriate unit tests
  • Considered security implications of the change
  • Updated associated docs where necessary
  • Updated associated configuration where necessary
  • Change is backwards compatible if it needs to be (user can upgrade without manual steps?)
  • Nothing sensitive logged
  • Commit message conforms to Conventional Commit
  • GitHub Actions are passing
  • Considered data migrations for smooth upgrades

Related Issues

Resolves #1250
Resolves #1393
Resolves #1432

internal/cmd/grants_add.go Show resolved Hide resolved
internal/cmd/grants_add.go Outdated Show resolved Hide resolved
internal/cmd/grants_add.go Outdated Show resolved Hide resolved
internal/cmd/grants_add.go Outdated Show resolved Hide resolved
internal/cmd/grants_add.go Outdated Show resolved Hide resolved
internal/cmd/grants_add.go Outdated Show resolved Hide resolved
internal/cmd/grants_add.go Show resolved Hide resolved
internal/cmd/grants_add.go Outdated Show resolved Hide resolved
internal/cmd/grants_add.go Outdated Show resolved Hide resolved
@jmorganca jmorganca changed the title fix(cli): updates 'grants add' command for launch fix(cli): updates 'grants add' command Apr 6, 2022
},
}
cmd.Flags().StringP("destination", "d", "", "[required] Name of destination that identity be given access to")
cmd.Flags().BoolP("group", "g", false, "Marks identity as type 'group'")
Copy link
Contributor

@jmorganca jmorganca Apr 6, 2022

Choose a reason for hiding this comment

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

Let's remove the -g and -i flags and just look up 1/ identity and 2/ group by name and select the first one

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not a fan of the ambiguity here, it opens up a lot of possibility for unexpected behavior.

If there is overlap between an identity name and group name you could accidentally grant or revoke access to the wrong subject.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we wanted them to be safely interchangeable we would probably need a unique constraint on the database to prevent adding an identity and a group with the same name.

internal/cmd/grants_add.go Outdated Show resolved Hide resolved
internal/cmd/grants_add.go Outdated Show resolved Hide resolved
Co-authored-by: Daniel Nephin <dnephin@gmail.com>
kimskimchi and others added 4 commits April 7, 2022 02:50
Co-authored-by: Bruce MacDonald <brucewmacdonald@gmail.com>
Co-authored-by: Bruce MacDonald <brucewmacdonald@gmail.com>
Copy link
Collaborator

@BruceMacD BruceMacD left a comment

Choose a reason for hiding this comment

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

Looking really good, my main concern at the moment is the "group" identity type that opens up the door for adding groups into db tables where they shouldn't be.

internal/cmd/errors.go Outdated Show resolved Hide resolved
$ infra grants add johndoe@acme.com -d kubernetes.production.default

# Grant identity access to Infra
$ infra grants add johndoe@acme.com -d infra
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to see more examples with roles personally, not something that blocks this change though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

role is optional, it will default to 'connect' so users who don't care about fine-grained roles have to specify them

internal/server/models/identity.go Outdated Show resolved Hide resolved
@kimskimchi kimskimchi merged commit bca65bf into main Apr 8, 2022
@kimskimchi kimskimchi deleted the add-grant branch April 8, 2022 05:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants