Skip to content
This repository was archived by the owner on Oct 6, 2019. It is now read-only.

Adding okta support for auth logins #146

Merged
merged 2 commits into from
Aug 23, 2017
Merged

Conversation

Mike-Dunton
Copy link
Contributor

@Mike-Dunton Mike-Dunton commented Aug 23, 2017

This adds an "okta" option to the frontend and handles passing users credentials to auth/okta/login/

Starting to see some code reuse in login.go. What are your thoughts?

Also could you point to something explaining how the tests work? Are they integration tests? Are you mocking the auth backends?

Copy link
Owner

@Caiyeon Caiyeon left a comment

Choose a reason for hiding this comment

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

Line 400 in index.vue should also report back user's client token after logging in.

Has this been tested?

@@ -112,6 +113,26 @@
</div>
</div>

<!-- Userpass login form -->
Copy link
Owner

Choose a reason for hiding this comment

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

Should be Okta instead of Userpass

@Caiyeon
Copy link
Owner

Caiyeon commented Aug 23, 2017

Yes, the backend login function should be just a simple refactor. I'll get to it in a couple of days unless you want to tackle it

The unit tests are definitely more integration than unit. A dev vault core is spun up in the backend before the tests, and provides a real vault to interact with. This vault dev core is version controlled via vendor, and should be locked to v0.7.3 at the moment.

There are no github or okta auth unit tests, simply because I haven't found the time to mess around with secrets (github in particular) on continuous integration. If you have an elegant way to do it, let me know

@Mike-Dunton
Copy link
Contributor Author

Mike-Dunton commented Aug 23, 2017

I built this and deployed it with a vault backend I maintain. I was able to authenticate with the Okta backend. It returned a token and set the session in goldfish. The only thing It didn't do was do the notify since I did not have 'Okta' in the if line. I updated that.

@Caiyeon Caiyeon merged commit 197a3eb into Caiyeon:master Aug 23, 2017
@Caiyeon
Copy link
Owner

Caiyeon commented Aug 23, 2017

That's good enough for me. I'll merge this and you should see it in the 0.7.0 release

Caiyeon added a commit that referenced this pull request Aug 24, 2017
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