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

Allow credentials to be configured via an environment variable #70

Merged
merged 3 commits into from
May 24, 2021

Conversation

samuong
Copy link
Owner

@samuong samuong commented May 24, 2021

This allows non-interactive use of Alpaca on Linux and Windows.

camh-
camh- previously approved these changes May 24, 2021
Copy link
Collaborator

@camh- camh- left a comment

Choose a reason for hiding this comment

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

Pretty much LGTM, Just some small comments. Approving in case you don't agree with those comments.

authenticator_test.go Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
credentials.go Outdated
at := strings.IndexRune(e.value, '@')
colon := strings.IndexRune(e.value, ':')
if at == -1 || colon == -1 || at > colon {
return nil, errors.New("invalid $NTLM_CREDENTIALS, please run `alpaca -H`")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Technically, this bit of code has no idea about envvars or specifically the NTLM_CREDENTIALS var. It really is just a string source, which could also be used later when implementing a config file.

Also I wonder whether the parsing of user@domain:hash belongs in authenticator, much as I suggested elsewhere that the formatting of the three fields perhaps belongs in a String() function on authenticator. At the moment, knowledge of this format is split between here and main().

Copy link
Collaborator

@camh- camh- left a comment

Choose a reason for hiding this comment

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

LGTM

@samuong samuong merged commit 3c58e5f into master May 24, 2021
@samuong samuong deleted the config branch May 24, 2021 22:48
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.

None yet

3 participants