Skip to content
This repository was archived by the owner on Mar 8, 2022. It is now read-only.

feat: basic auth credentials from netrc#191

Open
stuart-warren wants to merge 1 commit into
fishworks:mainfrom
stuart-warren:auth
Open

feat: basic auth credentials from netrc#191
stuart-warren wants to merge 1 commit into
fishworks:mainfrom
stuart-warren:auth

Conversation

@stuart-warren

@stuart-warren stuart-warren commented Oct 8, 2021

Copy link
Copy Markdown

add basic auth credentials to request for food packages if present in users
netrc file

add 'Accept: application/octet-stream' header to permit working through github
api

ref: #190

@bacongobbler

bacongobbler commented Oct 15, 2021

Copy link
Copy Markdown
Contributor

Forgive my naivety here. I've never worked with netrc files before.

Are these file paths configurable? A lot of people (myself included) prefer to keep configuration under $XDG_DATA_HOME. For example all of my neovim configuration lives under $HOME/.config/nvim, and all of my bash config is under $HOME/.config/bash. Does netrc follow this convention as well?

https://wiki.archlinux.org/title/XDG_Base_Directory

Comment thread food.go Outdated
func userNetrcCredentials(host string) (login, password string) {
var netrcMachine *netrc.Machine
for _, netrcFilePath := range []string{home.GPGNetrc(), home.Netrc()} {
if exists, _ := osutil.Exists(netrcFilePath); !exists {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we need to check for existence before parsing the file? Shouldn't netrc.Parse throw os.IsNotExist(err) in that case?

https://github.com/jdxcode/netrc/blob/926c7f70242abe00179235c2b06bb647c0c53a12/netrc.go#L41-L44

Seems like a redundant check.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

👍

Comment thread food.go Outdated
if exists, _ := osutil.Exists(netrcFilePath); !exists {
continue
}
netrcFile, err := netrc.Parse(netrcFilePath)

@bacongobbler bacongobbler Oct 15, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

with the previous comment in mind... This can be optimized further:

for _, netrcFilePath := range []string{home.GPGNetrc(), home.Netrc()} {
    if netrcFile, err := netrc.Parse(netrcFilePath); err != nil {
        if !os.IsNotExist(err) {
            // we probably want to report an error here; we only care about if the file wasn't found - anything else is indeterministic behavior
        }
    } else {
        netrcMachine = netrcFile.Machine(host)
	return netrcMachine.Get("login"), netrcMachine.Get("password")
    }
}
return "", ""

Also, how should we handle the case where both .netrc.gpg and .netrc are present on the user's machine? Right now it appears this logic will only read .netrc.gpg and ignore the latter. What if the user wants the program to read from .netrc?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I was copying what git-credential-netrc was doing

If the user has $NETRC set, it could use that first instead

Comment thread food.go Outdated
// addCredentialsToRequest will attempt to attach relevant credentials to the http.Request
func addCredentialsToRequest(req *http.Request) {
login, password := userNetrcCredentials(req.Host)
if password != "" {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What about user accounts with no password? Shouldn't we check both username and password?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

👍

@stuart-warren

Copy link
Copy Markdown
Author

Forgive my naivety here. I've never worked with netrc files before.

Are these file paths configurable? A lot of people (myself included) prefer to keep configuration under $XDG_DATA_HOME. For example all of my neovim configuration lives under $HOME/.config/nvim, and all of my bash config is under $HOME/.config/bash. Does netrc follow this convention as well?

https://wiki.archlinux.org/title/XDG_Base_Directory

.netrc tends to be in the $HOME directory https://linux.die.net/man/1/ftp

A number of systems support reading it from $NETRC

I can add that?

add basic auth credentials to request for food packages if present in users
netrc file

add 'Accept: application/octet-stream' header to permit working through [github
api](https://docs.github.com/en/rest/reference/repos#get-a-release-asset)

ref: fishworks#190
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants