Refactor default required credential fields in authentication mixin #10690

yaacov · 2016-08-23T09:04:21Z

Description
Container provider manager does not implement the function required_credential_fields, it relays on a wrong implementation in authentication_mixin. This PR implements the required_credential_fields in the container provider and remove the bad implementation in authentication_mixin

Screenshot
Before

After

Issue
#10689

yaacov · 2016-08-23T09:06:01Z

@miq-bot add_label providers/containers

@simon3z @zeari please review

simon3z · 2016-08-23T09:12:20Z

app/models/mixins/authentication_mixin.rb

-    else
-      [:userid]
-    end
+    []


@yaacov why the previous code was added here in #7950? Why don't we need it anymore?

Is the test you're adding demonstrating that we don't need this in any case? (I suppose there was a reasoning to add this in #7950)

@simon3z

Why don't we need it anymore?

👍 thanks, fixed the authentication_mixin to return [:userid] like the current fallback.

We do need the functionality, the code in Container provider multi endpoint #7950 does it wrong and need fixing :-(

I suppose there was a reasoning to add this in #7950

The problem was that if the type was :hawkular we got [:userid], but the authentication did not have one, #7950 fixed it.

Why we need to this refactor/fix ?

The fix in #7950 did two things wrong

Type :hawkular should return [:auth_key] and not [], the same as type :bearer

The default answer for required_credential_fields in the case of container manager is [:auth_key] and not [:userid]

When writing #7950 I did not notice this problems because:
a. I always added a :userid => '_' thinking it was something we need to do, and not just an artefact.
b. I didn't know that other providers override this function when they want to, like openstack/infra_manager.

@simon3z

Is the test you're adding demonstrating that we don't need this in any case?

👍 Added a test to check that now we can not create an authentication type :hawkular without the :auth_key field, The test in the original version of this PR did not check that.

The new test will fail without the fix in this PR.

@yaacov So, do we need a similar override for hawkular or the default [:userid] will work for it?
/cc @abonas

@chessbyte
Authentication connected to a container manager provider use :auth_key,
Overriding the required_credential_fields method is nice here because it covers all the authentications, :bearer, :hawkular ( and nil, :default ... that should not be used here )

do we need a similar override for hawkular or the default will work for it?

hawkular will work. All authentications currently connected to a container manager require the [:auth_key] field.

@chessbyte @abonas

So, do we need a similar override for hawkular or the default [:userid] will work for it?

Sorry, I didn't understand the question.
The hawkular middleware use the :defaultauthentication type, it requires the [:unierid, :password] fields. it will work with the new default [:userid].

simon3z · 2016-08-23T09:42:07Z

@miq-bot assign yaacov

abonas · 2016-08-23T16:00:27Z

@rubenvp8510 please review this- to see that doesn't cause a regression in the hawkular provider auth. thank you

rubenvp8510 · 2016-08-25T04:15:55Z

I verified this on the hawkular provider, there is no regression. this looks good to me.

rubenvp8510 · 2016-08-25T04:17:11Z

app/models/mixins/authentication_mixin.rb

@@ -79,14 +79,7 @@ def authentication_service_account(type = nil)
  end

  def required_credential_fields(type)


I think the type parameter should be renamed _type because is not used.

@rubenvp8510 Thanks 👍 , how did rubocop miss that :-)

simon3z · 2016-08-25T08:11:49Z

I like how this turned out (cleaner code). 👍

@miq-bot assign chessbyte

yaacov · 2016-09-01T14:12:14Z

@miq-bot add_label bug

This is a bug as it prevents a legitimate API call {"action": "refresh"} from completing successfully.

simon3z · 2016-09-02T15:51:35Z

@blomquisg can we merge this?

yaacov · 2016-09-06T10:46:20Z

@blomquisg ping

miq-bot · 2016-09-06T19:51:30Z

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

blomquisg · 2016-09-06T19:57:18Z

@yaacov looks like container_manager_mixin is overriding one of the methods of authentication_mixin. Can you include a spec to ensure that a container_manager gets the correct implementation of required_credential_fields?

I just want to make sure that the inheritance chain never gets messed up there in the future.

yaacov · 2016-09-07T15:11:32Z

@blomquisg Thanks, add two specs to check the required_credential_fields, one for container_manager and one for miq_region.

in 5dd6746 a new authentication type "system_api" is introduced.
I re-factored it's required_credential_fields because I think it has the same problem as bearer, and this PR will fix it too.
@carbonin please review.

…tion When checking for required_credential_fields for a container manager authentication we get wrong fields Issue ManageIQ#10689

miq-bot · 2016-09-07T15:22:15Z

Checked commit yaacov@5e59040 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
5 files checked, 0 offenses detected
Everything looks good. 🍰

carbonin · 2016-09-07T15:52:16Z

Thanks, @yaacov

The region changes look good and the behavior looks to be the same for everything except hawkular. If that's been cleared then this looks good to me 👍

yaacov · 2016-09-07T16:13:31Z

the behavior looks to be the same for everything except hawkular.

@carbonin Thanks, I've added a spec for in spec/models/miq_region_spec.rb , if it's the right behaviour of miq_region than it's ok :-)

yaacov · 2016-09-08T08:17:24Z

Can you include a spec to ensure that a container_manager gets the correct implementation of required_credential_fields?

@blomquisg It already has an extensive spec for the regular cases [1] e.g. [:userid] , the added specs in this PR are for the the irregular cases of container_manger [2] and miq_region ( @carbonin ) [3]

[1] https://github.com/ManageIQ/manageiq/blob/master/spec/models/mixins/authentication_mixin_spec.rb#L59

[2] https://github.com/ManageIQ/manageiq/pull/10690/files#diff-5fa47523d0443aa7bedb8f5b6a19ef41L23
[3] https://github.com/ManageIQ/manageiq/pull/10690/files#diff-c55d10da4f0a175082d58a2a06b39aedL188

yaacov force-pushed the refactor-default-required-credential-fields-in-authentication-mixin branch from 06a3717 to 219ae4e Compare August 23, 2016 09:05

miq-bot added the providers/containers label Aug 23, 2016

simon3z reviewed Aug 23, 2016
View reviewed changes

yaacov force-pushed the refactor-default-required-credential-fields-in-authentication-mixin branch from 219ae4e to 2b44ac7 Compare August 23, 2016 09:34

miq-bot assigned yaacov Aug 23, 2016

yaacov force-pushed the refactor-default-required-credential-fields-in-authentication-mixin branch 3 times, most recently from 3afcb56 to fef33eb Compare August 23, 2016 10:24

rubenvp8510 reviewed Aug 25, 2016
View reviewed changes

yaacov force-pushed the refactor-default-required-credential-fields-in-authentication-mixin branch from fef33eb to 12b2cac Compare August 25, 2016 04:26

miq-bot assigned chessbyte and unassigned yaacov Aug 25, 2016

chessbyte added the refactoring label Aug 30, 2016

chessbyte assigned blomquisg and unassigned chessbyte Aug 30, 2016

miq-bot added the bug label Sep 1, 2016

yaacov mentioned this pull request Sep 4, 2016

Refresh container providers via api fails #7256

Closed

yaacov force-pushed the refactor-default-required-credential-fields-in-authentication-mixin branch 2 times, most recently from 6e1cd9c to 396d077 Compare September 7, 2016 12:36

yaacov closed this Sep 7, 2016

yaacov force-pushed the refactor-default-required-credential-fields-in-authentication-mixin branch from 396d077 to f94a597 Compare September 7, 2016 12:38

yaacov reopened this Sep 7, 2016

yaacov closed this Sep 7, 2016

yaacov reopened this Sep 7, 2016

yaacov force-pushed the refactor-default-required-credential-fields-in-authentication-mixin branch from 9c0337e to a7ca610 Compare September 7, 2016 14:58

Fix a false missing creadential error when checking bearer authentica…

5e59040

…tion When checking for required_credential_fields for a container manager authentication we get wrong fields Issue ManageIQ#10689

yaacov force-pushed the refactor-default-required-credential-fields-in-authentication-mixin branch from a7ca610 to 5e59040 Compare September 7, 2016 15:16

blomquisg merged commit 81170f7 into ManageIQ:master Sep 8, 2016

blomquisg added this to the Sprint 46 Ending Sep 12, 2016 milestone Sep 8, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor default required credential fields in authentication mixin #10690

Refactor default required credential fields in authentication mixin #10690

yaacov commented Aug 23, 2016 •

edited by miq-bot

yaacov commented Aug 23, 2016

simon3z Aug 23, 2016

yaacov Aug 23, 2016 •

edited by chessbyte

yaacov Aug 23, 2016

chessbyte Aug 23, 2016

yaacov Aug 23, 2016

yaacov Aug 23, 2016

simon3z commented Aug 23, 2016

abonas commented Aug 23, 2016

rubenvp8510 commented Aug 25, 2016 •

edited

rubenvp8510 Aug 25, 2016

yaacov Aug 25, 2016

simon3z commented Aug 25, 2016

yaacov commented Sep 1, 2016

simon3z commented Sep 2, 2016

yaacov commented Sep 6, 2016

miq-bot commented Sep 6, 2016

blomquisg commented Sep 6, 2016

yaacov commented Sep 7, 2016

miq-bot commented Sep 7, 2016

carbonin commented Sep 7, 2016

yaacov commented Sep 7, 2016

yaacov commented Sep 8, 2016

		@@ -79,14 +79,7 @@ def authentication_service_account(type = nil)
		end

		def required_credential_fields(type)

Refactor default required credential fields in authentication mixin #10690

Refactor default required credential fields in authentication mixin #10690

Conversation

yaacov commented Aug 23, 2016 • edited by miq-bot

yaacov commented Aug 23, 2016

simon3z Aug 23, 2016

Choose a reason for hiding this comment

yaacov Aug 23, 2016 • edited by chessbyte

Choose a reason for hiding this comment

yaacov Aug 23, 2016

Choose a reason for hiding this comment

chessbyte Aug 23, 2016

Choose a reason for hiding this comment

yaacov Aug 23, 2016

Choose a reason for hiding this comment

yaacov Aug 23, 2016

Choose a reason for hiding this comment

simon3z commented Aug 23, 2016

abonas commented Aug 23, 2016

rubenvp8510 commented Aug 25, 2016 • edited

rubenvp8510 Aug 25, 2016

Choose a reason for hiding this comment

yaacov Aug 25, 2016

Choose a reason for hiding this comment

simon3z commented Aug 25, 2016

yaacov commented Sep 1, 2016

simon3z commented Sep 2, 2016

yaacov commented Sep 6, 2016

miq-bot commented Sep 6, 2016

blomquisg commented Sep 6, 2016

yaacov commented Sep 7, 2016

miq-bot commented Sep 7, 2016

carbonin commented Sep 7, 2016

yaacov commented Sep 7, 2016

yaacov commented Sep 8, 2016

yaacov commented Aug 23, 2016 •

edited by miq-bot

yaacov Aug 23, 2016 •

edited by chessbyte

rubenvp8510 commented Aug 25, 2016 •

edited