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

[F] Add support for OAuth authentication #208

Merged
merged 12 commits into from Apr 13, 2017

Conversation

Projects
None yet
4 participants
@scryptmouse
Contributor

scryptmouse commented Mar 31, 2017

[Delivers #139242561]

@zdavis

This comment has been minimized.

Show comment
Hide comment
@zdavis

zdavis Apr 1, 2017

Member

I eyeballed this and am going to work on getting it running in my env over the weekend. It looks like exceptionally good work, at first pass.

Would you mind including some specs—use your own judgement as to what is most worthy of tests—for the API changes? While our testing is regrettably lighter on the client side, the actions and reducers tend to be really easy to test because they're stateless.

Are the errors that show up in the oAuth monitor component meant to be visible to end users? If so, should we style them up like our other errors?

Member

zdavis commented Apr 1, 2017

I eyeballed this and am going to work on getting it running in my env over the weekend. It looks like exceptionally good work, at first pass.

Would you mind including some specs—use your own judgement as to what is most worthy of tests—for the API changes? While our testing is regrettably lighter on the client side, the actions and reducers tend to be really easy to test because they're stateless.

Are the errors that show up in the oAuth monitor component meant to be visible to end users? If so, should we style them up like our other errors?

@scryptmouse

This comment has been minimized.

Show comment
Hide comment
@scryptmouse

scryptmouse Apr 3, 2017

Contributor

Getting started on that today. The user upsertion is wrapped in ActiveInteractions, so getting specs up for the different providers should go swiftly.

Contributor

scryptmouse commented Apr 3, 2017

Getting started on that today. The user upsertion is wrapped in ActiveInteractions, so getting specs up for the different providers should go swiftly.

@zdavis

This comment has been minimized.

Show comment
Hide comment
@zdavis

zdavis Apr 4, 2017

Member

Another thought... in components/global/SignInUp/Login.js we need to be sure to not show the buttons for the various providers if the required settings haven't been put in place on the server-side. Because the client doesn't have access to the OAuth keys, one way to accomplish this might be to include some flags (google_oauth_enabled, etc) in the settings object that is exposed to the client. Those flags could be based on the existence of the requisite keys in the API env.

Member

zdavis commented Apr 4, 2017

Another thought... in components/global/SignInUp/Login.js we need to be sure to not show the buttons for the various providers if the required settings haven't been put in place on the server-side. Because the client doesn't have access to the OAuth keys, one way to accomplish this might be to include some flags (google_oauth_enabled, etc) in the settings object that is exposed to the client. Those flags could be based on the existence of the requisite keys in the API env.

@zdavis

This comment has been minimized.

Show comment
Hide comment
@zdavis

zdavis Apr 6, 2017

Member

Hey Alexa. Is this ready for final review in your opinion, or are you still working on it? If it's ready, please add the ready for review label. Thanks!

Member

zdavis commented Apr 6, 2017

Hey Alexa. Is this ready for final review in your opinion, or are you still working on it? If it's ready, please add the ready for review label. Thanks!

@scryptmouse

This comment has been minimized.

Show comment
Hide comment
@scryptmouse

scryptmouse Apr 6, 2017

Contributor

I am still wrapping up the specs & the settings for hiding buttons for providers that aren't set up, but it should be done this evening and will do so

Contributor

scryptmouse commented Apr 6, 2017

I am still wrapping up the specs & the settings for hiding buttons for providers that aren't set up, but it should be done this evening and will do so

@zdavis

This comment has been minimized.

Show comment
Hide comment
@zdavis

zdavis Apr 6, 2017

Member

These changes are breaking the rspec run on Travis

TypeError: Invalid OAuth app id
Member

zdavis commented Apr 6, 2017

These changes are breaking the rspec run on Travis

TypeError: Invalid OAuth app id

@zdavis zdavis added needs revision and removed needs review labels Apr 6, 2017

Show outdated Hide outdated api/app/lib/statistics.rb
start_date,
end_date,
"annotation").count
Annotation.where(created_at: @range, format: 'annotation').count

This comment has been minimized.

@houndci-bot

houndci-bot Apr 7, 2017

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@houndci-bot

houndci-bot Apr 7, 2017

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

Show outdated Hide outdated api/app/lib/statistics.rb
start_date,
end_date,
"highlight").count
Annotation.where(created_at: @range, format: 'highlight').count

This comment has been minimized.

@houndci-bot

houndci-bot Apr 7, 2017

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@houndci-bot

houndci-bot Apr 7, 2017

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@naomiyaki naomiyaki requested review from naomiyaki and removed request for naomiyaki Apr 7, 2017

@zdavis zdavis added needs review and removed needs revision labels Apr 8, 2017

@zdavis

This comment has been minimized.

Show comment
Hide comment
@zdavis

zdavis Apr 10, 2017

Member

Rebased onto development, resolved conflicts, and force pushed

Member

zdavis commented Apr 10, 2017

Rebased onto development, resolved conflicts, and force pushed

@zdavis

This comment has been minimized.

Show comment
Hide comment
@zdavis

zdavis Apr 11, 2017

Member

@scryptmouse I'm passing this back to you to adjust the environment settings. If you could tackle that this morning, I'd appreciate it. I've put a file called updated_dot_env in the Manifold project folder on the NAS. If you put that in your development environment and start the server, Manifold should update the settings from the environment when it starts up. With my change, all the settings you need for OAuth will be available within the API at Settings.instance.integrations and Settings.instance.secrets.

Member

zdavis commented Apr 11, 2017

@scryptmouse I'm passing this back to you to adjust the environment settings. If you could tackle that this morning, I'd appreciate it. I've put a file called updated_dot_env in the Manifold project folder on the NAS. If you put that in your development environment and start the server, Manifold should update the settings from the environment when it starts up. With my change, all the settings you need for OAuth will be available within the API at Settings.instance.integrations and Settings.instance.secrets.

Show outdated Hide outdated api/config/initializers/update_settings_from_env.rb
@@ -0,0 +1,12 @@
update = true
update = false if ENV["MANAGE_SETTINGS_FROM_ENV"].present?
update = false if ActiveRecord::Base.connection.data_source_exists?('settings')

This comment has been minimized.

@houndci-bot

houndci-bot Apr 11, 2017

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@houndci-bot

houndci-bot Apr 11, 2017

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@zdavis

This comment has been minimized.

Show comment
Hide comment
@zdavis

zdavis Apr 11, 2017

Member

Sometimes I want to punch Travis CI in his cute little face.

Member

zdavis commented Apr 11, 2017

Sometimes I want to punch Travis CI in his cute little face.

@zdavis zdavis added in progress and removed needs review labels Apr 11, 2017

Show outdated Hide outdated api/config/initializers/update_settings_from_env.rb
update = false if ENV["MANAGE_SETTINGS_FROM_ENV"].present?
update = false if ActiveRecord::Base.connection.data_source_exists?("settings")
Settings.instance.update_from_environment if update

This comment has been minimized.

@houndci-bot

houndci-bot Apr 11, 2017

Style/TrailingBlankLines: 1 trailing blank lines detected.

@houndci-bot

houndci-bot Apr 11, 2017

Style/TrailingBlankLines: 1 trailing blank lines detected.

Show outdated Hide outdated api/app/models/settings.rb
env_key_path = ENV["MANIFOLD_SETTING_SECRETS_GOOGLE_PRIVATE_KEY"]
path = Rails.application.root.join("..", env_key_path)
if File.exist?(path) && File.file?(path)
send("secrets=", { google_private_key: File.read(path) })

This comment has been minimized.

@houndci-bot

houndci-bot Apr 11, 2017

Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

@houndci-bot

houndci-bot Apr 11, 2017

Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

@zdavis zdavis merged commit a8caa83 into development Apr 13, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!

@zdavis zdavis deleted the ag/oauth-support branch Apr 13, 2017

@zdavis zdavis removed the in progress label Jun 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment