small config changes; added env variable to enable temp login in prod… #145

alexstrilets · 2017-04-04T19:22:27Z

1 config changes
2 added env variable ENABLE_TMP_LOGINS to enable temp logins in production mode
3 added Dockerfile to build docker image

…uction

mbarnett · 2017-04-04T20:51:53Z

config/initializers/omniauth.rb

@@ -12,7 +12,7 @@
           },
           name: 'CCID'

-  provider :developer unless Rails.env.production?
+  provider :developer unless (Rails.env.production? && !ENV['ENABLE_TMP_LOGINS'])


I think it would be a little more straightforward to test for the positive instead of the negation.

provider :developer unless (Rails.env.production? || ENV['ENABLE_TMP_LOGINS'].present?)

You can also forget about the parentheses as rubocop will complain to you about them.

config/initializers/omniauth.rb:15:30: C: Style/ParenthesesAroundCondition: Don't use parentheses around the condition of an unless. (https://github.com/bbatsov/ruby-style-guide#no-parens-around-condition) provider :developer unless (Rails.env.production? && !ENV['ENABLE_TMP_LOGINS']) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Just an aside, unrelated to the PR: it might be personal taste, but I think the version with parenthesis is more easily readable.

Actually can we fix that cop? I'd generally rather have parenthesis around post-conditionals. I've been bitten at least once by less-than-intuitive precedence when they're removed (because, naturally, rubocop complained about them).

agreed with @cwant. If we can fix that rule that would be great.

Regarding:
return true unless (method_possibly_throwing_exception rescue nil)
I would say that shouldn't be on one line?

🚲

Honestly this is why rubocop is both great and terrible. I don't want parentheses in

unless foo && bar do_thing end

But I DO want them in

do_thing unless (foo && bar)

because you can more easily get into precedence issues with the latter. I don't think rubocop treats them differently, so it might be better to just drop the cop and watch for the former manually.

Fair enough the people have spoken. Submit a PR/issue for it then or have it added to this one. ¯_(ツ)_/¯

Add

Style/ParenthesesAroundCondition: Enabled: false

to rubocop.yml

It's not so much that that example is great style or needs to be on one line (although it does take 7 lines to unroll it), it's just that it's an example where Rubocop will blindly tell you to break your code. This cop bugs me because it doesn't really understand the Ruby parse tree and it's easy-ish to get into situations where it will tell you to do the Wrong Thing™, which isn't great when not everyone will recognize situations where it's wrong.

I'll submit a PR to disable it, but I'll still consider it bad style to submit PRs which use parantheses around "normal" if/elsif/unless statements.

mbarnett · 2017-04-04T20:52:58Z

app/controllers/users/omniauth_callbacks_controller.rb

@@ -1,6 +1,6 @@
 class Users::OmniauthCallbacksController < ApplicationController
  # TODO: Remove this once we figure out IST, this is only required for the omniauth developer provider
-  skip_before_action :verify_authenticity_token, only: :complete unless Rails.env.production?
+  skip_before_action :verify_authenticity_token, only: :complete unless (Rails.env.production? && !ENV['ENABLE_TMP_LOGINS'])


I don't think we need this for the login form, as this just worked around an issue with TestShib.

Yeah this is needed for the developer login form, as it's form doesn't send CSRF tokens with it 👎 .

That's terrible. Where is the form generated & can we fix it?

Comes from ominauth gem (which probably doesn't have a dependency on Rails so thats why no CSRF token) . So probably not. It's honestly a hidden feature. Not documented at all and etc. AKA we shouldn't be using it much more then some quick dev testing

We could override the view to add the token? I dunno, it's not a deal breaker but it make me nervous anytime something is potentially disabling the auth_token checks. Thumbs down, omniauth devs. Add a soft-check for the method definition

Since apparently we have to keep this, I think we want the logic to be:

skip_before_action :verify_authenticity_token, only: :complete unless (Rails.env.production? || ENV['ENABLE_TMP_LOGINS'].present?)

To be consistent with the other change

Yeah we could override it or something. Code lives here:
https://github.com/omniauth/omniauth/blob/master/lib/omniauth/strategies/developer.rb
https://github.com/omniauth/omniauth/blob/master/lib/omniauth/form.rb

I am hoping this is temporary anyways. Hopefully testshib/IST SAML will replace the need for this code

Inline form HTML inside methods 🙀

murny

Build is failing because of rubocop errors. Make sure you run rubocop to catch some code style stuff. https://travis-ci.org/ualbertalib/Hydranorth2/builds/218592996

murny · 2017-04-04T21:02:50Z

config/solr.yml

@@ -4,4 +4,4 @@ development:
 test:
  url: <%= ENV['SOLR_TEST_URL'] || 'http://localhost:8985/solr/hydra-test' %>
 production:
-  url: http://your.production.server:8080/bl_solr/core0
+  url: <%= ENV['SOLR_PROD_URL'] || 'http://your.production.server:8080/bl_solr/core0' %>


Is having a different ENV VAR for each environment needed (e.g SOLR_<ENVIRONMENT>_URL)? Can a single ENV VAR of SOLR_URL be used? Same comment applies to blacklight.yml.

Looking at Hyku they do this: https://github.com/projecthydra-labs/hyku/blob/master/config/solr.yml
and Plum does this: https://github.com/pulibrary/plum/blob/master/config/solr.yml

OK, sounds like a good idea will fix it

murny · 2017-04-07T16:22:13Z

.rubocop.yml

@@ -80,3 +80,7 @@ RSpec/ExampleLength:

 RSpec/MultipleExpectations:
  Enabled: false
+
+# as discussed in issue#144
+Style/ParenthesesAroundCondition:


@mbarnett already fixed this in Master haha. Not sure if that was clear sorry alex.

https://github.com/ualbertalib/Hydranorth2/blob/master/.rubocop.yml#L74-L75

@astrilet this is already fixed, you don't need to include it in your PR.

murny · 2017-04-07T16:28:55Z

config/initializers/omniauth.rb

@@ -12,7 +12,8 @@
           },
           name: 'CCID'

-  provider :developer unless Rails.env.production?
+  provider :developer unless (Rails.env.production? ||
+                              ENV['ENABLE_TMP_LOGINS'].present?)


This conditional logic is still not quite right.

Previously it was unless (Rails.env.production? && !ENV['ENABLE_TMP_LOGINS']) which means it will only be enabled if it was both not in production AND the env var was set. Which isn't correct as it should be OR.

Now its unless (Rails.env.production? || ENV['ENABLE_TMP_LOGINS'].present?) which means its only enabled if its not in production OR ENV VAR is not present. Which is not true as we want the setting of the ENV VAR in production to allow this to be turned on?

Also unless gets very confusing when you have complicated conditionals (with &&, ||) so maybe it be easier if you switched it to an if?

Basically you want if !Rails.env.production? || ENV['ENABLE_TMP_LOGINS'].present? or maybe if Rails.env.development? || ENV['ENABLE_TMP_LOGINS'].present?

You are right unless gets very confusing, I will switch it to if statement

weiweishi

@astrilet I am not sure if the two docker related files really belong here. didocker-hydranorth2 is probably the better home for it (and I saw it's already there).

weiweishi · 2017-04-10T16:54:04Z

Dockerfile

+RUN cd /app && rake assets:precompile
+ADD bin/start_hn2.sh /usr/local/bin
+
+EXPOSE 3000


With Nginx configured, do we still need to expose port 3000 explicitly?

Yes we need to expose it inside docker image. This instruction means that other containers can connect to this one to the port 3000. This is what Nginx will do - will connect to instance of hydranorth2 running container and redirect port 3000 to its own port 80 which in turn will be redirected to atomic host port pointed by HOSTPORT environment variable.

weiweishi · 2017-04-10T17:02:45Z

bin/start_hn2.sh

+
+#check if db has been migrated
+if [ ! -e /db/initialized ]; then
+    sleep 10 && rake db:migrate && touch /db/initialized;


same question as @murny mentioned on the didocker-hydranorth2 repo, is the check here needed? db:migrate can be run as many times as you need.

OK, good point will remove that.

weiweishi · 2017-04-10T18:30:18Z

@astrilet I've found this comment here docker/compose#374:

Here's a way to do it with healthcheck and docker-compose 2.1+:

version: "2.1"
services:
db:
image: mysql:5.7
environment:
MYSQL_ROOT_PASSWORD: password
healthcheck:
test: mysqladmin -uroot -ppassword ping
interval: 2s
timeout: 5s
retries: 30
web:
image: nginx:latest # your image
depends_on:
db:
condition: service_healthy
Here docker-compose up will start the web container only after the db container is considered healthy.

This can probably be investigated and incorporated in if possible to avoid the use of additional script.

weiweishi

@astrilet Did you have a chance to take a look at Avalon's docker repo https://github.com/avalonmediasystem/avalon-docker. How they organize their project makes sense to me. Maybe we should not keep the Dockerfile in the application code repo.

weiweishi

Other than the change in .rubocop.yml, others look good as we discussed.

weiweishi · 2017-04-11T19:44:31Z

Please also update your PR description to include the Dockerfile change.

small config changes; added env variable to enable temp login in prod…

04c5121

…uction

alexstrilets assigned weiweishi Apr 4, 2017

weiweishi assigned mbarnett and murny Apr 4, 2017

mbarnett suggested changes Apr 4, 2017

View reviewed changes

murny suggested changes Apr 4, 2017

View reviewed changes

murny reviewed Apr 4, 2017

View reviewed changes

alexstrilets added 2 commits April 6, 2017 11:47

changes as discussed

74e98df

fixing rubocop errors

e53d922

murny reviewed Apr 7, 2017

View reviewed changes

alexstrilets added 3 commits April 7, 2017 13:49

switch unless ot if

f82fb73

fixed rubocop warning message

4ecdab7

moved Docker file into the repository

d8d1d94

weiweishi suggested changes Apr 10, 2017

View reviewed changes

weiweishi reviewed Apr 11, 2017

View reviewed changes

changes as discussed in the PR

5d4788e

weiweishi suggested changes Apr 11, 2017

View reviewed changes

revert changes as Shane already did them

1e8a457

weiweishi merged commit 15c2fd6 into master Apr 11, 2017

murny deleted the issue144 branch May 23, 2017 17:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

small config changes; added env variable to enable temp login in prod… #145

small config changes; added env variable to enable temp login in prod… #145

alexstrilets commented Apr 4, 2017 •

edited

mbarnett Apr 4, 2017

murny Apr 4, 2017

cwant Apr 4, 2017

mbarnett Apr 4, 2017

weiweishi Apr 4, 2017

murny Apr 4, 2017

mbarnett Apr 4, 2017 •

edited

murny Apr 4, 2017

murny Apr 4, 2017

mbarnett Apr 4, 2017

mbarnett Apr 4, 2017

murny Apr 4, 2017 •

edited

mbarnett Apr 4, 2017

murny Apr 4, 2017 •

edited

mbarnett Apr 4, 2017

mbarnett Apr 4, 2017

murny Apr 4, 2017

mbarnett Apr 4, 2017

murny left a comment

murny Apr 4, 2017 •

edited

alexstrilets Apr 6, 2017

murny Apr 7, 2017

weiweishi Apr 11, 2017

murny Apr 7, 2017 •

edited

alexstrilets Apr 7, 2017 •

edited

weiweishi left a comment

weiweishi Apr 10, 2017

alexstrilets Apr 10, 2017

weiweishi Apr 10, 2017

alexstrilets Apr 10, 2017

weiweishi commented Apr 10, 2017

weiweishi left a comment

weiweishi left a comment

weiweishi commented Apr 11, 2017

small config changes; added env variable to enable temp login in prod… #145

small config changes; added env variable to enable temp login in prod… #145

Conversation

alexstrilets commented Apr 4, 2017 • edited

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

mbarnett Apr 4, 2017 • edited

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

murny Apr 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murny Apr 4, 2017 • edited

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

murny left a comment

Choose a reason for hiding this comment

murny Apr 4, 2017 • edited

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

murny Apr 7, 2017 • edited

Choose a reason for hiding this comment

alexstrilets Apr 7, 2017 • edited

Choose a reason for hiding this comment

weiweishi 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

weiweishi commented Apr 10, 2017

weiweishi left a comment

Choose a reason for hiding this comment

weiweishi left a comment

Choose a reason for hiding this comment

weiweishi commented Apr 11, 2017

alexstrilets commented Apr 4, 2017 •

edited

mbarnett Apr 4, 2017 •

edited

murny Apr 4, 2017 •

edited

murny Apr 4, 2017 •

edited

murny Apr 4, 2017 •

edited

murny Apr 7, 2017 •

edited

alexstrilets Apr 7, 2017 •

edited