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

Auth list providers #1187

Merged
merged 22 commits into from
Mar 29, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
393a290
first pass at implementing auth list-providers
br-lewis Mar 21, 2018
c8f7ada
pull the functionality to make the request out into its own function
br-lewis Mar 21, 2018
db3c89f
Merge branch 'master' into auth-list-providers
br-lewis Mar 26, 2018
6263db2
Add verbose auth type descriptions and change the table output to use
br-lewis Mar 27, 2018
a4097d8
Change authtype check to a switch case block
br-lewis Mar 28, 2018
e7c945d
ran it through gofmt and goimport to sort the imports
br-lewis Mar 28, 2018
8efad16
Change auth to login because it's more accurate
br-lewis Mar 28, 2018
3bdeaad
swap to using shorter variable declaration syntax
br-lewis Mar 28, 2018
c3b0ea6
Sort imports for dcos.go
br-lewis Mar 28, 2018
f61e196
Change the command to use RunE and return an error instead
br-lewis Mar 28, 2018
aa74c1a
Make comments proper sentences and update some copypasted comment to be
br-lewis Mar 28, 2018
fa4be9d
Remove dcosConfig and instead grab it when needed in authListProviders
br-lewis Mar 28, 2018
e5fd923
Make the cluster config an argument of getProviders in preparation for
br-lewis Mar 28, 2018
69b41ad
Merge branch 'master' into auth-list-providers
br-lewis Mar 28, 2018
25c162a
Merge in master to allow the httpclient to take a baseurl instead of
br-lewis Mar 28, 2018
5e35527
Remove unnecessary error check
br-lewis Mar 28, 2018
21c8290
Fixed variable with Url in name to URL
br-lewis Mar 28, 2018
cb2df28
change url selection so that the config isn't parsed unless no url was
br-lewis Mar 29, 2018
bd9084b
Put import back with other github.com imports
br-lewis Mar 29, 2018
889bed6
Make each switch branch return instead of assigning to a string
br-lewis Mar 29, 2018
56fd31a
Change loginTypeDescription to output the given loginType if it wasn't
br-lewis Mar 29, 2018
4224085
Follow linter's advice and swap errors.New(fmt.Sprintf(...)) with
br-lewis Mar 29, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion Gopkg.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions Gopkg.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,7 @@
[[constraint]]
name = "github.com/spf13/afero"
version = "1.0.2"

[[constraint]]
branch = "master"
name = "github.com/olekukonko/tablewriter"
25 changes: 24 additions & 1 deletion pkg/cmd/dcos.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,20 @@ import (

"github.com/dcos/dcos-cli/pkg/config"
"github.com/spf13/cobra"
"io"
Copy link
Contributor

Choose a reason for hiding this comment

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

this should go in the previous import section (https://github.com/golang/go/wiki/CodeReviewComments#imports).

)

// rootCmd represents the base command when called without any subcommands.
var rootCmd = &cobra.Command{
Use: "dcos",
}

var dcosConfig *Cluster
Copy link
Contributor

Choose a reason for hiding this comment

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

we should avoid global variables in general, I know there are already some but this will be refactored. How about calling attachedCluster() at the beginning of the list-provider command into a local variable?


// Execute adds all child commands to the root command and sets flags appropriately.
// This is called by main.main(). It only needs to happen once to the rootCmd.
func Execute() {
dcosConfig = attachedCluster()
if err := rootCmd.Execute(); err != nil {
fmt.Println(err)
os.Exit(1)
Expand Down Expand Up @@ -46,8 +50,27 @@ func attachedCluster() *Cluster {
os.Exit(1)
}

// Default to the old config path. If .dcos/clusters exists, it will search that for an attached cluster and if it
// finds one there, it will override this var to use that cluster's config instead.
configPath := filepath.Join(usr.HomeDir, ".dcos")

clustersDir := filepath.Join(configPath, "clusters")

// Find the config of the attached cluster.
err = filepath.Walk(clustersDir, func(path string, info os.FileInfo, err error) error {
if filepath.Base(path) == "attached" {
configPath = filepath.Dir(path)
return io.EOF
}
return nil
})
if err != io.EOF && err != filepath.SkipDir && err != nil {
fmt.Println(err)
}


// Make sure "~/.dcos/dcos.toml" exists.
path := filepath.Join(dir, "dcos.toml")
path := filepath.Join(configPath, "dcos.toml")
f, err := os.OpenFile(path, os.O_RDONLY|os.O_CREATE, 0600)
if err != nil {
fmt.Printf("Couldn't create config file \"%s\": %s.\n", path, err)
Expand Down
120 changes: 120 additions & 0 deletions pkg/cmd/dcos_auth_listproviders.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
package cmd

import (
"encoding/json"
"errors"
"fmt"
"github.com/dcos/dcos-cli/pkg/httpclient"
"github.com/olekukonko/tablewriter"
"github.com/spf13/cobra"
"os"
Copy link
Contributor

Choose a reason for hiding this comment

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

imports should be sorted as mentioned before.

)

// These are the different auth types that DC/OS supports with the names that they'll be given from the providers
// endpoint.
const (
AuthTypeDCOSUidPassword = "dcos-uid-password"
AuthTypeDCOSUidServiceKey = "dcos-uid-servicekey"
AuthTypeDCOSUidPasswordLDAP = "dcos-uid-password-ldap"
AuthTypeSAMLSpInitiated = "saml-sp-initiated"
AuthTypeOIDCAuthCodeFlow = "oidc-authorization-code-flow"
AuthTypeOIDCImplicitFlow = "oidc-implicit-flow"
Copy link
Contributor

Choose a reason for hiding this comment

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

JP suggests using the "login provider" semantic here, as authentication is only about sending the ACS token to DC/OS. We still have quite some docs using "auth provider" wording, but I'd rather use the wording recommended by the security team.

)

var jsonOutput bool
Copy link
Contributor

Choose a reason for hiding this comment

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

This would require having constructor-based commands rather than global variables like authListProviders, but once this is done that'd be nice to have structs for options. Similar to the pattern here : https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/auth/cani.go#L39-L53

(I'm leaving this comment for reference not necessarily to tackle in this PR).


// authCmd represents the `dcos auth` subcommand.
var authListProvidersCmd = &cobra.Command{
Use: "list-providers",
Copy link
Contributor

Choose a reason for hiding this comment

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

dcos auth list-providers accepts an optional parameter, which refers to the cluster URL to get providers from instead of using the attached cluster's URL.

Run: listProviders,
}

func init() {
authCmd.AddCommand(authListProvidersCmd)
authListProvidersCmd.Flags().BoolVar(&jsonOutput, "json", false,
"returns providers in json format")
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no line-length limit in Go, we use ~110 characters as a rule of thumb, a single line is fine here.

}

func listProviders(cmd *cobra.Command, args []string) {
providers, err := getProviders()
if err != nil {
fmt.Println(err)
os.Exit(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of printing and calling exit like here or below, other commands define the RunE func field and it can return an error.

}

if jsonOutput {
// re-marshal it into json with indents added in for pretty printing
Copy link
Contributor

Choose a reason for hiding this comment

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

We use sentences starting with uppercase and ending with a dot. The cmd package was a quick proof of concept but we started doing this in other packages.

out, err := json.MarshalIndent(providers, "", "\t")
if err != nil {
fmt.Println(err)
os.Exit(1)
}
fmt.Println(string(out))
} else {
table := tablewriter.NewWriter(os.Stdout)
table.SetHeader([]string{"PROVIDER ID", "AUTHENTICATION TYPE"})
// turn off wrapping because it seems to wrap even if the column is set to be wide enough
table.SetAutoWrapText(false)
table.SetBorder(false)
table.SetRowSeparator(" ")
table.SetColumnSeparator(" ")
table.SetCenterSeparator(" ")

for name, provider := range *providers {
desc, err := authTypeDescription(provider.AuthenticationType, provider)
if err != nil {
fmt.Printf("Unknown authentication type %s", provider.AuthenticationType)
os.Exit(1)
}
table.Append([]string{name, desc})
}
table.Render()
}
}

func getProviders() (*map[string]authProvider, error) {
var config = dcosConfig.Config
var client = httpclient.New(config)
Copy link
Contributor

Choose a reason for hiding this comment

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

httpclient has been changed recently: 3560d63

var response, err = client.Get("/acs/api/v1/auth/providers")
Copy link
Contributor

Choose a reason for hiding this comment

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

the short variable declaration is more common in go, that's what we currently use, eg. response, err := client.Get("/acs/api/v1/auth/providers")

if err != nil {
return nil, err
}
defer response.Body.Close()

var resp map[string]authProvider
err = json.NewDecoder(response.Body).Decode(&resp)
if err != nil {
return nil, err
}

return &resp, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, this is enough as callers must check for the error and not for the pointer to be nil :

err = json.NewDecoder(response.Body).Decode(&resp)
return &resp, err

}

func authTypeDescription(authType string, provider authProvider) (string, error) {
var desc string
if authType == AuthTypeDCOSUidPassword {
desc = "Log in using a standard DC/OS user account (username and password)"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: returning directly from switch cases removes the need for the lines surrounding the switch block.

} else if authType == AuthTypeDCOSUidServiceKey {
desc = "Log in using a DC/OS service user account (username and private key)"
} else if authType == AuthTypeDCOSUidPasswordLDAP {
desc = "Log in in using an LDAP user account (username and password)"
} else if authType == AuthTypeSAMLSpInitiated {
desc = fmt.Sprintf("Log in using SAML 2.0 (%s)", provider.Description)
} else if authType == AuthTypeOIDCAuthCodeFlow || authType == AuthTypeOIDCImplicitFlow {
desc = fmt.Sprintf("Log in using OpenID Connect(%s)", provider.Description)
} else {
return "", errors.New("unknown authentication type")
Copy link
Contributor

Choose a reason for hiding this comment

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

A switch statement would be more readable here.

}
return desc, nil
}

type authProvider struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

As commented above : LoginProvider, this would also make sense to create an auth package as this would be used in other places (eg. cluster setup).

AuthenticationType string `json:"authentication-type"`
ClientMethod string `json:"client-method"`
Config authListProviderConfig `json:"config"`
Description string `json:"description"`
}

type authListProviderConfig struct {
StartFlowUrl string `json:"start_flow_url"`
}
8 changes: 8 additions & 0 deletions vendor/github.com/mattn/go-runewidth/.travis.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

21 changes: 21 additions & 0 deletions vendor/github.com/mattn/go-runewidth/LICENSE

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

27 changes: 27 additions & 0 deletions vendor/github.com/mattn/go-runewidth/README.mkd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading