Removing need to specify requester email #30

Merged
merged 8 commits into from Jan 18, 2013

Projects

None yet

3 participants

@benilovj

Requesters no longer need to specify their email address. Information from SSO is used to set the requester details on the submitted requests and Zendesk tickets.

This pull request changes the ReadOnlyUser warden model to a model backed by a Redis-based cache store.

@fatbusinessman

Since the user isn’t going to be typing their email to say who they are, should we make it obvious that they’re logged in to avoid confusion? Maybe the same way as in our other backend tools?

"Logged in as" header from Publisher

@benilovj
@fatbusinessman fatbusinessman commented on an outdated diff Dec 14, 2012
app/models/user.rb
end
def update_attribute(*args)
end
def update_attributes(params, hash)
- ReadOnlyUser.new(params)
+ User.new(params)
@fatbusinessman
fatbusinessman Dec 14, 2012

Since we now have a backing store, shouldn’t the update_attribute and update_attributes methods now update the cache?

@fatbusinessman

I would say put it in this pull request: it feels to me like part of the same change, or at least a side-effect of it.

@benilovj

@fatbusinessman I've added the username and log-out to the layout

@benilovj

@fatbusinessman @jamiecobbett I have implemented more of the User interface to be backed by the Rails cache, as well as adding some tests. Does the User class look complete now?

@jamiecobbett jamiecobbett and 1 other commented on an outdated diff Dec 17, 2012
config/application.rb
@@ -48,6 +48,8 @@ class Application < Rails::Application
# Enable escaping HTML in JSON.
config.active_support.escape_html_entities_in_json = true
+ config.cache_store = :memory_store
@jamiecobbett
jamiecobbett Dec 17, 2012

If I understand correctly, this means that the sessions are stored in memory in each Rails process. So that means I'll need to login (or at least go through the OAuth redirect dance) for every Rails process that I hit whilst using the app? That'll break POST requests - you can't redirect to a POST.

I think we have a default of 4 unicorn slaves, and given that we avoid any single points of failure, that'll be on multiple boxes...

It'll also mean that the sessions won't be in sync across the processes, so if eg my permissions are updated, they'll only be updated in one process (unless I sign in and out until they're all done).

http://guides.rubyonrails.org/caching_with_rails.html#activesupport-cache-memorystore

I know @jystewart has reservations, but I think it might make sense to use MySQL for session storage, unless we can add memcached (or similar) to the stack? It could always be a stop-gap (famous last words).

@benilovj
benilovj Dec 17, 2012

One option that @jystewart and I discussed last week was to use a Redis or Memcached-backed Rails cache store (presumably shared across the nodes). Would that work, @jamiecobbett ?

@benilovj
benilovj Dec 18, 2012

A lot of work has already been done (https://github.com/alphagov/puppet/pull/187) to get redis integrated into the stack. I'll try to piggyback on those efforts.

@jamiecobbett

We should probably invalidate all existing sessions when this change is rolled out so that we don't try to get the user's name from a session which doesn't have it. @benilovj I don't believe you have access to this repo, but what we'll do is change the token that's used to sign sessions (a one char change would be enough): https://github.com/alphagov/alphagov-deployment/blob/master/support/to_upload/initializers/production/secret_token.rb

@jamiecobbett jamiecobbett merged commit 043e678 into master Jan 18, 2013
@jamiecobbett

@benilovj After speaking to @nickstenning via @maxamg it seems Redis isn't ready for use in production yet, so we might need to back this out.

@benilovj
@benilovj benilovj added a commit that referenced this pull request Jan 21, 2013
@benilovj benilovj Revert "Merge pull request #30 from alphagov/removing_need_to_specify…
…_requester_email"

This reverts commit 043e678, reversing
changes made to 2b3e204.

Since redis isn't yet production-ready, we're backing this change out
156bb70
@benilovj benilovj added a commit that referenced this pull request Mar 12, 2013
@benilovj benilovj reinsert removing_need_to_specify_requester_email
Revert "Revert "Merge pull request #30 from alphagov/removing_need_to_specify_requester_email""

This reverts commit 156bb70.

Conflicts:

	Gemfile.lock
	test/test_helper.rb
51e6830
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment