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: Detect JWT expiration in token values and prompt user if expired #3174

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

traeok
Copy link
Member

@traeok traeok commented Oct 3, 2024

Proposed changes

Implements JWT expiration checks for profiles using a token for authentication.

  • Prompts the user if the JWT could be successfully decoded from the token and the expire time/date has elapsed
  • Skips LTPA token values (cannot be decoded w/o private key, only given to servers generating the tokens)

How to test

  1. Log into a profile using API ML or z/OSMF using JWT token authentication.
  2. Wait for the token to expire ⏳
  3. Once expired, attempt to search again - notice the login prompt will appear instead of the search prompt, as the token has expired

Release Notes

Milestone: 3.1.0

Changelog:

  • Added expired JWT token detection for profiles in each tree view (Data Sets, USS, Jobs). When a user performs a search on a profile, they are prompted to log in if their token expired. #3175

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (non-breaking change which adds or improves functionality)
  • Breaking change (a change that would cause existing functionality to not work as expected)
  • Documentation (Markdown, README updates)
  • Other (please specify above in "Proposed changes" section)

Checklist

General

  • I have read the CONTRIBUTOR GUIDANCE wiki
  • All PR dependencies have been merged and published (if applicable)
  • A GIF or screenshot is included in the PR for visual changes
  • The pre-publish command has been executed:
    • v2 and below: yarn workspace vscode-extension-for-zowe vscode:prepublish
    • v3: pnpm --filter vscode-extension-for-zowe vscode:prepublish

Code coverage

  • There is coverage for the code that I have added
  • I have added new test cases and they are passing
  • I have manually tested the changes

Deployment

  • I have added developer documentation (if applicable)
  • Documentation should be added to Zowe Docs
    • If you're an outside contributor, please post in the #zowe-doc Slack channel to coordinate documentation.
    • Otherwise, please check with the rest of the squad about any needed documentation before merging.
  • These changes may need ported to the appropriate branches (list here):

Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
@traeok traeok linked an issue Oct 3, 2024 that may be closed by this pull request
Copy link

codecov bot commented Oct 3, 2024

Codecov Report

Attention: Patch coverage is 92.59259% with 2 lines in your changes missing coverage. Please review.

Project coverage is 92.80%. Comparing base (60dec5a) to head (96f3312).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...ckages/zowe-explorer/src/trees/ZoweTreeProvider.ts 90.47% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3174      +/-   ##
==========================================
- Coverage   92.81%   92.80%   -0.01%     
==========================================
  Files         113      113              
  Lines       11672    11694      +22     
  Branches     2463     2598     +135     
==========================================
+ Hits        10833    10853      +20     
- Misses        837      839       +2     
  Partials        2        2              

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

Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
@traeok traeok marked this pull request as ready for review October 3, 2024 17:53
Copy link

github-actions bot commented Oct 3, 2024

📅 Suggested merge-by date: 10/17/2024

Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
@traeok
Copy link
Member Author

traeok commented Oct 3, 2024

Failing stage ze-build (20.x, ubuntu-latest) is due to an unrelated, transient failure in the integration tests.

@adam-wolfe
Copy link
Contributor

Tried it out. Looks great.

@t1m0thyj t1m0thyj added this to the v3.1.0 milestone Oct 3, 2024
Copy link

sonarcloud bot commented Oct 7, 2024

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.

the functionality LGTM! 😋

requesting changes to prevent this PR from being merged before we publish 3.0.1 😅

Comment on lines +331 to +343
const fullToken = tokenValueProp.argValue.toString();
// JWT format: [header].[payload].[signature]
const tokenParts = fullToken.split(".");
try {
const payloadJson = JSON.parse(Buffer.from(tokenParts[1], "base64url").toString("utf8"));
if ("exp" in payloadJson) {
const expireDate = dayjs.unix(payloadJson.exp);
if (expireDate.isBefore(dayjs())) {
await AuthUtils.promptUserForSsoLogin(profileName);
}
}
} catch (err) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

Curious if this should be moved to the SDKs 😅
Or at least be made available to extenders 😅

Copy link
Member Author

@traeok traeok Oct 8, 2024

Choose a reason for hiding this comment

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

I like the idea of moving it to the SDKs so anyone can leverage this - thanks for the suggestion! I moved this function implementation into Imperative's ProfileInfo class.
Here is the PR: zowe/zowe-cli#2298

Once that's approved & merged, we can adopt it in this PR - this would mean that extenders also get access to it directly from the facility that they already use for profiles ^^

Copy link
Contributor

@anaxceron anaxceron 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, @traeok. Had one question on the changelog.

@@ -6,6 +6,8 @@ All notable changes to the "vscode-extension-for-zowe" extension will be documen

### New features and enhancements

- Added expired JWT token detection for profiles in each tree view (Data Sets, USS, Jobs). When a user performs a search on a profile, they are prompted to log in if their token expired. [#3175](https://github.com/zowe/zowe-explorer-vscode/issues/3175)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we spell out JWT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Review/QA
Development

Successfully merging this pull request may close these issues.

Add detection for JWT token expiration
5 participants