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.

Sign up for GitHub

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

core: kgw cookie on multi providers #691

Merged
merged 2 commits into from
May 9, 2024
Merged

core: kgw cookie on multi providers #691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8aa3934
core: kgw cookie on multi providers
Yaiba May 1, 2024
70ba0b8
fix getDomain
Yaiba May 9, 2024
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
41 changes: 33 additions & 8 deletions cmd/kwil-cli/cmds/common/authinfo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@ import (
"encoding/json"
"fmt"
"net/http"
"net/url"
"os"
"path/filepath"
"strings"
"time"

"github.com/kwilteam/kwil-db/cmd/kwil-cli/config"
Expand Down Expand Up @@ -77,6 +79,30 @@ func convertToHttpCookie(c cookie) *http.Cookie {
}
}

// getDomain returns the domain of the URL.
func getDomain(target string) (string, error) {
if target == "" {
return "", fmt.Errorf("target is empty")
}

if !(strings.HasPrefix(target, "http://") || strings.HasPrefix(target, "https://")) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For 0.7 or v0.6, this should work, since we require scheme https://docs.kwil.com/docs/kwil-cli/configuration#available-configs.

For v0.8, seems this could break?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you got a pretty handy util function here https://github.com/kwilteam/kwil-db/pull/703/files#diff-7a8b6c70656b28a8bba3d014ad27d1a48efe27f72f184939a4b9d7b5d2313a84R72, I think it will help.

Copy link
Member

Choose a reason for hiding this comment

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

With latest update to that PR, cfg.Provider must be a proper URL now. Another reason to keep that convention I figure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The persistened cfg.Provider could still be without scheme, am I right?

Copy link
Member

Choose a reason for hiding this comment

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

Not any more. It's an error:

$  ./kwil-cli database list
Error: parse url: parse "127.0.0.1:8484": first path segment in URL cannot contain colon
parse url: parse "127.0.0.1:8484": first path segment in URL cannot contain colon
$  cat ~/.kwil-cli/config.json 
{"provider":"127.0.0.1:8484"}

I think we had that same constraint in v0.7?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have some unpleasant constraint, like this :

.build/kwil-cli database list --kwil-provider localhost:8080                                                   
Error: wrap client: chain_info: Get "localhost:8080/api/v1/chain_info": unsupported protocol scheme "localhost"
wrap client: chain_info: Get "localhost:8080/api/v1/chain_info": unsupported protocol scheme "localhost"

It'll get error when create client, that's before getDomain, so I guess we're fine

return "", fmt.Errorf("target missing scheme")
}

parsedTarget, err := url.Parse(target)
Copy link
Member

Choose a reason for hiding this comment

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

One quirk of url.Parse is how it handles a string like localhost:8080. It does not error. It returns a URL with no host and "Opaque":

&url.URL{Scheme:"localhost", Opaque:"8080", User:(*url.Userinfo)(nil), Host:"", Path:"", RawPath:"", OmitHost:false, ForceQuery:false, RawQuery:"", Fragment:"", RawFragment:""}

We'll probably be rejecting or erroring on such a target before we reach getDomain, but it's still probably an unintended result for getDomain:

  • input localhost:8080
  • output localhost://

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah I got screwed every time I use url.Parse 🤦‍♂️

if err != nil {
return "", fmt.Errorf("parse target: %w", err)
}

return parsedTarget.Scheme + "://" + parsedTarget.Host, nil
}

// getCookieIdentifier returns a unique identifier for a cookie, base64 encoded.
func getCookieIdentifier(domain string, userIdentifier []byte) string {
return base64.StdEncoding.EncodeToString(
append([]byte(domain+"_"), userIdentifier...))
}

// PersistedCookies is a set of Gateway Auth cookies that can be saved to a file.
// It maps a base64 user identifier to a cookie, ensuring only one cookie per wallet.
// It uses a custom cookie type that is json serializable.
Expand All @@ -85,7 +111,7 @@ type PersistedCookies map[string]cookie
// LoadPersistedCookie loads a persisted cookie from the auth file.
// It will look up the cookie for the given user identifier.
// If nothing is found, it returns nil, nil.
func LoadPersistedCookie(authFile string, userIdentifier []byte) (*http.Cookie, error) {
func LoadPersistedCookie(authFile string, domain string, userIdentifier []byte) (*http.Cookie, error) {
if _, err := os.Stat(authFile); os.IsNotExist(err) {
return nil, nil
}
Expand All @@ -101,24 +127,23 @@ func LoadPersistedCookie(authFile string, userIdentifier []byte) (*http.Cookie,
return nil, fmt.Errorf("unmarshal kgw auth file: %w", err)
}

b64Identifier := base64.StdEncoding.EncodeToString(userIdentifier)
b64Identifier := getCookieIdentifier(domain, userIdentifier)
cookie := aInfo[b64Identifier]

return convertToHttpCookie(cookie), nil
}

// SaveCookie saves the cookie to auth file.
// It will overwrite the cookie if the address already exists.
func SaveCookie(authFile string, userIdentifier []byte, originCookie *http.Cookie) error {
func SaveCookie(authFile string, domain string, userIdentifier []byte, originCookie *http.Cookie) error {
b64Identifier := getCookieIdentifier(domain, userIdentifier)
cookie := convertToCookie(originCookie)

authInfoBytes, err := utils.ReadOrCreateFile(authFile)
if err != nil {
return fmt.Errorf("read kgw auth file: %w", err)
}

b64Identifier := base64.StdEncoding.EncodeToString(userIdentifier)

var aInfo PersistedCookies
if len(authInfoBytes) == 0 {
aInfo = make(PersistedCookies)
Expand All @@ -144,14 +169,12 @@ func SaveCookie(authFile string, userIdentifier []byte, originCookie *http.Cooki

// DeleteCookie will delete a cookie that exists for a given user identifier.
// If no cookie exists for the user identifier, it will do nothing.
func DeleteCookie(authFile string, userIdentifier []byte) error {
func DeleteCookie(authFile string, domain string, userIdentifier []byte) error {
authInfoBytes, err := utils.ReadOrCreateFile(authFile)
if err != nil {
return fmt.Errorf("read kgw auth file: %w", err)
}

b64Identifier := base64.StdEncoding.EncodeToString(userIdentifier)

var aInfo PersistedCookies
if len(authInfoBytes) == 0 {
aInfo = make(PersistedCookies)
Expand All @@ -161,6 +184,8 @@ func DeleteCookie(authFile string, userIdentifier []byte) error {
return fmt.Errorf("unmarshal kgw auth file: %w", err)
}
}

b64Identifier := getCookieIdentifier(domain, userIdentifier)
delete(aInfo, b64Identifier)

jsonBytes, err := json.MarshalIndent(&aInfo, "", " ")
Expand Down
103 changes: 101 additions & 2 deletions cmd/kwil-cli/cmds/common/authinfo_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,45 @@ import (
"github.com/stretchr/testify/assert"
)

func TestLoadKGWAuthInfo_without_domain(t *testing.T) {
// this test just to show what the old behavior was

ckA := http.Cookie{
Name: "AAA",
Value: "AAA",
Path: "AAA",
Domain: "AAA",
Expires: time.Date(2023, 10, 27, 15, 46, 58, 651387237, time.UTC),
}

ckB := http.Cookie{
Name: "BBB",
Value: "BBB",
Path: "BBB",
Domain: "BBB",
Expires: time.Date(2023, 10, 27, 15, 46, 58, 651387237, time.UTC),
}

var err error
authFile := filepath.Join(t.TempDir(), "auth.json")
domain := ""

// authn on site A
err = SaveCookie(authFile, domain, []byte("0x123"), &ckA)
assert.NoError(t, err)

// authn on site B
err = SaveCookie(authFile, domain, []byte("0x123"), &ckB)
assert.NoError(t, err)

got, err := LoadPersistedCookie(authFile, domain, []byte("0x123"))
assert.NoError(t, err)

// ckA has been overwritten by ckB
assert.NotEqualValues(t, &ckA, got)
assert.EqualValues(t, &ckB, got)
}

func TestLoadKGWAuthInfo(t *testing.T) {
ck := http.Cookie{
Name: "test",
Expand All @@ -27,12 +66,72 @@ func TestLoadKGWAuthInfo(t *testing.T) {

var err error
authFile := filepath.Join(t.TempDir(), "auth.json")
domain := "https://kgw.kwil.com"

err = SaveCookie(authFile, []byte("0x123"), &ck)
err = SaveCookie(authFile, domain, []byte("0x123"), &ck)
assert.NoError(t, err)

got, err := LoadPersistedCookie(authFile, []byte("0x123"))
got, err := LoadPersistedCookie(authFile, domain, []byte("0x123"))
assert.NoError(t, err)

assert.EqualValues(t, &ck, got)
}

func Test_getDomain(t *testing.T) {
type args struct {
target string
}
tests := []struct {
name string
args args
wantErr bool
wantDoamin string
}{
// TODO: Add test cases.
{
name: "empty string",
args: args{
target: "",
},
wantErr: true,
wantDoamin: "",
},
{
name: "localhost with port",
args: args{
target: "http://localhost:8080/api",
},
wantDoamin: "http://localhost:8080",
},
{
name: "https localhost with port",
args: args{
target: "https://localhost:8080/api/",
},
wantDoamin: "https://localhost:8080",
},
{
name: "http example.com",
args: args{
target: "http://example.com/a/b/c",
},
wantDoamin: "http://example.com",
},
{
name: "https example.com",
args: args{
target: "https://example.com/a/b/c",
},
wantDoamin: "https://example.com",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
domain, err := getDomain(tt.args.target)
if tt.wantErr {
assert.Errorf(t, err, "getDomain(%v)", tt.args.target)
}
assert.Equalf(t, tt.wantDoamin, domain, "getDomain(%v)", tt.args.target)
})
}
}
11 changes: 8 additions & 3 deletions cmd/kwil-cli/cmds/common/roundtripper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,14 +98,19 @@ func DialClient(ctx context.Context, cmd *cobra.Command, flags uint8, fn RoundTr
return fn(ctx, client, conf)
}

cookie, err := LoadPersistedCookie(KGWAuthTokenFilePath(), clientConfig.Signer.Identity())
providerDomain, err := getDomain(conf.Provider)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When backport, conf.Provider should be conf.GrpcUrl

if err != nil {
return err
}

cookie, err := LoadPersistedCookie(KGWAuthTokenFilePath(), providerDomain, clientConfig.Signer.Identity())
if err == nil && cookie != nil {
// if setting fails, then don't do fail usage- failure likely means that the client has
// switched providers, and the cookie is no longer valid. The gatewayclient will re-authenticate.
// delete the cookie if it is invalid
err = client.SetAuthCookie(cookie)
if err != nil {
err2 := DeleteCookie(KGWAuthTokenFilePath(), clientConfig.Signer.Identity())
err2 := DeleteCookie(KGWAuthTokenFilePath(), providerDomain, clientConfig.Signer.Identity())
if err2 != nil {
return fmt.Errorf("failed to delete cookie: %w", err2)
}
Expand All @@ -123,7 +128,7 @@ func DialClient(ctx context.Context, cmd *cobra.Command, flags uint8, fn RoundTr
return nil
}

err = SaveCookie(KGWAuthTokenFilePath(), clientConfig.Signer.Identity(), cookie)
err = SaveCookie(KGWAuthTokenFilePath(), providerDomain, clientConfig.Signer.Identity(), cookie)
if err != nil {
return fmt.Errorf("save cookie: %w", err)
}
Expand Down
Loading