Allow user to manage user pools by maybelinot · Pull Request #4 · openshift-qe/ownthat

maybelinot · 2016-10-24T13:53:55Z

@akostadinov can you review changes please?
one question: Would it be better to update only those values which were provided with request? For example if :note parameter was skipped then ownthat will not rewrite its value on empty, WDYT?

PS: will add unit tests soon

akostadinov · 2016-10-24T16:10:25Z

+      update_params = pool_params
+      update_params[:active] = update_params.has_key?(:active) &&
+                               update_params[:active] == "true"
+      if @pool.update(update_params)


I figured this can raise errors if database returns an error for example for violating index. See #3
Could you review #3?

akostadinov · 2016-10-24T16:13:40Z

+      end
+    else
+      # for non-admin allow only updating active state to prevent
+      # pools stealing and other mistakes


I am not really sure this logic is even necessary here. See my comment about auth.

akostadinov · 2016-10-24T16:16:52Z

      if params["controller"] == "locks" &&
-          ["create", "update", "lock_from_pool"].include?(params["action"])
+          ["create", "update", "lock_from_pool"].include?(params["action"]) ||
+         params["controller"] == "pools" &&


I don't think we should allow user to modify the pools. Should we? The most we should allow user IMO is to disable an entry (as a separate safe action). But any other pools modifications should be only allowed to the master user as far as I can tell.
This would ensure user can at most do a DoS attack but not mess up everything.

I am not against having API access to modify pools in all possible ways. It just makes more sense to me to do that with master auth.

Totally agreed. Added separate method to just disable pool, will modify update method so API could be used to update any info with master credentials..

akostadinov · 2016-10-24T16:19:40Z

Hi, it's nice that you look at this. See my line comments.

wrt your suggestion, the requirement to pass all params to locks was for some safety checking to avoid stealing locks from each other.
For pools, and especially if we only allow master to modify pools, then updating individual fields is fine with me. That means, if field is not present in the request, then do not modify it. Not important feature for me but generally makes sense.

maybelinot · 2016-10-25T09:54:54Z

  post '/locks/from_pool(.:format)' => 'locks#lock_from_pool', as: "create_from_pool"

+  match '/pools/disable(.:format)' => 'pools#disable', via: [:put, :patch], as: "disable_pool"
  resources :pools


@akostadinov I was confused here a bit, when I'm changing the order of these two lines request on /pools/disable redirected to pools#update method..

Will need more time to become familiar with all RoR stuff..

Probably order of precedence. Because put and patch are used for update usually. You need to list routes with both orders to see what's the difference. bin/rails routes

@akostadinov Yeah, exactly. Just was confused that it works for locks fine, but in locks you changing the same update method..

maybelinot · 2016-10-25T09:56:44Z

Do you have any proposal how to make deactivation of pools on the CucuShift side?

akostadinov · 2016-10-28T20:11:42Z

If I understand the question correctly, if login fails, then we can make user manages issue a disable call. Not sure if it will be straightforward to catch this when trying to use user. If not, we can use the clean-up in user manager to disable any users that cannot be cleaned-up properly. This btw makes sense anyway. If we fail to clean-up user, then it might be invalid.
But wait! I think we need to disable users only by a very long lock. One pool for example some ldap pool might be used by different environments. So it doesn't make sense to disable a user just because it doesnt work in a single environment.
Alternatively we may have a parameter for the user manager that will tell it to disable or not disable a pool entry.
But I think a very long lock is a safer choice. Perhaps we can use some special time value to identify disabled users.

maybelinot · 2016-11-07T12:32:48Z

@akostadinov Actually I wanted to deactivate only tokens, because user:password could be function in different envs at the same time and does not have any fixed time to expire, when :tokens are unique, if I get it right..
OpenShift give us pretty distinct error message: "error: The token provided is invalid or expired."
Didn't get why should we disable very long locks..

Can you review changes in this PR? Let me know if anything need to be fixed!

akostadinov · 2016-11-10T07:28:21Z

@maybelinot , just recently you added ldap pool. With LDAP, same pool can be used for multiple environments. If for some reason a user becomes broken in one env and we want it disabled, but if we disable the pool entry, then we will effectively disable it for all of the environments using that LDAP pool.

That's why having a long lock will allow locking for particular env only. Does this make more sense to you now?

P.S. and that's why I suggested call to disable pool entry to be allowed only for admin

maybelinot · 2016-11-10T10:38:36Z

@akostadinov Yeah, now it makes more sense.. But I expected this to be more kind of helpful or .. I don't know..
Because if we have a resource, which works for many envs, then if we will lock it for a long time it could mean only:

user pick wrong pool and lock entry for his env (in this case locking doesn't make much sense to me)
user pick right pool, but some(all) of the pool entries wasn't valid for his env, because he forget to add users or smth like that.. (in this case he will need to add users to env and manually/with API delete lock)

And third option is yours(this one would be the most infrequent, I think):

for some reason a user becomes broken in one env

I would suggest to add another parameter for pool:

namespace: string (link(dev_preview_int) or any or other)
or
multienv: boolean

And lock only pools with namespace!=any.. Because at least for online envs this is much easier to see which tokens are expired(if pool deactivated) and update them.. But this crazy idea in my mind caused by personal tokens, if we would have auto generated users then we could even delete pool entry with invalid tokens..

akostadinov · 2016-11-10T21:12:15Z

If idea is to catch invalid pools (or incomplete env user setup), then doing disable operations would make little to no sense.
The only use case where updating makes sense is when user is rendered unusable.

But I see your primary use case is to detect expired tokens in pools with tokens. But how do you know if token expired or you used wrong pool?

How about including the config in pool specification. e.g. pool:shared:poolname and pool:dedicated:poolname (not sure which one should be default)
In this way manager would know which disable strategy to use. WDYT?

akostadinov · 2016-11-15T17:25:58Z

Lets discuss this pull in a short meeting.

akostadinov · 2017-03-21T17:27:43Z

+      # concurrency issue or not matching name/resource specification
+      @persisted = false
+      @errors = ["conflict with existing pool or not matching name/resource"]
+      render status: :conflict


I think that this condition can only happen when pool entry is not found. Perhaps change to :not_found and the respective @errors message. I don't see any possibility to be a conflict. Probably using this update approach instead of normal Rails update is an overkill. But I think all we need is nod_found and a proper @error.

akostadinov · 2017-03-21T17:31:55Z

One question - you want to disable pool entries from cucushift automatically or just have a call to run manually?

maybelinot · 2017-03-21T17:54:02Z

I imagine this to be automatically

akostadinov reviewed Oct 24, 2016

View reviewed changes

maybelinot force-pushed the master branch from 738d701 to 4fdbbd1 Compare October 25, 2016 09:46

maybelinot commented Oct 25, 2016

View reviewed changes

allow user to update user pools

1d5b777

maybelinot force-pushed the master branch from 4fdbbd1 to 1d5b777 Compare November 7, 2016 12:14

maybelinot changed the title ~~[WIP] Allow user to manage user pools~~ Allow user to manage user pools Nov 7, 2016

akostadinov reviewed Mar 21, 2017

View reviewed changes

Uh oh!

Conversation

maybelinot commented Oct 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akostadinov commented Oct 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maybelinot commented Oct 25, 2016

Uh oh!

akostadinov commented Oct 28, 2016

Uh oh!

maybelinot commented Nov 7, 2016

Uh oh!

akostadinov commented Nov 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maybelinot commented Nov 10, 2016

Uh oh!

akostadinov commented Nov 10, 2016

Uh oh!

akostadinov commented Nov 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akostadinov commented Mar 21, 2017

Uh oh!

maybelinot commented Mar 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

maybelinot commented Oct 24, 2016 •

edited

Loading

akostadinov commented Nov 10, 2016 •

edited

Loading