Reinstate token precedence over password in AbstractRestClient #2119
Conversation
Signed-off-by: Gene Johnston <Eugene.Johnston@broadcom.com>
gejohnston
requested review from
t1m0thyj,
zFernand0,
awharn,
traeok and
adam-wolfe
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2119 +/- ##
==========================================
+ Coverage 91.05% 91.12% +0.06%
==========================================
Files 636 636
Lines 19004 19035 +31
Branches 3997 4009 +12
==========================================
+ Hits 17305 17346 +41
+ Misses 1698 1688 -10
Partials 1 1 ☔ View full report in Codecov by Sentry. |
| * @memberof AbstractRestClient | ||
| */ | ||
| constructor(private mSession: AbstractSession) { | ||
| constructor(private mSession: AbstractSession, credOrder = ICredOrder.TOKEN_OVER_PASSWORD) { |
There was a problem hiding this comment.
without much thought, I "assumed" this option would have been implemented here: https://github.com/zowe/zowe-cli/blob/master/packages/imperative/src/rest/src/session/doc/ISession.ts
do you think this is simply an alternative place to add this new option?
There was a problem hiding this comment.
AbstractRestClient is only called from one file (RestClient). Thus, the correction can be made in 2 existing files (and 1 new file). No other zowe code should see any potential change or capabilities. It would just correct something that was broken.
ISession is used in over a dozen other Zowe files. I am a little concerned about how widely visible this change would become.
We have had requests to officially change the credential order of precedence. To do that properly, we may have to introduce the ability to control the order of password, token, certificate, and Personal-Access-Token, (and maybe something else?) Further, we may have to define a way for end-users to set the precedence differently for different sessions. We have not thought through these requirements well enough to start down any particular implementation path.
I suppose that by making a new ISession property optional, it would not break other functions. Further, if RestClient ALWAYS sets that property, and AbstractRestClient is the only class to read that property, it could just as effectively revert the breaking change and limit the change to only the broken class.
Your comment has made me think about making an ISession property more complex so that it might accommodate more than just password and token. A more complex property might provide the means to control the order of more credential types in the future, even though we would only implement password and token now.
I will try out some of my thoughts and update this pull request if they pan out.
There was a problem hiding this comment.
Update: The only way to make this alternative non-breaking is for the AbstractRestClient constructor to Always set the Session property to prefer "token". Otherwise a direct caller of AbstractRestClient would have to set the new ISession property before calling AbstractRestClient.constructor. The RestClient already overrides this AbstractRestClient behavior to prefer password, so this should be ok.
There was a problem hiding this comment.
Hi @dkelosky - I made revisions as described above. Please re-review.
There was a problem hiding this comment.
Hey @gejohnston, thanks for considering this approach. It feels a little more natural to me. Hopefully others feel the same 😃
Signed-off-by: Gene Johnston <Eugene.Johnston@broadcom.com>
There was a problem hiding this comment.
Thanks for separating the logic for authentication methods into different functions, this is easier to read and keeps code complexity at a minimum.
I noticed some tests are failing in AbstractRestClient, but the solution itself looks good to me! This implementation should also take things a step forward to allow for custom authentication precedence in the future.
Signed-off-by: Gene Johnston <Eugene.Johnston@broadcom.com>
Signed-off-by: Gene Johnston <Eugene.Johnston@broadcom.com>
Signed-off-by: Gene Johnston <Eugene.Johnston@broadcom.com>
Signed-off-by: Gene Johnston <Eugene.Johnston@broadcom.com>
There was a problem hiding this comment.
LGTM, thanks @gejohnston!
AFAICT this should fix the potential breaking change that was reported, and I like the way it sets us up for supporting user-defined auth type order in the future.
When we're ready to build a CLI TGZ for others to test, we should be able to use the npm run package script to bundle the Imperative package changes in this PR 😋
Signed-off-by: Gene Johnston <Eugene.Johnston@broadcom.com>
Signed-off-by: Gene Johnston <Eugene.Johnston@broadcom.com>
|
gejohnston
added
the
release-patch
|
Release succeeded for the The following packages have been published:
Powered by Octorelease 🚀 |
What It Does
Modified this description to reflect changes made in response to reviews.
The AbstractRestClient class was originally released with logic that chose a token over user and password. Other commonly used Zowe SDK APIs enforce the order back to PASSWORD_OVER_TOKEN. Consumers which directly extended AbstractRestClient came to rely on the unintended order of TOKEN_OVER_PASSWORD. Later changes in this class to adhere to the Zowe policy of PASSWORD_OVER_TOKEN inadvertently broke such extenders. Until a means is provided for consumers (and/or end-users) to customize their credential order of precedence, we now revert the behavior of AbstractRestClient of back to TOKEN_OVER_PASSWORD.
A new array named authTypeOrder was added to ISession. It contains the order of precedence for authentication types. It can provide a building block for a future, customizable order of precedence. For now, that array is hard-coded in the AbstractRestClient constructor. Logic in another function within AbstractRestClient uses the order of that array to determine the type of authentication to use.
Related issue: Breaking Change in User/Password/Token Order of Precedence
How to Test
Zowe CLI and ZE cannot be used to test the TOKEN_OVER_PASSWORD option. Too many up-stream safeguards ensure that using a token is not even a possibility when user and password are present. We would have to write a specialized test app that confirms that a token can be given precedence over password. Even then we would only confirm that our test app works. I think that everyone is better served if the consuming team confirms that the modifications address their needs.
Therefore, this pull request has been opened as a draft. I propose that we take the following actions:
Review Checklist
I certify that I have:
Additional Comments