Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add ManageIQ::Session, with adapters and .revoke functionality #20549

Merged
merged 4 commits into from
Sep 23, 2020
Merged

Add ManageIQ::Session, with adapters and .revoke functionality #20549

merged 4 commits into from
Sep 23, 2020

Conversation

NickLaMuro
Copy link
Member

@NickLaMuro NickLaMuro commented Sep 15, 2020 •
edited
Loading

Part of the #20378 and #20462 efforts.

Refactors the config/initializers/session_store.rb and config/initializers/session_store_memory.rb into the lib/manageiq/session_store* dir, and sets up each option as an adapter. Configured in the same fashion, but using this pattern allows for being able to make easier modifications/patches to each of the stores used.

The main change was the addition of the .delete_sessions, which takes an array of session_ids and removes the from the configured store.

In addition, to support this, a ManageIQ::SessionStore.store method/variable was added which keeps a singleton instance of the session store available for the backend to use when for revoking sessions. ManageIQ::SessionStore.revoke_all was also added as convenience method for calling .delete_sessions on the configured store.

Links

Steps for Testing/QA

I still need to do some more tests, but I was testing this locally by:

  • Running bin/rails s
  • Logging in
  • In a bin/rails c running: irb> ManageIQ::SessionStore.revoke_all(Session.pluck(:session_id))
  • Refreshing the browser and being logged out

Not perfect, and makes a lot of assumptions, but works as a POC for now. Will probably update these steps later to be a little more "idiot proof".

@miq-bot miq-bot added the wip label Sep 15, 2020
@NickLaMuro NickLaMuro mentioned this pull request Sep 15, 2020
Closed
8 tasks
@NickLaMuro
Copy link
Member Author

NickLaMuro commented Sep 15, 2020 •
edited by chessbyte
Loading

Okay, looks like I have to fix tests with this one... might need @gtanzillo 's migration for this to work as well.

That said:

@Fryguy @gtanzillo are you at least cool with the ManageIQ::SessionStore.revoke_all interface that this PR introduces? Don't need a full review right now, but I think Keenan and Libor are going to be using this PR as a base for some of their work, so I don't want drastic changes if you think they are needed from what I have here.

cc @kbrock @lpichler (opinions on this also welcome if you have them)

@chessbyte
Copy link
Member

chessbyte commented Sep 15, 2020 •
edited
Loading

@NickLaMuro why the name ManageIQ::SessionStore.revoke_all instead of ManageIQ::SessionStore.revoke? Feels like revoke_all should be looping over revoke with some list.

@NickLaMuro
Copy link
Member Author

@chessbyte No reason beyond I was just matching the delete_all convention. I don't have a strong preference, just would like to get the interface nailed down so Keenan and Libor have something to reliably code against (even if the internals have to change).

kbrock
kbrock reviewed Sep 15, 2020
View reviewed changes
Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

I like how you got all the special ruby logic out of the initializer.

Is it possible to use rails's session factory instead of adding our own?
We'd need to patch their class, but seems we don't have major qualms of doing that

lib/manageiq/session_store.rb Outdated Show resolved Hide resolved
@NickLaMuro
Copy link
Member Author

NickLaMuro commented Sep 16, 2020 •
edited
Loading

Is it possible to use rails's session factory instead of adding our own?

@kbrock While yes, the ManageIQ::SessionStore currently is in charge of some configuration of the storage implementations instead of just modifying them and acting as an interface (which I fully admit muddies the water a bit), nothing would be simplified by using a "Rails factory" in any way from what wasn't already done in config/initializers/session_store.rb prior to this PR.

What that file did was:

  • It determined which one of the adapters you wanted to use based on multiple input sources (Rails.env, ENV vars, Settings)
  • Configured shared options across all provider types
  • Patches the memcache store if being used
  • And configures the middleware using the standard Rails interface
  • Log what session store we are using (if appropriate)

The reason I didn't continue using the initializer is that it didn't make sense to add even more patches and branching logic to this config file, as it would have gotten unwieldy, but there isn't anything that would have made it "simpler".

The patches are required because Rails assumes a request object for deleting sessions, and for the API, we just won't have that. Period.

@Fryguy
Copy link
Member

Fryguy commented Sep 16, 2020

A lot of the adapter management feels a little overkill in that it's trying to be very targeted about only patching the session store class that the user chose. Instead, can we patch all of the session stores regardless? It wouldn't be very expensive since the stores are, I think, all loaded anyway, and code-wise it would be much simplified.

I like the ManageIQ::SessionStore.store abstraction in general as it isolates the session options, but without the patch management aspect, I'm not sure it's really needed. That is, I think the original code can stay as is, then the patches can be introduced under lib/extensions/ar_session/*, which would be auto-patched the same way the other extensions are patched in.

@NickLaMuro
Copy link
Member Author

... can we patch all of the session stores regardless?

Yeah, I think that is a good point. I sort of kept the same mind set of what was originally in the initializer, which only patched the Dalli store if it was being used. However, I think I agree with your point and that would probably simplify things.

Will look into that more today.

NickLaMuro
NickLaMuro commented Sep 17, 2020
View reviewed changes
lib/manageiq/session.rb Outdated Show resolved Hide resolved
Fryguy
Fryguy reviewed Sep 17, 2020
View reviewed changes
lib/manageiq/session.rb Outdated Show resolved Hide resolved
Fryguy
Fryguy reviewed Sep 17, 2020
View reviewed changes
app/models/user.rb Outdated Show resolved Hide resolved
@Fryguy Fryguy changed the title [WIP] Add ManageIQ::SessionStore, with adapters and .revoke_all functionality [WIP] Add ManageIQ::Session, with adapters and .revoke functionality Sep 17, 2020
Fryguy
Fryguy reviewed Sep 17, 2020
View reviewed changes
lib/manageiq/session.rb Outdated Show resolved Hide resolved
@Fryguy
Copy link
Member

Fryguy commented Sep 17, 2020

This is awesome! Really coming along nice

@NickLaMuro NickLaMuro force-pushed the revoke-user-session branch 4 times, most recently from 5b7e707 to fa0ec4f Compare September 17, 2020 15:43
@NickLaMuro NickLaMuro changed the title [WIP] Add ManageIQ::Session, with adapters and .revoke functionality Add ManageIQ::Session, with adapters and .revoke functionality Sep 17, 2020
@NickLaMuro
Copy link
Member Author

^ Just added some tests and rebased in a few fixup commits.

@miq-bot miq-bot removed the wip label Sep 17, 2020
@Fryguy Fryguy self-assigned this Sep 18, 2020
Fryguy
Fryguy reviewed Sep 18, 2020
View reviewed changes
lib/manageiq/session.rb Outdated Show resolved Hide resolved
Fryguy
Fryguy reviewed Sep 18, 2020
View reviewed changes
lib/manageiq/session.rb Outdated Show resolved Hide resolved
Fryguy
Fryguy approved these changes Sep 18, 2020
View reviewed changes
Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

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

LGTM - two minor comments - I'll wait on a response before merge.

@Fryguy
Copy link
Member

Fryguy commented Sep 21, 2020

Also noticed a few rubocops, which most look legit.

@NickLaMuro
Copy link
Member Author

lib/manageiq/session.rb

  • ❗ - Line 24, Col 7 - Rails/Output - Do not write to stdout. Use Rails's logger if you want to log.

This is how it was previously, so not going to change.

Yeah, botched the spacing here at some point. Will fix.

spec/lib/manageiq/session_spec.rb
...

Will address all of these.

@NickLaMuro
[ManageIQ::Session] Refactor config into lib
1a6b054 
Moves the logic in the config/initializers/session.rb to facilitate
future extensions.
@NickLaMuro
[ManageIQ::Session] Patch .delete_sessions
adc14ef 
Adds/Patches a .delete_sessions method to all of the session store
adapters that can delete a collection of ids that are passed to it.

Implements a `FakeRequest` object that can be passed as part of the args
for `delete_sessions` for each store that can act like a
`ActionDispatch::Request` object, but simply provides the `.env` var as
an empty hash.  This allows `.delete_session` to be used with it's
original call signatures (for the adapters that require it) without
having to dive deep into the internals of each adapter to call custom
methods that do the same thing.

`:drop` is basically assumed for all adapters since there is no good way
to keep track of the existing data for the session when dealing with
multiple sessions.
@NickLaMuro
[ManageIQ::Session] Add .store
586fc8f 
Adds a quick way of accessing the session store from outside of the
Rails session middleware.  Allows our backend to more easily interact
with the SessionStore via the ManageIQ::Session interface.

The `.fetch_store_from_rails` is lazily fetched to ensure that the
middleware stack is built before we try and access it.  Also, the code
for that is pretty gnarly... blame Jason...
@NickLaMuro
Copy link
Member Author

@Fryguy alright, changes applied. For some extra credit, I also documented the class a bit.

@NickLaMuro
[ManageIQ::Session] Implement .revoke_all
b0297ca 
Adds a convenience method to `ManageIQ::Session` that can use the
configured store to call `delete_sessions` on the configured
*StoreAdapter.
@NickLaMuro
Copy link
Member Author

@Fryguy besides the puts (that existed previously), I fixed up the remaining linting issues. Is there anything else that you wanted addressed before a merge?

@miq-bot
Copy link
Member

miq-bot commented Sep 22, 2020

Some comments on commits NickLaMuro/manageiq@1a6b054~...b0297ca

lib/manageiq/session.rb

  • ⚠️ - 50 - Detected puts. Remove all debugging statements.

@miq-bot
Copy link
Member

miq-bot commented Sep 22, 2020

Checked commits NickLaMuro/manageiq@1a6b054~...b0297ca with ruby 2.6.3, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
10 files checked, 1 offense detected

lib/manageiq/session.rb

  • ❗ - Line 50, Col 7 - Rails/Output - Do not write to stdout. Use Rails's logger if you want to log.

@Fryguy Fryguy merged commit 907fea1 into ManageIQ:master Sep 23, 2020
This was referenced Sep 23, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fryguy Fryguy Fryguy approved these changes

@kbrock kbrock kbrock approved these changes

@agrare agrare Awaiting requested review from agrare

@gtanzillo gtanzillo Awaiting requested review from gtanzillo

@jrafanie jrafanie Awaiting requested review from jrafanie jrafanie is a code owner

Assignees

@Fryguy Fryguy

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@NickLaMuro @chessbyte @Fryguy @miq-bot @kbrock @gtanzillo