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

feat: add support for setting password and org for a new user in the cli #15981

Merged
merged 2 commits into from
Nov 20, 2019

Conversation

jsteenb2
Copy link
Contributor

@jsteenb2 jsteenb2 commented Nov 19, 2019

one thing to note here is that new endpoint was created. there was no
endpoint for setting an initial password that worked. The existing endpoint
was a bit messy and coupled across multiple routes. Having multiple auth
schemes proved incredibly taxing to write against.

update: this came up in discussion in another topic with @goller and from that conversation decided to fixup the password service interface here. To provide the user id instead of name as the first arg to each set/compare func in the PasswordService.

closes: #15977

@jsteenb2 jsteenb2 requested review from a team and desa November 19, 2019 20:27
@jsteenb2 jsteenb2 force-pushed the 15897/pkger_authentication branch 5 times, most recently from 80dd3e5 to c8e504c Compare November 19, 2019 20:44
@@ -17,21 +17,27 @@ import (
"github.com/spf13/viper"
)

var influxCmd = &cobra.Command{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

want to make this noted, the init and global state are 100% not needed. Only adds side effects and indirection in this case. Creating constructors allows the ability to provide DI for testing (see pkg_test.go)

Copy link
Contributor

@kelwang kelwang left a comment

Choose a reason for hiding this comment

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

I didn't see any authorizer wrapped post password, can you wrap it into an authorizer, and add some tests in http package as well?

Copy link
Contributor

@dearyhud dearyhud left a comment

Choose a reason for hiding this comment

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

lgtm


// ComparePassword compares the user new password with existing. Note: is not implemented.
func (s *PasswordService) ComparePassword(ctx context.Context, name string, password string) error {
panic("not implemented")
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -72,6 +73,10 @@ func NewUserHandler(b *UserBackend) *UserHandler {
h.HandlerFunc("GET", usersLogPath, h.handleGetUserLog)
h.HandlerFunc("PATCH", usersIDPath, h.handlePatchUser)
h.HandlerFunc("DELETE", usersIDPath, h.handleDeleteUser)
// the POST doesn't need to be nested under users in this scheme
// seems worthwhile to make this a root resource in our HTTP API
Copy link
Contributor

Choose a reason for hiding this comment

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

I can agree w/ this, seems kind of odd to have this nested under the /users/:id path when id isn't used

@jsteenb2 jsteenb2 force-pushed the 15897/pkger_authentication branch 6 times, most recently from ee0278b to 81c39d7 Compare November 20, 2019 14:40
Copy link
Contributor

@desa desa left a comment

Choose a reason for hiding this comment

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

LGTM

one thing to note here is that new endpoint was created. there was no
endpoint for setting an initial password that worked. The existin endpoint
was a bit messy and coupled across multiple routes. Having multiple auth
schemes proved incredibly taxing to write against.
@jsteenb2
Copy link
Contributor Author

jsteenb2 commented Nov 20, 2019

tests failing in idpe here, will have follow up PR in idpe to touch that up. This is an expected failure:

# github.com/influxdata/idpe/kv_test [github.com/influxdata/idpe/kv.test]
kv/passwords_test.go:57:44: cannot use f.Users[i].Name (type string) as type influxdb.ID in argument to svc.SetPassword
kv/passwords_test.go:310:54: cannot use tt.args.name (type string) as type influxdb.ID in argument to s.SetPassword
kv/passwords_test.go:485:58: cannot use tt.args.name (type string) as type influxdb.ID in argument to s.ComparePassword
+ record_failure 'go vet failed'

@jsteenb2 jsteenb2 merged commit dc2d931 into master Nov 20, 2019
@jsteenb2 jsteenb2 deleted the 15897/pkger_authentication branch November 20, 2019 17:16
bednar added a commit to influxdata/influxdb-client-csharp that referenced this pull request Nov 22, 2019
bednar added a commit to influxdata/influxdb-client-java that referenced this pull request Nov 22, 2019
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.

Creating user via cli without org renders UI unusable
4 participants