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

feat: support basic-auth for loading scripts #1516

Merged
merged 2 commits into from
Dec 21, 2022

Conversation

micmeyer
Copy link
Contributor

Adds support for basic-auth when loading a script.

Usage:
export JBANG_BASIC_AUTH_USERNAME=
export JBANG_BASIC_AUTH_PASSWORD=
jbang https:///.....

@maxandersen
Copy link
Collaborator

hmm - I understand the need but would it make more sense we used maven style config for this ? basically use auth settings in /.m2/pom.xml ?

@micmeyer
Copy link
Contributor Author

Being able to store credentials in a file would definitely be nice. From a security point of view the credentials would have to be encrypted tough.

Maven and Ansible use a "master password" for this:

With the "tooling" that is needed for users to encrypt individual passworts (Maven) or a whole file (Ansible) this would become quite a big feature.

I liked the solution that was chosen for the GITHUB_TOKEN (#590) since one doesn't have to store unencrypted passwords in a file.

@micmeyer
Copy link
Contributor Author

@maxandersen I would really like to see support for Basic-Auth in JBang. This would allow us to have branch-specific scrips for setting up a workspace.
Any chance that this pull request will be merged? Or are you planning to add this functionality in combination with a bigger refactoring that supports encrypted configuration properties/files?

@maxandersen
Copy link
Collaborator

I want something like this in too but just considering how. I know that the single global env variable won't work in the long run though. Thus im being hesitant to just add it straight up.

@quintesse
Copy link
Contributor

@maxandersen I do feel the same way as you , but after having thought about it I'm not coming up with alternatives that are much better, I also imagine that we won't support many more different authentication schemes so we'd probably have something like BEARER and TOKEN and that's it, right? It also seems that most services like GitHub or anything container based tend to depend on passing secrets using environment variables. Given all that I'm leaning towards applying this.

NB: The only thing I would like to see changed is the naming of the variables: JBANG_AUTH_BASIC_USERNAME and JBANG_AUTH_BASIC_PASSWORD, so future extensions would be likewise named, eg: JBANG_AUTH_BEARER, JBANG_AUTH_TOKEN, etc.

@codecov
Copy link

codecov bot commented Nov 21, 2022

Codecov Report

Base: 0.00% // Head: 0.00% // No change to project coverage 👍

Coverage data is based on head (b739775) compared to base (35d2038).
Patch coverage: 0.00% of modified lines in pull request are covered.

❗ Current head b739775 differs from pull request most recent head 7a82b94. Consider uploading reports for the commit 7a82b94 to get more accurate results

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1516   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files        110     110           
  Lines       6977    6985    +8     
  Branches    1141    1142    +1     
=====================================
- Misses      6977    6985    +8     
Flag Coverage Δ
Linux 0.00% <0.00%> (ø)
Windows 0.00% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/main/java/dev/jbang/util/Util.java 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@micmeyer
Copy link
Contributor Author

Naming updated according to the feedback of @quintesse .

Copy link
Contributor

@quintesse quintesse left a comment

Choose a reason for hiding this comment

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

LGTM

@maxandersen
Copy link
Collaborator

merging with the understanding we'll want to broaden this in future. we'll treat the JBANG_AUTH as fallback values on hosts without specific setting in future.

@maxandersen maxandersen merged commit 019d732 into jbangdev:main Dec 21, 2022
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.

3 participants