Skip to content
This repository has been archived by the owner on Mar 27, 2019. It is now read-only.

Add LDAP login option #16

Merged
merged 4 commits into from
Dec 1, 2016
Merged

Add LDAP login option #16

merged 4 commits into from
Dec 1, 2016

Conversation

joraff
Copy link
Contributor

@joraff joraff commented Nov 29, 2016

Adds LDAP as a login method option. Finishes username & password authentication, which LDAP shares some functions with. Minor refactoring.

All auth methods return the same token payload, more or less. Let them
all share how the access token is set.
@joraff
Copy link
Contributor Author

joraff commented Nov 29, 2016

Addresses #15

@Lucretius
Copy link
Collaborator

Nice - will take a closer look at this when we get the chance in the next few days. Thanks for contributing!

@joraff
Copy link
Contributor Author

joraff commented Nov 30, 2016

Sweet! Thanks. I showed this to my team today and we voted to adopt. I can be a regular contributor if you're open to it.

@djenriquez
Copy link
Owner

Most definitely @joraff!! This LGTM. Currently at re:Invent, I'll do some deeper checks later tonight and get it merged in.

@@ -70,12 +70,11 @@ export default class Login extends React.Component {
"Creds": {
"Type": this.state.loginMethodType,
"Username": this.state.username,
"password": this.state.password
"Password": this.state.password
}
})
.then((resp) => {
Copy link
Owner

Choose a reason for hiding this comment

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

We need to add an LDAP login methodtype to the login API here: https://github.com/djenriquez/vault-ui/blob/master/src/login.js#L29 because LDAP uses a different endpoint for authentication. Without this change the API will return a 400: "Invalid auth method".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See next commit :)

@@ -153,6 +125,24 @@ export default class Login extends React.Component {
}
}

setAccessToken(resp) {
Copy link
Owner

Choose a reason for hiding this comment

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

Nice 👍

};
break;
case 'ldap':
endpoint = `/v1/auth/ldap/login/${username}`;
Copy link
Owner

Choose a reason for hiding this comment

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

Ah there it is 😜

@djenriquez
Copy link
Owner

Thanks @joraff!

@djenriquez djenriquez merged commit 420f889 into djenriquez:master Dec 1, 2016
@msabramo msabramo mentioned this pull request Dec 4, 2016
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.

3 participants