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(auth): adding userpass and ldap auth #440

Merged
merged 4 commits into from
Mar 31, 2023

Conversation

saipranav
Copy link
Contributor

PR helps in appending /{username} at last after /login in path while calling vault auth API.

Related and fixes #433

Added approle test which can now run as part of not just in basic integration suite instead of enterprise alone

Please review and let me know if I need to code it differently since I am not super familiar with nodejs.

Let me know If you need me to add changelog, it had pull request ids so thought you might add at later stage

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 15, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@robmonte robmonte left a comment

Choose a reason for hiding this comment

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

Thanks so much for this contribution! I've been testing this out myself and everything seems to work well.

I left review comments on the code and also have a couple callouts below:

The action.yml file also needs to be updated to list the new inputs for username and password. Here's my recommended descriptions to start with, and I can ask others on my team to provide feedback on them:

  • Username: 'The username of the user to log in to Vault as. Available to both Userpass and LDAP auth methods'
  • Password: 'The password of the user to log in to Vault as. Available to both Userpass and LDAP auth methods'

 

What prompted these changes to index.js? I'll have to ask someone to help review these changes; I simply don't have enough JS experience for that one.

integrationTests/basic/userpass_auth.test.js Outdated Show resolved Hide resolved
src/auth.js Show resolved Hide resolved
@saipranav
Copy link
Contributor Author

Thanks for the review.

I forgot the action.yml part, added it now.

I removed changes for the index.js inside dist/ after seeing this comment in recent pull request #208 (review)

Copy link
Member

@robmonte robmonte left a comment

Choose a reason for hiding this comment

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

Nice find about the index.js changes. Everything seems great. I'm just asking for a quick second opinion on the description text before merging 👍

@robmonte robmonte merged commit 1d767e3 into hashicorp:main Mar 31, 2023
5 checks passed
@saipranav
Copy link
Contributor Author

Thanks, Robert.

saipranav added a commit to saipranav/vault-action that referenced this pull request Apr 11, 2023
saipranav added a commit to saipranav/vault-action that referenced this pull request Apr 11, 2023
@sladebaumann
Copy link

sladebaumann commented May 11, 2023

This will be a super helpful feature to have! Is there any timeline for another release that contains this change? I see this has been done for a while - I'm just not sure what the release cadence is like for this Action.

wagnerm added a commit to wagnerm/vault-action that referenced this pull request May 17, 2023
* main:
  Add userpass auth and ldap auth support (hashicorp#440)
  chore(deps-dev): bump jest from 29.4.3 to 29.5.0 (hashicorp#438)
maxcoulombe pushed a commit that referenced this pull request May 19, 2023
@robmonte
Copy link
Member

robmonte commented Jun 7, 2023

Hi @sladebaumann - Sorry about the wait time, but I wanted to let you know that v2.6.0 went out today with this feature 😄

@sladebaumann
Copy link

Thank you so much for letting me know! No worries at all on the wait time, I know how it goes :)

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.

[FEAT] Support for LDAP auth method
4 participants