[WIP] GitHub Authentication #1595

sbpipb · 2019-10-20T13:41:31Z

Description

Integrates GitHub OmniAuth to allow users to register and login using their Github account.

More Details

Allows users to signin and sign up using Github

refactor views
refactor model methods

Specs

I've rebased this branch to my other PR #1594

Corresponding Issue

#961

Screenshots

Reviewing this pull request? Check out our Code Review Practices guide if you haven't already!

sbpipb · 2019-10-20T13:43:55Z

app/models/user.rb

@@ -94,10 +96,22 @@ def self.find_for_google_oauth2(access_token)
    user
  end

+  # to refactor, could be single oauth method to begin with
+  def self.from_omniauth(auth)
+    where(provider: auth.provider, uid: auth.uid).first_or_create do |user|


I'd like some feedback regarding how we'd want to do this., compared to the Google version, this uses provider and UID, I'm okay to change this to email if that's what's required.

sbpipb · 2019-10-20T13:44:48Z

app/models/user.rb

@@ -94,10 +96,22 @@ def self.find_for_google_oauth2(access_token)
    user
  end

+  # to refactor, could be single oauth method to begin with
+  def self.from_omniauth(auth)
+    where(provider: auth.provider, uid: auth.uid).first_or_create do |user|


I think uid is the correct option here than email

I'm fine with using UID here!

sbpipb · 2019-10-20T13:45:06Z

app/views/devise/shared/_links.erb

@@ -4,8 +4,12 @@
      <%- resource_class.omniauth_providers.each do |provider| %>
        <% if provider.to_s == "google_oauth2" && (current_page?(new_user_session_path) || current_page?(new_user_password_path)) %>
          <%= button_to t('devise.shared.sign_in_google'), omniauth_authorize_path(resource_name, provider), method: :post, class: 'buttonGhostM' %>
+        <% elsif provider.to_s == "github" && (current_page?(new_user_session_path) || current_page?(new_user_password_path)) %>


this part is a bit dirty, needs some refactoring

That would be great!

julianguyen

Looking good so far! Great job 👏

julianguyen · 2019-10-21T22:07:30Z

app/models/user.rb

  def google_access_token
    google_access_token_expired? ? update_access_token : token
  end

+  # TODO: refactor this and use one checker


julianguyen · 2019-10-21T22:10:50Z

app/models/user.rb

@@ -94,10 +96,22 @@ def self.find_for_google_oauth2(access_token)
    user
  end

+  # to refactor, could be single oauth method to begin with
+  def self.from_omniauth(auth)
+    where(provider: auth.provider, uid: auth.uid).first_or_create do |user|


I'm fine with using UID here!

guilhermekbral · 2019-10-23T13:44:10Z

Hey @sbpipb 👋
Looks like the files from #1594 are present in this branch too. It would be better to remove that commit from this branch.
If for any reason #1594 won't be merged, but this PR will, we don't want that commit going to master accidentally! 🙈

sbpipb · 2019-10-25T10:56:04Z

Hey @sbpipb
Looks like the files from #1594 are present in this branch too. It would be better to remove that commit from this branch.
If for any reason #1594 won't be merged, but this PR will, we don't want that commit going to master accidentally!

I understand, I'll remove the docker setup related files when I finish this PR. For now i'll just leave it as is.

sbpipb · 2019-10-26T14:38:09Z

app/views/devise/shared/_links.erb

@@ -4,8 +4,12 @@
      <%- resource_class.omniauth_providers.each do |provider| %>
        <% if provider.to_s == "google_oauth2" && (current_page?(new_user_session_path) || current_page?(new_user_password_path)) %>


TODO:
add a helper that handles showing of button links

show_signup_page? provider

show_register_page? provider

julianguyen · 2019-11-26T19:20:16Z

Just checking in! Any updates?

sbpipb · 2019-12-09T03:42:07Z

Just checking in! Any updates?

Hello, It's been a while...

sorry, I'll be working on this by the holiday break. got busy over work.

gr1d99 · 2020-01-17T20:40:42Z

app/controllers/omniauth_callbacks_controller.rb

+    end
+  end
+
+  def github


I am just curious why we have github auth, are our users mostly from github?

this is something that @julianguyen can answer, but if I may try. since this is a platform that encourages collaboration, maybe a target audience of tech people are in mind?

Yeah GitHub authentication isn't a priority like Facebook would be for instance. This was requested by a contributor 🙃

But we could use this functionality to show links for filing GitHub issues just for folks signed in it.

We should also test this new method!

gr1d99 · 2020-01-17T20:43:26Z

docker-entrypoint.sh

@@ -0,0 +1,9 @@
+#!/bin/bash


sbpipb · 2020-04-02T15:46:57Z

Hello, I'm sorry if this feature has taken quite some time, but I'm finally freed up to resume work. I'll have some updates by this week. 🙇‍♂️ apologies!

- should be refactored to be reusable across different providers

- mvp version - skips validation of password when user is created via oauth

- to be refactored - working version

- uid uses "provider" + uid

- used english values for the meantime

julianguyen

Great work! We're almost there and ready to merge 🎉

julianguyen · 2020-04-09T06:38:40Z

Gemfile

@@ -84,6 +85,7 @@ group :development, :test do

  gem 'bullet'

+  gem 'awesome_print'


Is this gem necessary? What's it being used for?

julianguyen · 2020-04-09T06:39:48Z

app/controllers/omniauth_callbacks_controller.rb

+    end
+  end
+
+  def github


Yeah GitHub authentication isn't a priority like Facebook would be for instance. This was requested by a contributor 🙃

But we could use this functionality to show links for filing GitHub issues just for folks signed in it.

julianguyen · 2020-04-09T06:42:00Z

app/models/user.rb

-  validate :password_complexity
+
+  # add unless statement here instead of the concern for readability
+  # TODO: refactor the Password validator concern


Will this be done in another PR? :) Feel free to create an issue for this as well just to document.

This is one of the setbacks on making this PR short. I agree on having this as another ticket.

julianguyen · 2020-04-09T06:42:10Z

app/models/user.rb

-    user = find_or_initialize_by(email: access_token.info.email)
-    user.name ||= access_token.info.name
+  # TODO: to refactor and to move to builder pattern
+  # UserBuilder::GoogleOauth2


Similar comment as above.

julianguyen · 2020-04-09T06:43:01Z

app/services/user_builder/base.rb

@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+module UserBuilder


julianguyen · 2020-04-09T06:43:27Z

app/services/user_builder/builder.rb

+  class Builder
+    # Adds new omni auth providers here
+    SERVICE = {
+      # 'google_oauth' => GoogleOauth2


Should this be uncommented?

julianguyen · 2020-04-09T06:44:56Z

config/locales/it.yml

@@ -440,6 +440,7 @@ it:
      nostra %{code_of_conduct_link}. Non sei più bannato.'
  omniauth:
    google: Google
+    github: Github


So I don't think we need to add another GitHub translation since we already have one at navigation.github. In general, we don't want to introduce duplication in our translation files.

julianguyen · 2020-04-09T06:45:27Z

app/controllers/omniauth_callbacks_controller.rb

+    end
+  end
+
+  def github


We should also test this new method!

sbpipb · 2020-05-11T16:04:05Z

hello! this is taking longer than expected, and the quarantine has been affecting me as well on taking some time.

I'm determined to finish this within this month. Thanks everyone for being patient.

sbpipb · 2020-05-11T17:13:41Z

Merging master has been cumbersome. I feel like it's easier for me to just rewrite on a new branch 😞

julianguyen · 2020-05-11T19:02:15Z

Thanks so much for the update! Let me know if you need any help with the merge conflicts. I think creating a new branch and cherry-picking your commits should be a good way to resolve them faster.

sbpipb added the wip label Oct 20, 2019

sbpipb self-assigned this Oct 20, 2019

sbpipb commented Oct 20, 2019

View reviewed changes

julianguyen reviewed Oct 21, 2019

View reviewed changes

sbpipb force-pushed the github branch from 4d77366 to 1d5391f Compare October 25, 2019 10:54

sbpipb force-pushed the github branch from 1d5391f to aadece5 Compare October 25, 2019 10:57

sbpipb commented Oct 26, 2019

View reviewed changes

gr1d99 reviewed Jan 17, 2020

View reviewed changes

docker-entrypoint.sh Outdated

@@ -0,0 +1,9 @@

#!/bin/bash

Copy link

Contributor

gr1d99 Jan 17, 2020

Choose a reason for hiding this comment

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

👏 👏

sbpipb force-pushed the github branch from deb182a to f6ba3f4 Compare April 2, 2020 15:54

sbpipb added 13 commits April 6, 2020 16:07

adds omniauth github as dependency to Gemfiles

557bce5

add locales

4d66809

adds omniauth on devise initializer

d1771fb

adds github oauth2 as omniauth providers

06f2740

adds github method to persist user

6b1698a

- should be refactored to be reusable across different providers

initial controller action for github omniauth

6e6a3ac

updates github user builder

67c8e5e

- mvp version - skips validation of password when user is created via oauth

adds locale for github

bc9eb4c

adds links for github

e94f239

- to be refactored - working version

adds ap gem for dev

69c7cf4

updates auth on github to save uid unique

e4d6359

- uid uses "provider" + uid

uses alert option instead of notice to display authentication errors

55fbb03

adds new user builder

25f05a5

sbpipb added 8 commits April 6, 2020 16:07

adds comments to refactor google oauth 2

333028a

adds margin bottom

dd6b7bb

updates user builder

06b4482

adds missing keys for Omniauth Github

97641a7

- used english values for the meantime

Linter via rubocop

c82023f

comments

b585f49

lint fixes

c517f55

fix typo

a1e85f3

sbpipb force-pushed the github branch from d2ca4e1 to a1e85f3 Compare April 6, 2020 08:09

initial tests for user builder base

411ac59

julianguyen requested changes Apr 9, 2020

View reviewed changes

julianguyen closed this Oct 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] GitHub Authentication #1595

[WIP] GitHub Authentication #1595

sbpipb commented Oct 20, 2019 •

edited

Loading

sbpipb Oct 20, 2019

sbpipb Oct 20, 2019

julianguyen Oct 21, 2019

sbpipb Oct 20, 2019

julianguyen Apr 6, 2020

julianguyen left a comment

julianguyen Oct 21, 2019

julianguyen Oct 21, 2019

guilhermekbral commented Oct 23, 2019

sbpipb commented Oct 25, 2019

sbpipb Oct 26, 2019

julianguyen commented Nov 26, 2019

sbpipb commented Dec 9, 2019

gr1d99 Jan 17, 2020

sbpipb Apr 2, 2020

julianguyen Apr 9, 2020

julianguyen Apr 9, 2020

gr1d99 Jan 17, 2020

sbpipb commented Apr 2, 2020

julianguyen left a comment

julianguyen Apr 9, 2020

julianguyen Apr 9, 2020

julianguyen Apr 9, 2020

sbpipb May 11, 2020

julianguyen Apr 9, 2020

julianguyen Apr 9, 2020

julianguyen Apr 9, 2020

julianguyen Apr 9, 2020

julianguyen Apr 9, 2020

sbpipb commented May 11, 2020

sbpipb commented May 11, 2020

julianguyen commented May 11, 2020

		@@ -4,8 +4,12 @@
		<%- resource_class.omniauth_providers.each do \|provider\| %>
		<% if provider.to_s == "google_oauth2" && (current_page?(new_user_session_path) \|\| current_page?(new_user_password_path)) %>

		@@ -84,6 +85,7 @@ group :development, :test do

		gem 'bullet'

		gem 'awesome_print'

		@@ -0,0 +1,31 @@
		# frozen_string_literal: true
		module UserBuilder

[WIP] GitHub Authentication #1595

[WIP] GitHub Authentication #1595

Conversation

sbpipb commented Oct 20, 2019 • edited Loading

Description

More Details

Corresponding Issue

Screenshots

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianguyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilhermekbral commented Oct 23, 2019

sbpipb commented Oct 25, 2019

Choose a reason for hiding this comment

julianguyen commented Nov 26, 2019

sbpipb commented Dec 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbpipb commented Apr 2, 2020

julianguyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbpipb commented May 11, 2020

sbpipb commented May 11, 2020

julianguyen commented May 11, 2020

sbpipb commented Oct 20, 2019 •

edited

Loading