New Authentication mechanism #21

ehdsouza · 2019-08-16T21:51:08Z

This PR supports

the new authentication mechanism, an example:

from ibm_cloud_sdk_core.authenticators import BasicAuthenticator

assistant = AssistantV1(version='2018-07-10', 
                                        authenticator=BasicAuthenticator('<username>', '<password>'))

removed older python versions from travis
moved all code for get_authenticator_from_environment in utils file
The base service has request is broken up into: prepare_request and send. So the generator would be updated to:

        request = self.prepare_request(
            method='GET',
            url=url,
            headers=headers,
            params=params,
            accept_json=True)
        response = self.send(request)

Also updated the design doc: https://github.ibm.com/CloudEngineering/openapi-sdkgen/wiki/Core-SDK-Authentication-Design

codecov · 2019-08-16T21:51:59Z

Codecov Report

Merging #21 into next will increase coverage by 2%.
The diff coverage is 97.81%.

@@           Coverage Diff           @@
##             next      #21   +/-   ##
=======================================
+ Coverage   95.75%   97.76%   +2%     
=======================================
  Files           9       16    +7     
  Lines         448      492   +44     
=======================================
+ Hits          429      481   +52     
+ Misses         19       11    -8

Impacted Files	Coverage Δ
...oud_sdk_core/authenticators/basic_authenticator.py	`100% <100%> (ø)`
...loud_sdk_core/authenticators/cp4d_authenticator.py	`100% <100%> (ø)`
ibm_cloud_sdk_core/iam_token_manager.py	`100% <100%> (ø)`	⬆️
..._core/authenticators/bearer_token_authenticator.py	`100% <100%> (ø)`
ibm_cloud_sdk_core/jwt_token_manager.py	`97.72% <100%> (-0.32%)`	⬇️
ibm_cloud_sdk_core/authenticators/__init__.py	`100% <100%> (ø)`
ibm_cloud_sdk_core/cp4d_token_manager.py	`100% <100%> (ø)`
ibm_cloud_sdk_core/__init__.py	`100% <100%> (ø)`	⬆️
...d_sdk_core/authenticators/no_auth_authenticator.py	`100% <100%> (ø)`
ibm_cloud_sdk_core/authenticators/authenticator.py	`66.66% <66.66%> (ø)`
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed2147a...cec3609. Read the comment docs.

dpopp07 · 2019-08-16T22:03:36Z

Haven't looked at the code yet, but have a couple questions about the release strategy:

I see you're merging this into master, so will that trigger a release? Isn't this a breaking change? If this is going into the major release candidate for the core, perhaps it should go into a next branch or something like that.

That is, unless this is the only change you want to go into the next major release. If that's the case, we can just do the major release with this PR like @padamstx did with Java.

Either way, shouldn't one of the commit messages have the BREAKING CHANGE tag on it?

ehdsouza · 2019-08-16T22:20:29Z

@dpopp07 I changed the base to next as I would be making more minor changes like updating the request library. Together, Ill merge it as an official major 1.0.0 release

dpopp07

This looks pretty good but needs some changes to adhere to the design and stay consistent with the other SDKs

ibm_cloud_sdk_core/authenticators/__init__.py

ibm_cloud_sdk_core/authenticators/basic_authenticator.py

ibm_cloud_sdk_core/authenticators/bearer_authenticator.py

ibm_cloud_sdk_core/authenticators/cp4d_authenticator.py

ibm_cloud_sdk_core/authenticators/iam_authenticator.py

ibm_cloud_sdk_core/base_service.py

ibm_cloud_sdk_core/authenticators/authenticator.py

ibm_cloud_sdk_core/base_service.py

padamstx · 2019-08-16T22:26:12Z

@ehdsouza I haven't had a chance to look at the changes in detail yet, but could you add python code examples to the design wiki.
That will likely help us a bit during the review, plus it would be good to have examples for all the languages on the wiki :)

ibm_cloud_sdk_core/authenticators/iam_authenticator.py

mediumTaj

Looks good, I think Dustin covered everything :)

One question about making the authenticate function consistent.

ibm_cloud_sdk_core/authenticators/basic_authenticator.py

ehdsouza

@dpopp07 @mediumTaj thanks for the review comments. Ill make the necessary changes.
@padamstx Ill update the doc with examples shortly

padamstx

I left a bunch of comments pointing out where things deviate from the design.
Please read through the details of the design wiki (https://github.ibm.com/CloudEngineering/openapi-sdkgen/wiki/Core-SDK-Authentication-Design) and update the code to comply with that as much as you can.
Thanks

ibm_cloud_sdk_core/authenticators/authenticator.py

ibm_cloud_sdk_core/authenticators/basic_authenticator.py

ibm_cloud_sdk_core/authenticators/bearer_authenticator.py

ibm_cloud_sdk_core/authenticators/no_auth_authenticator.py

ibm_cloud_sdk_core/authenticators/__init__.py

ibm_cloud_sdk_core/authenticators/iam_authenticator.py

ibm_cloud_sdk_core/base_service.py

padamstx · 2019-08-19T16:20:57Z

ibm_cloud_sdk_core/base_service.py


        # 2. Credentials from credential file
-        if display_name and not self.username and not self.token_manager:
+        if display_name and not self.authenticator:
            service_name = display_name.replace(' ', '_').lower()
            self._load_from_credential_file(service_name)


Nope, we don't do this from BaseService any more.

dpopp07

Looking good but I have a few more changes to request

README.md

dpopp07 · 2019-08-20T20:42:20Z

ibm_cloud_sdk_core/authenticators/iam_authenticator.py

+        self.token_manager = IAMTokenManager(
+            apikey, url, client_id, client_secret, disable_ssl_verification,
+            headers, proxies)
+        self.validate()


I wonder if you should validate before you create the token manager, so that you're not passing a null value for apikey if none is given. Probably doesn't make a big difference though

ibm_cloud_sdk_core/authenticators/iam_authenticator.py

ibm_cloud_sdk_core/base_service.py

ibm_cloud_sdk_core/cp4d_token_manager.py

ibm_cloud_sdk_core/iam_token_manager.py

ibm_cloud_sdk_core/utils.py

padamstx

LGTM; I could only find one little typo to complain about :)

padamstx · 2019-08-22T14:48:14Z

ibm_cloud_sdk_core/base_service.py

-        2) Credentials loaded from credentials file if present
-        3) Credentials loaded from VCAP_SERVICES environment variable if available and use_vcap_services is True
+        :attr str url: The url for service api calls
+        :attr Athenticator authenticator: The authenticator for authentication


s/Athenticator/Authenticator/ :)

I think this still needs to be fixed!

dpopp07

A couple small docs items to address still but otherwise, looks good!

dpopp07 · 2019-08-22T15:18:28Z

ibm_cloud_sdk_core/base_service.py

-        2) Credentials loaded from credentials file if present
-        3) Credentials loaded from VCAP_SERVICES environment variable if available and use_vcap_services is True
+        :attr str url: The url for service api calls
+        :attr Athenticator authenticator: The authenticator for authentication


I think this still needs to be fixed!

dpopp07 · 2019-08-22T15:23:31Z

ibm_cloud_sdk_core/iam_token_manager.py

@@ -81,5 +90,23 @@ def set_iam_authorization_info(self, iam_client_id, iam_client_secret):
        If these values are not supplied, then a default Authorization header
        is used.


Last little docs change, there is no default header anymore

dpopp07 · 2019-08-22T15:24:42Z

ibm_cloud_sdk_core/utils.py

+    if config:
+        authenticator = contruct_authenticator(config)
+
+    # 3. From env variables


Small typo

Suggested change

# 3. From env variables

# 2. From env variables

mediumTaj

👍 looks good!

dpopp07

Changes need to be made now that we are changing the design

README.md

ibm_cloud_sdk_core/authenticators/no_auth_authenticator.py

ibm_cloud_sdk_core/utils.py

resources/ibm-credentials-no-auth.env

dpopp07

Looks good! 👍

watson-github-bot · 2019-10-03T16:31:55Z

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

ehdsouza added 2 commits August 16, 2019 17:42

feat(authentication): Revanp to new authentication mechanism

b878a49

chore(travis): remove older versions of python

7c969c8

ehdsouza added the ready for review label Aug 16, 2019

ehdsouza requested review from mediumTaj, dpopp07 and padamstx August 16, 2019 21:51

ehdsouza changed the title ~~Auth~~ New Authentication mechanism Aug 16, 2019

ehdsouza changed the base branch from master to next August 16, 2019 22:19

dpopp07 suggested changes Aug 16, 2019

View reviewed changes

dpopp07 reviewed Aug 16, 2019

View reviewed changes

ibm_cloud_sdk_core/authenticators/iam_authenticator.py Outdated Show resolved Hide resolved

mediumTaj suggested changes Aug 18, 2019

View reviewed changes

ibm_cloud_sdk_core/authenticators/basic_authenticator.py Outdated Show resolved Hide resolved

ehdsouza commented Aug 19, 2019

View reviewed changes

padamstx requested changes Aug 19, 2019

View reviewed changes

ehdsouza added 5 commits August 20, 2019 10:10

feat(authenticator): Refactor autethnticators

bf7da48

fix(base service): Separate out request into prepare_request and send

8a95b89

chore(token test): Handle tokens for all python versions

52d6108

feat(config): get_authenticator_from_environment for loading from env

c50d2bb

doc(readme): Update readme supporting auth methods

b12274e

ehdsouza requested review from padamstx, dpopp07 and mediumTaj August 20, 2019 20:04

dpopp07 suggested changes Aug 20, 2019

View reviewed changes

ehdsouza added 3 commits August 21, 2019 11:22

chore(auth): Changes as per Dustin's comments

c7fcb3e

chore(imports): remove unused imports

e67e35a

feat(env): Load from env variables

866f279

ehdsouza requested a review from dpopp07 August 21, 2019 23:19

fix(value error): Throw error if authenticator is not set

795c630

padamstx approved these changes Aug 22, 2019

View reviewed changes

dpopp07 suggested changes Aug 22, 2019

View reviewed changes

doc(typos): Correct typos by silly me

e89e1a1

mediumTaj approved these changes Aug 22, 2019

View reviewed changes

chore(rename): rename noath and iam authenticators to camel case

9054fa9

dpopp07 suggested changes Aug 22, 2019

View reviewed changes

README.md Show resolved Hide resolved

ibm_cloud_sdk_core/authenticators/no_auth_authenticator.py Outdated Show resolved Hide resolved

ibm_cloud_sdk_core/utils.py Outdated Show resolved Hide resolved

resources/ibm-credentials-no-auth.env Outdated Show resolved Hide resolved

ehdsouza added 2 commits August 22, 2019 14:28

chore(noAuth): changes as per new naming

c488ace

chore(noAuth): Consistent naming of noAuth value

cec3609

dpopp07 approved these changes Aug 26, 2019

View reviewed changes

germanattanasio requested a review from lpatino10 August 26, 2019 21:03

ehdsouza merged commit 7eb7e88 into next Aug 27, 2019

ehdsouza deleted the auth branch August 27, 2019 13:48

watson-github-bot added the released label Oct 3, 2019

		@@ -81,5 +90,23 @@ def set_iam_authorization_info(self, iam_client_id, iam_client_secret):
		If these values are not supplied, then a default Authorization header
		is used.

New Authentication mechanism #21

New Authentication mechanism #21

Uh oh!

Conversation

ehdsouza commented Aug 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dpopp07 commented Aug 16, 2019

Uh oh!

ehdsouza commented Aug 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dpopp07 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

padamstx commented Aug 16, 2019

Uh oh!

Uh oh!

mediumTaj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ehdsouza left a comment

Choose a reason for hiding this comment

Uh oh!

padamstx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

padamstx Aug 19, 2019

Choose a reason for hiding this comment

Uh oh!

dpopp07 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dpopp07 Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

padamstx left a comment

Choose a reason for hiding this comment

Uh oh!

padamstx Aug 22, 2019

Choose a reason for hiding this comment

Uh oh!

dpopp07 Aug 22, 2019

Choose a reason for hiding this comment

Uh oh!

dpopp07 left a comment

Choose a reason for hiding this comment

Uh oh!

ehdsouza commented Aug 16, 2019 •

edited

Loading

codecov bot commented Aug 16, 2019 •

edited

Loading

ehdsouza commented Aug 16, 2019 •

edited

Loading