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

feat(MCSPAuthenticator): add new authenticator for Multi-Cloud Saas Platform #181

Merged
merged 1 commit into from
Nov 15, 2023

Conversation

padamstx
Copy link
Member

This commit introduces the new MCSPAuthenticator that can be used to exchange an apikey for an MCSP access token using the Multi-Cloud Saas Platform authentication token server's 'POST /siusermgr/api/1.0/apikeys/token' operation.

@padamstx padamstx self-assigned this Nov 10, 2023
@padamstx padamstx force-pushed the mcsp-authenticator branch 2 times, most recently from 167c676 to 2bc374f Compare November 10, 2023 17:32
Copy link
Member

@pyrooka pyrooka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good!

ibm_cloud_sdk_core/get_authenticator.py Outdated Show resolved Hide resolved
…latform

This commit introduces the new MCSPAuthenticator that can be used
to exchange an apikey for an MCSP access token using the Multi-Cloud Saas Platform
authentication token server's 'POST /siusermgr/api/1.0/apikeys/token' operation.

Signed-off-by: Phil Adams <[email protected]>
@pyrooka
Copy link
Member

pyrooka commented Nov 13, 2023

One question that's slightly related to this. Currently, when the AUTHTYPE is not specified we try to determine it based on the existence of the APIKEY: if it's found, then we use IAM, otherwise the container auth. However the CP4D and the MCSP authenticators also use apikey, so I am not sure if the current approach is quite correct. WDYT?

@dpopp07
Copy link
Member

dpopp07 commented Nov 13, 2023

if it's found, then we use IAM, otherwise the container auth

I think that's an intentional priority we built into the code based on which authenticators are most common in practice and I think the other cores will do the same thing (though I could be wrong on that).

It is worth considering how good of an experience that is because implicit behavior can often be a source of confusion for users but if we're doing this in every core, altering the behavior would be a breaking change as users could be relying on this implicit behavior.

@padamstx
Copy link
Member Author

padamstx commented Nov 13, 2023

I think that's an intentional priority we built into the code based on which authenticators are most common in practice and I think the other cores will do the same thing

correct, this is intentional, and we have the same logic in the other cores as well

the net result is... to use IAM, just configure the APIKEY config property at a minimum; to use one of the other authenticators that support APIKEY, you'd need to also set AUTH_TYPE to indicate which one to use.

@pyrooka
Copy link
Member

pyrooka commented Nov 13, 2023

Okay thanks, it makes sense!

Copy link
Member

@dpopp07 dpopp07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 👍

@padamstx padamstx merged commit 1be97e5 into main Nov 15, 2023
4 checks passed
@padamstx padamstx deleted the mcsp-authenticator branch November 15, 2023 18:00
ibm-devx-sdk pushed a commit that referenced this pull request Nov 15, 2023
# [3.18.0](v3.17.3...v3.18.0) (2023-11-15)

### Features

* **MCSPAuthenticator:** add new authenticator for Multi-Cloud Saas Platform ([#181](#181)) ([1be97e5](1be97e5))
@ibm-devx-sdk
Copy link

🎉 This PR is included in version 3.18.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

4 participants