Add User.revoke_sessions #20601

NickLaMuro · 2020-09-25T21:12:28Z

Built on top of #20477
Requires ManageIQ/manageiq-schema#503

Adds functionality to revoke all sessions (API, UI, etc.) from a given user.

Links

Part of Link user to session ids to enable looking up the session ids of every session a user owns #20476
Link user to the sessions create to enable lookup of all sessions by user #20477

Steps for Testing/QA

I tested this locally by:

Starting up a server
Running a collection of API calls, where I first requested a token
Looked up the user that was fetching said token
Called this new method on that User instance (.revoke_sessions)

NickLaMuro · 2020-09-25T21:12:44Z

cc @Fryguy @gtanzillo

app/models/user.rb

kbrock

this looks good nick

lib/token_store/sql_store.rb

app/models/user.rb

Fryguy · 2020-09-29T14:19:24Z

app/models/user.rb

+ def revoke_sessions
+ current_sessions = Session.where(:user_id => id)
+ ManageIQ::Session.revoke(current_sessions.map(&:session_id))
+ current_sessions.destroy_all


Wondering if this should be in a transaction?

Probably not a bad idea.

Actually, I am going to hold off on this.

While a transaction would be nice, unfortunately, there is a possibility it will be not function as expected, since ManageIQ::Session could have a .store of memcached, so the transaction wouldn't be "fully reverted" if we were to wrap both .revoke and .destroy_all in a single transaction.

I think the .destroy_all would be in it's own transaction, but I could be wrong so I don't know if there is any value of wrapping just that line.

lib/token_store.rb

Fryguy · 2020-09-29T14:24:55Z

lib/token_store.rb

+ if data[:userid]
+ user_tokens_cache_key = "tokens_for_#{data[:userid]}"
+ user_tokens_cache = read(user_tokens_cache_key) || []
+ user_tokens_cache << token


This is really tricky in a multi-threaded environment particularly with memcached. As two competing threads come in, they can't step on each other.

This was sort of what I was hinting at in the original proposal (#20378 (comment) - see the cross out section on memory_store), where I mentioned for memached we might need to use CAS.

These particular "create an extra index in the memory store" was what we were trying to avoid by storing all of the records in the sessions table regardless of the store type. I'm not against encoding an index in each one instead, but I think each one has nuances.

if we are using multiple memcached instances, then each server will have a unique list of session ids
and has a low likelihood of stepping on another server's toes. Also, every server can not clear the sessions on other memcached servers. Maybe the use cases for problems are simpler in terms of the session store and more complicated in terms of broadcasting to delete all of a user's sessions.

Yes. A single server could have multiple sessions coming live at the same time. Maybe via the api and rails at the same time? I am having trouble coming up with a use case that doesn't seem far fetched.

Before we go super complex on distributed memcached servers, can we entertain using a single session store? Just like we use a single data store (postgres).

lib/token_store/sql_store.rb

app/models/user.rb

NickLaMuro · 2020-09-30T15:36:43Z

@Fryguy @kbrock @lpichler I think I have addressed all of your feedback, so please let me know if there is anything else.

@lpichler do these changes now work for you? I assume you had this pulled down locally previously.

@Fryguy So, I think the only outstanding item in this PR is the threading issues, which I think I would like to address in a followup, assuming you are okay with the approach we have now currently.

This isn't to say it isn't important, but since I am not sure how we want to tackle it, and if what we did is even helping, I think deferring the implementation of this would allow the rest of the effort to move forward while we mull over the details to address that particular issue.

That said, if you think how any of this is implemented currently might be at odds with a future "thread/process -safe" solution in the future, I am happy to address that now. The deferring of implementing the locking/mutex/CAS bits is what I am suggesting at the moment.

app/models/user.rb

lpichler · 2020-09-30T17:42:25Z

@lpichler do these changes now work for you? I assume you had this pulled down locally previously.

yes it works - otherwise I have two comments and it looks that those issues need to be solved:

thanks

NickLaMuro · 2020-09-30T18:45:43Z

@lpichler thanks for taking a look! I should have addressed your issues now. Sorry for the derps...

lpichler · 2020-10-01T12:22:38Z

@lpichler thanks for taking a look! I should have addressed your issues now. Sorry for the derps...

@NickLaMuro, it is working great, thanks!

it looks that CI failure is related to this PR.

NickLaMuro · 2020-10-01T17:00:55Z

it looks that CI failure is related to this PR.

It was, though the way the spec was written, it probably was a scenario that wouldn't happen. Regardless, I added a guard clause (.try) to allow it to work properly.

NickLaMuro · 2020-10-06T14:53:15Z

@Fryguy @kbrock I think you two are the outliers here that left reviews, but didn't follow up.

Are you good with the new changes? In particular, @Fryguy , are you good with the rationale I gave regarding the locking functionality and addressing it at another time? [ref]

Fryguy · 2020-10-06T21:36:07Z

I am good with deferring the theadsafety issues to another PR, but I think it has to be in relatively quickly. Puma by its nature is multi-threaded, so we will start seeing issues almost immediately, I'd expect. @jrafanie wdyt?

…user Fixes ManageIQ#20476

Implements methods for tracking user tokens for a given userid so they can be deleted all at once.

Makes the instance variable `@token_store` of `TokenStore` a `cattr_reader`, and then uses that to loop through each of the token stores to delete all of the sessions associated with the currently instanced userid.

miq-bot · 2020-10-07T13:20:25Z

Checked commits NickLaMuro/manageiq@0bd0c87~...7b4679d with ruby 2.6.3, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
4 files checked, 0 offenses detected
Everything looks fine. 🍪

kbrock

this is looking much nicer. thanks nick

kbrock · 2020-10-08T01:02:05Z

lib/token_store.rb

@@ -1,20 +1,47 @@
 class TokenStore
- @token_caches = {} # Hash of Memory/Dalli Store Caches, Keyed by namespace
+ module KeyValueHelpers


thanks - this is much nicer than the individual monkey patches

kbrock · 2020-10-08T01:07:58Z

lib/token_store.rb

+ if data[:userid]
+ user_tokens_cache_key = "tokens_for_#{data[:userid]}"
+ user_tokens_cache = read(user_tokens_cache_key) || []
+ user_tokens_cache << token


if we are using multiple memcached instances, then each server will have a unique list of session ids
and has a low likelihood of stepping on another server's toes. Also, every server can not clear the sessions on other memcached servers. Maybe the use cases for problems are simpler in terms of the session store and more complicated in terms of broadcasting to delete all of a user's sessions.

Yes. A single server could have multiple sessions coming live at the same time. Maybe via the api and rails at the same time? I am having trouble coming up with a use case that doesn't seem far fetched.

Before we go super complex on distributed memcached servers, can we entertain using a single session store? Just like we use a single data store (postgres).

NickLaMuro · 2020-10-08T01:56:54Z

Before we go super complex on distributed memcached servers, can we entertain using a single session store? Just like we use a single data store (postgres).

@kbrock I will not be "entertaining" anything like that until after this PR has been merged, and probably not even consider it myself until the next release. I already asked for this to be reviewed a few days ago now after it has been open for close to a couple of weeks, only for it to be made unmergable to "entertain a refactoring" that really solved nothing:

#20663

This is holding up other dependent work on being finished, so anything in he name of refactoring is a hard pass until the MVP of this feature is addressed.

gtanzillo · 2020-10-08T20:28:00Z

@Fryguy Can this be merged? I think Nick addressed all of the comments

NickLaMuro requested review from agrare, Fryguy, gtanzillo and kbrock as code owners September 25, 2020 21:12

miq-bot added the wip label Sep 25, 2020

NickLaMuro force-pushed the user-revoke-sessions branch 2 times, most recently from 7446bc3 to 580cfcb Compare September 28, 2020 16:22

NickLaMuro commented Sep 28, 2020

View reviewed changes

app/models/user.rb Show resolved Hide resolved

kbrock reviewed Sep 28, 2020

View reviewed changes

lib/token_store/sql_store.rb Show resolved Hide resolved

lib/token_store/sql_store.rb Outdated Show resolved Hide resolved

app/models/user.rb Show resolved Hide resolved

app/models/user.rb Outdated Show resolved Hide resolved

app/models/user.rb Outdated Show resolved Hide resolved

Fryguy reviewed Sep 29, 2020

View reviewed changes

app/models/user.rb Show resolved Hide resolved

Fryguy reviewed Sep 29, 2020

View reviewed changes

lib/token_store.rb Outdated Show resolved Hide resolved

Fryguy reviewed Sep 29, 2020

View reviewed changes

lib/token_store/sql_store.rb Outdated Show resolved Hide resolved

lpichler reviewed Sep 29, 2020

View reviewed changes

app/models/user.rb Show resolved Hide resolved

NickLaMuro force-pushed the user-revoke-sessions branch 2 times, most recently from b4bf634 to 7b86b37 Compare September 30, 2020 15:13

lpichler mentioned this pull request Sep 30, 2020

Add action "revoke sessions" to users_controllers ManageIQ/manageiq-api#900

Merged

3 tasks

lpichler reviewed Sep 30, 2020

View reviewed changes

app/models/user.rb Outdated Show resolved Hide resolved

NickLaMuro force-pushed the user-revoke-sessions branch from 7b86b37 to fb2cb6b Compare September 30, 2020 18:45

NickLaMuro mentioned this pull request Sep 30, 2020

Ability to track and revoke sessions #20378

Closed

8 tasks

NickLaMuro force-pushed the user-revoke-sessions branch from fb2cb6b to 137d230 Compare October 1, 2020 15:40

NickLaMuro changed the title ~~[WIP] Add User.revoke_sessions~~ Add User.revoke_sessions Oct 1, 2020

miq-bot removed the wip label Oct 1, 2020

gtanzillo added the enhancement label Oct 1, 2020

lpichler approved these changes Oct 1, 2020

View reviewed changes

gtanzillo approved these changes Oct 1, 2020

View reviewed changes

gtanzillo assigned Fryguy Oct 1, 2020

miq-bot added the unmergeable label Oct 6, 2020

gtanzillo and others added 5 commits October 7, 2020 08:09

Link user to the sessions create to enable lookup of all sessions by …

0bd0c87

…user Fixes ManageIQ#20476

[User] Add #revoke_sessions

7b3bf27

[TokenManager] Track user tokens

d809ae9

Implements methods for tracking user tokens for a given userid so they can be deleted all at once.

[User#revoke_sessions] Remove API tokens

66b3c1c

Makes the instance variable `@token_store` of `TokenStore` a `cattr_reader`, and then uses that to loop through each of the token stores to delete all of the sessions associated with the currently instanced userid.

[User] Add .broadcast_revoke_sessions

7b4679d

NickLaMuro force-pushed the user-revoke-sessions branch from 137d230 to 7b4679d Compare October 7, 2020 13:16

miq-bot removed the unmergeable label Oct 7, 2020

kbrock reviewed Oct 8, 2020

View reviewed changes

Fryguy merged commit 46d1dd4 into ManageIQ:master Oct 9, 2020

Fryguy mentioned this pull request Oct 12, 2020

Link user to the sessions create to enable lookup of all sessions by user #20477

Closed

Fryguy mentioned this pull request Oct 20, 2020

Need multi-appliance revoke user support #20465

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add User.revoke_sessions #20601

Add User.revoke_sessions #20601

NickLaMuro commented Sep 25, 2020 •

edited

Loading

NickLaMuro commented Sep 25, 2020

kbrock left a comment

Fryguy Sep 29, 2020

NickLaMuro Sep 29, 2020

NickLaMuro Sep 30, 2020

Fryguy Sep 29, 2020

kbrock Oct 8, 2020

NickLaMuro commented Sep 30, 2020

lpichler commented Sep 30, 2020 •

edited

Loading

NickLaMuro commented Sep 30, 2020

lpichler commented Oct 1, 2020

NickLaMuro commented Oct 1, 2020

NickLaMuro commented Oct 6, 2020 •

edited by chessbyte

Loading

Fryguy commented Oct 6, 2020

miq-bot commented Oct 7, 2020

kbrock left a comment

kbrock Oct 8, 2020

kbrock Oct 8, 2020

NickLaMuro commented Oct 8, 2020 •

edited

Loading

gtanzillo commented Oct 8, 2020

Add User.revoke_sessions #20601

Add User.revoke_sessions #20601

Conversation

NickLaMuro commented Sep 25, 2020 • edited Loading

Links

Steps for Testing/QA

NickLaMuro commented Sep 25, 2020

kbrock left a comment

Choose a reason for hiding this comment

Fryguy Sep 29, 2020

Choose a reason for hiding this comment

NickLaMuro Sep 29, 2020

Choose a reason for hiding this comment

NickLaMuro Sep 30, 2020

Choose a reason for hiding this comment

Fryguy Sep 29, 2020

Choose a reason for hiding this comment

kbrock Oct 8, 2020

Choose a reason for hiding this comment

NickLaMuro commented Sep 30, 2020

lpichler commented Sep 30, 2020 • edited Loading

NickLaMuro commented Sep 30, 2020

lpichler commented Oct 1, 2020

NickLaMuro commented Oct 1, 2020

NickLaMuro commented Oct 6, 2020 • edited by chessbyte Loading

Fryguy commented Oct 6, 2020

miq-bot commented Oct 7, 2020

kbrock left a comment

Choose a reason for hiding this comment

kbrock Oct 8, 2020

Choose a reason for hiding this comment

kbrock Oct 8, 2020

Choose a reason for hiding this comment

NickLaMuro commented Oct 8, 2020 • edited Loading

gtanzillo commented Oct 8, 2020

NickLaMuro commented Sep 25, 2020 •

edited

Loading

lpichler commented Sep 30, 2020 •

edited

Loading

NickLaMuro commented Oct 6, 2020 •

edited by chessbyte

Loading

NickLaMuro commented Oct 8, 2020 •

edited

Loading