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.

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

Fix openstack v3 authentication #744

Closed
wants to merge 2 commits into from

Conversation

@schaubl
Copy link
Contributor

schaubl commented Apr 12, 2016

Fix openstack v3 authentication

Description

This PR allows to define the OpenStack domain to another value that the default Default.
It also adds the ability to define the scope of the token.

With the code for the OpenStack Identity API v3, two new parameters were added: domain_name and token_scope but it was impossible to define them to a value other than their respective default.

Status

  • done, ready for review

Checklist (tick everything that applies)

  • Code linting (required, can be done after the PR checks)
  • Documentation
  • Tests
  • ICLA (required for bigger changes)
@Kami
Copy link
Member

Kami commented Apr 12, 2016

Great, thanks!

I will do a proper review later, but from a quick glance, it looks like it would be great to add some more tests to cover all the (edge) cases.

@schaubl
Copy link
Contributor Author

schaubl commented Apr 13, 2016

@Kami I don't know how to add tests for this fix because I need to check the body of the request but all tests seems to test the reply.

To test the body of the request I have to do it in OpenStackIdentity_3_0_MockHttp which is not a subclass of unittest.TestCase and doesn't have the assert... methods.

timeout=None, parent_conn=None):
super(OpenStackIdentityConnection, self).__init__(user_id=user_id,
key=key,
url=auth_url,
timeout=timeout)

self.auth_url = auth_url

This comment has been minimized.

@Kami

Kami Apr 13, 2016 Member

(just thinking out loud)

I assume you removed those attributes since they are duplicated aka are declared again a couple of lines below, right?

This comment has been minimized.

@schaubl

schaubl Apr 13, 2016 Author Contributor

Right

@@ -926,8 +929,7 @@ class OpenStackIdentity_3_0_Connection(OpenStackIdentityConnection):
]

def __init__(self, auth_url, user_id, key, tenant_name=None,
domain_name='Default',
token_scope=OpenStackIdentityTokenScope.PROJECT,
domain_name=None, token_scope=None,

This comment has been minimized.

@Kami

Kami Apr 13, 2016 Member

Any particular reason why you changed the default values? Doesn't this make it backward incompatible?

This comment has been minimized.

@schaubl

schaubl Apr 13, 2016 Author Contributor

Before this PR the only place where it is called wasn't passing these two parameters so their default value was the only possible value.
With the way this parameter is passed from the driver's __init__ to this method, it will be None if it is omitted.
So I changed the logic from

use the default value if it is not passed during the instanciation of OpenStackIdentity_3_0_Connection

(which is a case that cannot happens)
to

use the default value if it is None

This is also why I removed the if below.

raise ValueError('Must provide tenant_name and domain_name '
'argument')
elif (token_scope == OpenStackIdentityTokenScope.DOMAIN and

This comment has been minimized.

@Kami

Kami Apr 13, 2016 Member

Any particular reason you removed this? Shouldn't we still have this check here?

This comment has been minimized.

@Kami

Kami Apr 15, 2016 Member

Ah, thanks to for the clarification.

Also re to the comment above - I would prefer to specify default values for the arguments in the method signature instead of doing it in the method body.

Doing it in the body kinda defeats the purpose of default argument values and it also breaks things such as static code analysis (now if you want to know the default value you need to be aware of the implementation details and inspect the method body).

Would it be possible to change that back to assign default values in the method signature?

This comment has been minimized.

@schaubl

schaubl Apr 15, 2016 Author Contributor

Ok, I will try to do something about that.

This comment has been minimized.

@Kami

Kami Apr 15, 2016 Member

Great, thanks.

@schaubl
Copy link
Contributor Author

schaubl commented Apr 15, 2016

@Kami: Now default values are specified in the method signature.
I also added back a test case and the condition which check if domain_name is defined.

:type token_scope: ``str``
"""
super(OpenStackIdentity_3_0_Connection,
self).__init__(auth_url=auth_url,
user_id=user_id,
key=key,
tenant_name=tenant_name,
domain_name=domain_name,
token_scope=token_scope,
timeout=timeout,
parent_conn=parent_conn)

This comment has been minimized.

@schaubl

schaubl Apr 15, 2016 Author Contributor

I would suggest to change that code to:

def __init__(self, *args, **kwargs):
    # Docstring
    super(OpenStackIdentity_3_0_Connection, self).__init__(*args, **kwargs)

So the signature doesn't have to be specified twice.
But I know that kwargs are not appreciated, so tell me if you want me to do this change or not.

This comment has been minimized.

@tonybaloney

tonybaloney Apr 23, 2016 Contributor

@schaubl this is a clear case to use kwargs, its not that the project dictates not using kwargs but in the case where the keyword arguments can be listed they can.
this is fine as is.

@Kami
Copy link
Member

Kami commented Apr 23, 2016

@schaubl Thanks.

I will review the changes again and if everything looks good go ahead and merge them intro trunk.

@asfgit asfgit closed this in eb497fa Apr 23, 2016
asfgit pushed a commit that referenced this pull request Apr 23, 2016
@Kami
Copy link
Member

Kami commented Apr 23, 2016

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.