-
Notifications
You must be signed in to change notification settings - Fork 897
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
User login scoping (API) #20506
User login scoping (API) #20506
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice changes
I am a little concerned about caching users and keeping them in memory for development purposes.
Also curious how this would do in production.
there are probably not very many scopes, so in terms of memory, this seems fine to me. I do wonder about storing default scope for almost every user. seems that a fallback may be the same without all the scope storage
A little confused what the first level of lookup scope buys us. But that is probably because I don't see too many examples of this besides the api.
app/models/authenticator/base.rb
Outdated
def lookup_by_identity(username, *_args) | ||
case_insensitive_find_by_userid(username) | ||
def lookup_by_identity(username, request: nil, lookup_scope: nil) | ||
@lookup ||= {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the lifespan of this cache?
do we need to bother with a timeout mechanism?
or will it break code reloading in development?
or could we pass the hash in? not sure how that would worm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the lifespan of this cache?
Just for the duration of the request. This method was called multiple times as part of the Authenticator::Base#authenticate
method, so was hoping to avoid redundant lookups. That said, I put it in a "hash" incase it was ever passed different args, and I didn't want to return an invalid cached result if it wasn't the same as was requested originally.
@@ -200,8 +199,10 @@ def authenticate_with_http_basic(username, password, request = nil, options = {} | |||
[!!result, username] | |||
end | |||
|
|||
def lookup_by_identity(username, *_args) | |||
case_insensitive_find_by_userid(username) | |||
def lookup_by_identity(username, request: nil, lookup_scope: nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure the advantage of this method.
do both this class and case_insensitive_find
use the same caching lookup?
Think they can be merged? (I'm probably not seeing a character change or something in there)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I was kinda confused about this as well. I decided not to address that in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me try again.
you added a cache here
you added a cache in the case_insensitive_find_by_userid
can we get away with making this method just the call into case_insensitive_find_by_userid
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol. ^ idiot. totally didn't process your response.
LGTM
b8e8f2e
to
06c71cc
Compare
app/models/user.rb
Outdated
# Where :default_scoped is the value for anything that doesn't match a key in | ||
# the original array. The "lookup hash" was chossen since it is much faster | ||
# to do a hash table lookup than a `Array.includes?(KEY)`. | ||
LOOKUP_SCOPES = [:api_includes].inject(Hash.new { :default_scoped }) {|h,i| h[i] = i; h}.freeze |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a little cleaner, personally...wdyt?
LOOKUP_SCOPES = Hash.new(:default_scoped).merge([:api_includes].index_by(&:itself))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, if you don't really need the Array thing...
LOOKUP_SCOPES = Hash.new(:default_scoped).merge(:api_includes => :api_includes)
Apologies for the late reply...was actually looking at the follow up PRs and realized I never commented on the base PR. |
@Fryguy no worries, though this is probably going to take a back seat since I don't really have a need for this at the moment, and I pushed it off into ManageIQ/manageiq-api#889 as well. Also... I really don't want to bother with rebasing it at the moment... |
06c71cc
to
14bf42f
Compare
8f55dae
to
734234b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a big fan of these changes.
Looks like the failure may be legit though
734234b
to
f0d7cb8
Compare
Okay, I wouldn't mind waiting until CI does pass, but I think this failure is from Libor's changes (and by extension, @kbrock ): https://travis-ci.com/github/ManageIQ/manageiq/jobs/474546624#L791-L802 So I will wait to see if master starts passing before I try again. |
A scope for eager loading `current_user` associated records that will be used in the process of authenticating on every request. Will be used in subsequent commits.
f0d7cb8
to
e00448c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think most of the rubocop complaints are that we are using colon hashes instead of hash rockets.
you going to address all the hashrockets - or you want to ship as is ruby 3.0 does need colons and not hash rockets |
And applies them to the `Authenticator` models. This allows components of the application (like the API) to pass specific scopes for the user that is being authenticated to include so they can be loaded in one fell swoop, but only ones we allow via the LOOKUP_SCOPES constant. This also changes a lot of the `lookup_by_*` methods to use kwargs to simplify the interface a bit, and support the new `:lookup_scope` argument.
e00448c
to
bb58c70
Compare
Checked commits NickLaMuro/manageiq@54b48d3~...bb58c70 with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint app/models/authenticator/base.rb
app/models/authenticator/database.rb
app/models/authenticator/httpd.rb
app/models/authenticator/ldap.rb
app/models/user.rb
spec/models/authenticator/httpd_spec.rb
|
@kbrock All of these are for method arguments that are
and I agree. Also, as you said (I guess...)
Which is the only place these This is also a style for manageiq/lib/ansible/runner.rb Line 15 in 72fa6ef
So I am not doing anything non-standard here. |
I wonder if rubocop has a way to differentiate Hashes and keyword args (particularly because of the upcoming 3.0 changes) |
This pull request is not mergeable. Please rebase and repush. |
This pull request has been automatically closed because it has not been updated for at least 3 months. Feel free to reopen this pull request if these changes are still valid. Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation. |
cc @kbrock |
This pull request is not mergeable. Please rebase and repush. |
This pull request has been automatically closed because it has not been updated for at least 3 months. Feel free to reopen this pull request if these changes are still valid. Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation. |
NOTE: Currently built on top of #20471(though, this probably could be done without, but worth noting the Benchmarks included those changes)A collection of tweaks to the
User
andAuthenticator::*
models to enable more effecient querying ofUser
and related modelsRequires
manageiq-api
changes to take effect: ManageIQ/manageiq-api#888Benchmarks
Note: Improvements are in number of queries.
Before
After
Links