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

Auth order spike #2372

Merged
merged 19 commits into from
Dec 18, 2024
Merged

Auth order spike #2372

merged 19 commits into from
Dec 18, 2024

Conversation

gejohnston
Copy link
Member

@gejohnston gejohnston commented Nov 19, 2024

What It Does

This PR contains a design document proposing how Zowe clients could enable users to specify their own preferred order of authentication.

This PR also contains a proof-of-concept prototype of a new AuthOrder class. When local code is modified to call AuthOrder it successfully performs the basic required functionality.

  • Since our real Zowe code has not been modified to call AuthOrder, the AuthOrder.ts file should be safe to merge into master.
  • It can provide a good starting point for final implementation.

Certainly, the proposed design document should be the primary focus of reviews. Several secondary items about the document to consider are:

  • I purposely chose to place this design document in the Zowe GitHub repository.

    • It is reasonable to allow interested, external people see the design of our open source app before it is implemented.
    • Even after implementation, we can point people to such a document as an easy way to explain how the app is intended to operate.
    • We can consider whether or not we want our future designs to follow this publicly-available approach.
  • It will be easier to read the document from the repo than from the file diffs.
    https://github.com/zowe/zowe-cli/blob/auth-order-spike/docs/Design_for_selecting_auth_type.md

  • Alternatively, if you download the file and view it in the VSCode "Markdown Preview" window, you get a little splash of color which make some things easier to identify.

The following implementation stories were created as a result of this research spike:

US1003134 CLI: Use the AuthOrder class in Zowe CLI/API

US1003140 CLI: Display authentication type from commands

US1003152 CLI: Include authOrder in zowe.schema.json

US1003153 CLI: Use authOrder for SSH connections

US1003155 ZE: Use the AuthOrder class in Zowe Explorer

Review Checklist
I certify that I have:

  • Completed analysis and/or prototype of proposed software
  • Decided that no changelog update is required
  • Created stories for future implementation

Copy link

codecov bot commented Nov 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.28%. Comparing base (2bf93ca) to head (bef7195).
Report is 21 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2372   +/-   ##
=======================================
  Coverage   91.28%   91.28%           
=======================================
  Files         638      638           
  Lines       18207    18207           
  Branches     3822     3822           
=======================================
  Hits        16620    16620           
  Misses       1586     1586           
  Partials        1        1           

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

Copy link
Member

@traeok traeok left a comment

Choose a reason for hiding this comment

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

I know that the tests are failing, but I approve of the design document, hence my approval. Very comprehensive and covered any questions that I had throughout my review. Thanks Gene for writing this proposal.

Given the context of where the changes would need to be made and following up on our previous in-person discussion, we may be able to leverage the Config class for these new functions. ProfileInfo has access to the loaded, merged config instance, so we can leverage this in Zowe Explorer to get access to the authentication order for a given profile. It seems that ImperativeConfig.instance.config might also be valid at the time a CLI command is run, so it could be used to access the authentication order as part of building the session.

@gejohnston gejohnston added no-changelog no-release Indicates that there is no new functionality being delivered labels Dec 4, 2024
@gejohnston gejohnston marked this pull request as ready for review December 4, 2024 23:52
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.

Hey Gene,
Thanks for holding a meeting to discuss these concepts 🙏

After moving things into a /prototypes/imperative/src/rest/src/session/AuthOrder.ts I think I'm fine to approve this PR 🙏

Update: After some discussion, we agreed to use `/prototypes/packages/imperative/src/rest/src/session/AuthOrder.ts. This allows for other kinds of prototype-related code outside of the packages directory 🙏

@JTonda JTonda requested a review from zFernand0 December 16, 2024 16:05
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.

LGTM! 😋

left some minor suggestions/questions, but nothing that should prevent this PR from being merged.

prototypes/Readme.md Outdated Show resolved Hide resolved
@gejohnston gejohnston merged commit aa2d6e6 into master Dec 18, 2024
20 checks passed
@gejohnston gejohnston deleted the auth-order-spike branch December 18, 2024 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog no-release Indicates that there is no new functionality being delivered
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

3 participants