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

Adding the ability to switch between basic authentication and token-based authentication #2944

Merged
merged 48 commits into from
Jul 17, 2024

Conversation

SanthoshiBoyina1
Copy link
Contributor

Proposed changes

To add the ability to switch between basic authentication and token-based authentication.

Release Notes

Milestone:

Changelog:

Types of changes

What types of changes does your code introduce to Zowe Explorer?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Updates to Documentation or Tests (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This checklist will be used as reference for both the contributor and the reviewer

  • I have read the CONTRIBUTOR GUIDANCE wiki
  • PR title follows Conventional Commits Guidelines
  • PR Description is included
  • gif or screenshot is included if visual changes are made
  • yarn workspace vscode-extension-for-zowe vscode:prepublish has been executed
  • All checks have passed (DCO, Jenkins and Code Coverage)
  • I have added unit test and it is passing
  • I have added integration test and it is passing
  • There is coverage for the code that I have added
  • I have tested it manually and there are no regressions found
  • I have added necessary documentation (if appropriate)
  • Any PR dependencies have been merged and published (if appropriate)

Further comments

SanthoshiBoyina1 and others added 25 commits April 22, 2024 22:43
Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
Copy link

codecov bot commented Jun 10, 2024

Codecov Report

Attention: Patch coverage is 96.62921% with 3 lines in your changes missing coverage. Please review.

Project coverage is 93.47%. Comparing base (adf0f28) to head (8457c99).

Files Patch % Lines
packages/zowe-explorer/src/Profiles.ts 97.59% 2 Missing ⚠️
...kages/zowe-explorer/src/utils/ProfileManagement.ts 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2944      +/-   ##
==========================================
- Coverage   93.54%   93.47%   -0.07%     
==========================================
  Files         104      104              
  Lines       10889    10978      +89     
  Branches     2293     2400     +107     
==========================================
+ Hits        10186    10262      +76     
- Misses        702      715      +13     
  Partials        1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

traeok
traeok previously approved these changes Jul 15, 2024
adam-wolfe
adam-wolfe previously approved these changes Jul 15, 2024
t1m0thyj
t1m0thyj previously approved these changes Jul 15, 2024
Copy link
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @SanthoshiBoyina1!

Tested this PR with a config file that had user/password in base profile. Switching first time stored token in base profile, and then switching back stored user/password directly on service profile. Although this is different from the original state of my config file, I think it is expected behavior 👍

Comment on lines 1250 to 1251
configApi.delete(`${profInfo.mergeArgsForProfile(profAttrs).knownArgs.find((arg) => arg.argName === "user")?.argLoc.jsonLoc}`);
configApi.delete(`${profInfo.mergeArgsForProfile(profAttrs).knownArgs.find((arg) => arg.argName === "password")?.argLoc.jsonLoc}`);
Copy link
Member

Choose a reason for hiding this comment

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

The string interpolation isn't necessary here:

Suggested change
configApi.delete(`${profInfo.mergeArgsForProfile(profAttrs).knownArgs.find((arg) => arg.argName === "user")?.argLoc.jsonLoc}`);
configApi.delete(`${profInfo.mergeArgsForProfile(profAttrs).knownArgs.find((arg) => arg.argName === "password")?.argLoc.jsonLoc}`);
configApi.delete(profInfo.mergeArgsForProfile(profAttrs).knownArgs.find((arg) => arg.argName === "user")?.argLoc.jsonLoc);
configApi.delete(profInfo.mergeArgsForProfile(profAttrs).knownArgs.find((arg) => arg.argName === "password")?.argLoc.jsonLoc);

Comment on lines 1261 to 1269
configApi.delete(`${profInfo.mergeArgsForProfile(profAttrs).knownArgs.find((arg) => arg.argName === "tokenType")?.argLoc.jsonLoc}`);
configApi.delete(`${profInfo.mergeArgsForProfile(profAttrs).knownArgs.find((arg) => arg.argName === "tokenValue")?.argLoc.jsonLoc}`);
configApi.delete(`${profInfo.mergeArgsForProfile(profAttrs).knownArgs.find((arg) => arg.argName === "tokenExpiration")?.argLoc.jsonLoc}`);
} else {
const profAttrs = await this.getProfileFromConfig(profileName);
configApi.set(`${profAttrs.profLoc.jsonLoc}.secure`, ["user", "password"]);
configApi.delete(`${profInfo.mergeArgsForProfile(profAttrs).knownArgs.find((arg) => arg.argName === "tokenType")?.argLoc.jsonLoc}`);
configApi.delete(`${profInfo.mergeArgsForProfile(profAttrs).knownArgs.find((arg) => arg.argName === "tokenValue")?.argLoc.jsonLoc}`);
configApi.delete(`${profInfo.mergeArgsForProfile(profAttrs).knownArgs.find((arg) => arg.argName === "tokenExpiration")?.argLoc.jsonLoc}`);
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above about string interpolation 😋

@@ -1361,7 +1511,7 @@ export class Profiles extends ProfilesCache {
.map((arg) => arg.argName);
}

private async loginWithRegularProfile(serviceProfile: zowe.imperative.IProfileLoaded, node?: IZoweNodeType): Promise<boolean> {
public async loginWithRegularProfile(serviceProfile: zowe.imperative.IProfileLoaded, node?: IZoweNodeType): Promise<boolean> {
Copy link
Member

Choose a reason for hiding this comment

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

This method seems like it could be kept private if the only added call is inside this file.

@zFernand0 zFernand0 dismissed their stale review July 15, 2024 15:00

seeing some odd behavior, but I believe the changes I requested earlier were addressed

Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

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

Could we wait for one more day before merging?
Just seeing a few things that may or may not be related.

Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
…/vscode-extension-for-zowe into feat/authSwitch

Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
Copy link
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @SanthoshiBoyina1!

@traeok traeok merged commit e6a0174 into zowe:main Jul 17, 2024
15 checks passed
Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

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

apologies for the delay.
I'll open the issue I mentioned in a few moments

@t1m0thyj
Copy link
Member

@SanthoshiBoyina1 @JillieBeanSim Should this feature be ported to the vNext branch?

@SanthoshiBoyina1
Copy link
Contributor Author

@SanthoshiBoyina1 @JillieBeanSim Should this feature be ported to the vNext branch?

I will move this feature to vNext branch

@t1m0thyj
Copy link
Member

I will move this feature to vNext branch

Done in #3062

@t1m0thyj t1m0thyj removed the needs-ported Apply to issues or PRs that need ported label Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

Cannot switch from basic auth. to token-based auth.
7 participants