Skip to content
This repository was archived by the owner on Aug 16, 2024. It is now read-only.

WIP: GitHub Auth First Cut #142

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

WIP: GitHub Auth First Cut #142

wants to merge 13 commits into from

Conversation

jrwhite17
Copy link
Contributor

Please give any feedback on this hacking of omni-auth/devise and GitHub auth.

GitHub tokens need to be added to config/initializers/devise.rb -- line 289.

I am by no means a Rails Jedi, so please give any constructive feedback. I'm sure I have created some bugs or overlooked a few things. Maybe we can hash out the changes over a paired programming session?

Thanks!

@rbclark rbclark changed the title GitHub Auth First Cut WIP: GitHub Auth First Cut Jun 24, 2020
Copy link

@rbclark rbclark left a comment

Choose a reason for hiding this comment

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

Can you verify that you have made sure of the following?

  1. it doesn't break local login
  2. It doesn't break an existing LDAP login
  3. There's a reasonable migration path for existing LDAP users

@@ -286,7 +286,8 @@
# ==> OmniAuth
# Add a new OmniAuth provider. Check the wiki for more information on setting
# up on your models and hooks.
# config.omniauth :github, 'APP_ID', 'APP_SECRET', scope: 'user,public_repo'
config.omniauth :github, 'N/A', 'N/A', scope: 'user:email',
Copy link

Choose a reason for hiding this comment

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

Requiring users to set these directly in the initializer is going to cause problems, this should be configurable via ENV or some other 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.

Good point. I'll set that up. Should I build out a settings file similar to Vulcan?

Copy link

@rbclark rbclark Jun 24, 2020

Choose a reason for hiding this comment

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

That would be nice to have, we would need to then figure out a migration path for the existing LDAP configuration though: https://github.com/mitre/heimdall/blob/master/config/ldap.example.yml. We could consider adding a rake task which generates a heimdall.yml from a users existing ldap.yml if it exists.

@@ -1,32 +1,67 @@
class CreateUsers < ActiveRecord::Migration[5.2]
Copy link

Choose a reason for hiding this comment

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

The updated schema.rb should be included in the PR.

</div>
<!-- /.login-box-body -->

<!-- GitHub -->
Copy link

Choose a reason for hiding this comment

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

This should be hidden if the user has not enabled GitHub login. Also there is no need to show this on the registration page, it only needs to show on the new sessions page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll swap that over and setup something to hide the GitHub login if it's not enabled.

@aaronlippold
Copy link
Member

@rbclark can we add deploy-preview on Heroku for this

@aaronlippold aaronlippold temporarily deployed to mitre-heimdall-pr-142 July 3, 2020 18:33 Inactive
Copy link

@rbclark rbclark left a comment

Choose a reason for hiding this comment

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

It looks as though this pulls in webpacker (which is a replacement for sprockets) but still leaves most of the assets in sprockets. If we are going to use webpacker can we move over everything to use it?

https://blog.carbonfive.com/migrating-from-sprockets-to-webpacker/

@@ -33,6 +33,8 @@ default: &default
- .woff2

extensions:
- .vue
- vue
Copy link

Choose a reason for hiding this comment

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

The vue extension without a . should be unnecessary.

@aaronlippold aaronlippold requested a review from a team November 12, 2020 02:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants