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

Add support for custom keyring names on linux #47

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JojiiOfficial
Copy link

@JojiiOfficial JojiiOfficial commented Apr 15, 2020

I've implemented a way to use custom keyring names (on linux).
To let it compile on another OS than linux, every keyring_<os>.go file needs to implement the secretServiceProvider to allow a typecast in the keyring.go. This is required since there must be a way to set the keyringName in the keyring_linux.go from keyring.go to pass the desired keyring name.

I've successfully cross compiled a test application for every os

A test function for each function from the keyring interface is available as well.
To use it a customKeyring object must be created using keyring.NewCustomKeyring("name").
Fixes #46

@szuecs
Copy link
Member

szuecs commented Apr 15, 2020

Thanks for the pr, 2 comments from my side.

func (s secretServiceProvider) Set(service, user, password string) error { return nil }

// Get password from keyring given service and user name.
func (s secretServiceProvider) Get(service, user string) (string, error) { return "", nil }
Copy link
Member

Choose a reason for hiding this comment

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

Should not this return DefaultKeyringName?

Copy link
Author

Choose a reason for hiding this comment

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

Those functions aren't getting called at all. Those are just for the compiler to not throw an error. Otherwise on any other GOOS than linux the compiler would say that there is no such secretServiceProvider to typecast to and it wouldn't compile.
Same in keyring_darwin.go

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

For me it looks like you should refactor this part, such that the empty string check should be dropped and the default should be returned by the new functions created.

@mikkeloscar Wdyt?

func (s secretServiceProvider) Set(service, user, password string) error { return nil }

// Get password from keyring given service and user name.
func (s secretServiceProvider) Get(service, user string) (string, error) { return "", nil }
Copy link
Member

Choose a reason for hiding this comment

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

Should not this return DefaultKeyringName?

}

// NewSecretService inializes a new SecretService object.
func NewSecretService() (*SecretService, error) {
func NewSecretService(keyringName string) (*SecretService, error) {
Copy link
Member

Choose a reason for hiding this comment

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

A breaking change in libraries is not a great thing to do.
Please let the signature like this and add another func:

func WithKeyringName(name string) (*SecretService, error) {
    s,err := NewSecretService()
    if err != nil { return nil, err }
    s.KeyringName = name
    return s, nil
}

Copy link
Member

@szuecs szuecs left a comment

Choose a reason for hiding this comment

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

There are a couple of comments that were not addressed and I just added one other.

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.

Use custom keyring name
2 participants