OCM-4969 | feat: Add deprecation warning for login using offline token and disable oauth tokens for FedRAMP #1821

tirthct · 2024-03-04T19:36:16Z

Changed

Add deprecation warning for login using offline token
Disable oauth tokens for FedRAMP

Tested

Warning message should be displayed in case of offline tokens : tested

…n and disable oauth tokens for FedRAMP

openshift-ci · 2024-03-04T19:38:44Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tirthct
Once this PR has been reviewed and has the lgtm label, please assign gdbranco for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2024-03-04T19:38:45Z

Hi @tirthct. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tylercreller · 2024-03-04T21:11:20Z

cmd/login/cmd.go

+ cfg = new(config.Config)
+ }
+
+ if (cfg.FedRAMP || fedramp.HasFlag(cmd) || fedramp.IsGovRegion(arguments.GetRegion())) && (args.useAuthCode || args.useDeviceCode) {


We want to avoid using the stored configuration for this check. It will prevent someone from switching between govcloud and commercial environments if "fedramp": "true" is stored in their configuration.

@tirthct @tylercreller Can we combine this statement into a function and write a unit test for it e.g something like fedramp.IsLoginMethodSupported()

cheers,

Rob

@robpblake turns out we don't need the extra conditions except for fedramp.HasFlag() check. So just kept that instead of creating a whole new method.
Fixed the condition

tylercreller · 2024-03-04T21:14:19Z

cmd/login/cmd.go

@@ -325,6 +335,10 @@ func run(cmd *cobra.Command, argv []string) {
 clientID = args.clientID
 }
 }
+ } else {
+ if !args.useDeviceCode && !args.useDeviceCode {


We should specifically target displaying this message only if the offline token is provided. In this case we would display the warning message for client credentials, which we don't want.

Ack. Added offline token condition.

robpblake · 2024-03-05T10:15:13Z

cmd/login/cmd.go

+ r.Reporter.Errorf("Failed to load config file: %v", err)
+ os.Exit(1)
+ }
+ if cfg == nil {


@tirthct You don't need this check as the config.Load() method will always return an empty Config if one doesn't exist.

I don't think that check would be working properly. I removed it and it stopped me from logging in:

Not looking into that in this PR this change is already in the master branch for long, but def something to look into.

PS. ignore the double warnings. For some reason my machine sometimes does that.
@tylercreller has verified and confirmed the message is only shown once

robpblake · 2024-03-05T10:22:35Z

cmd/login/cmd.go

+ cfg = new(config.Config)
+ }
+
+ if (cfg.FedRAMP || fedramp.HasFlag(cmd) || fedramp.IsGovRegion(arguments.GetRegion())) && (args.useAuthCode || args.useDeviceCode) {


@tirthct @tylercreller Can we combine this statement into a function and write a unit test for it e.g something like fedramp.IsLoginMethodSupported()

cheers,

Rob

robpblake · 2024-03-05T10:25:53Z

/ok-to-test

…exclusion of offline and oauth login

tirthct · 2024-03-05T15:47:58Z

#1821 (comment)

We removed that combined condition, so a separate function is no longer needed

codecov · 2024-03-05T15:49:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 21.12%. Comparing base (40561a0) to head (0d5e3e7).
Report is 351 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1821      +/-   ##
==========================================
+ Coverage   20.81%   21.12%   +0.30%     
==========================================
  Files          87       89       +2     
  Lines       15108    15177      +69     
==========================================
+ Hits         3145     3206      +61     
- Misses      11722    11730       +8     
  Partials      241      241

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

tylercreller · 2024-03-11T17:37:10Z

Hold this PR until we get the green-light from PM

tirthct · 2024-03-11T17:46:23Z

Hold this PR until we get the green-light from PM

@tylercreller : Converted to draft

robpblake · 2024-04-08T09:30:02Z

@tirthct What's the status of this PR please?

cheers,
Rob

tirthct · 2024-04-10T12:02:40Z

@tylercreller should we merge this?

tylercreller · 2024-04-10T12:04:38Z

@tirthct @robpblake We are still waiting for the appropriate time to merge this. There are internal processes that need to happen before we can ship this.

openshift-ci · 2024-05-11T12:49:16Z

@tirthct: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

OCM-4969 | feat: Add deprecation warning for login using offline toke…

ece6311

…n and disable oauth tokens for FedRAMP

openshift-ci bot requested review from hunterkepley and marcolan018 March 4, 2024 19:37

openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 4, 2024

OCM-4969 | feat: Add error message for FedRAMP env

f8e636e

tylercreller reviewed Mar 4, 2024

View reviewed changes

robpblake reviewed Mar 5, 2024

View reviewed changes

openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 5, 2024

OCM-4969 | feat: Add conditions to check of offline token and mutual …

0d5e3e7

…exclusion of offline and oauth login

tirthct marked this pull request as draft March 11, 2024 17:45

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 11, 2024

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 13, 2024

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCM-4969 | feat: Add deprecation warning for login using offline token and disable oauth tokens for FedRAMP #1821

OCM-4969 | feat: Add deprecation warning for login using offline token and disable oauth tokens for FedRAMP #1821

tirthct commented Mar 4, 2024

openshift-ci bot commented Mar 4, 2024

openshift-ci bot commented Mar 4, 2024

tylercreller Mar 4, 2024

robpblake Mar 5, 2024

tirthct Mar 5, 2024

tylercreller Mar 4, 2024

tirthct Mar 5, 2024

robpblake Mar 5, 2024

tirthct Mar 5, 2024

tirthct Mar 5, 2024

robpblake Mar 5, 2024

robpblake commented Mar 5, 2024

tirthct commented Mar 5, 2024

codecov bot commented Mar 5, 2024 •

edited

tylercreller commented Mar 11, 2024

tirthct commented Mar 11, 2024

robpblake commented Apr 8, 2024

tirthct commented Apr 10, 2024

tylercreller commented Apr 10, 2024

openshift-ci bot commented May 11, 2024

OCM-4969 | feat: Add deprecation warning for login using offline token and disable oauth tokens for FedRAMP #1821

Are you sure you want to change the base?

OCM-4969 | feat: Add deprecation warning for login using offline token and disable oauth tokens for FedRAMP #1821

Conversation

tirthct commented Mar 4, 2024

openshift-ci bot commented Mar 4, 2024

openshift-ci bot commented Mar 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robpblake commented Mar 5, 2024

tirthct commented Mar 5, 2024

codecov bot commented Mar 5, 2024 • edited

Codecov Report

tylercreller commented Mar 11, 2024

tirthct commented Mar 11, 2024

robpblake commented Apr 8, 2024

tirthct commented Apr 10, 2024

tylercreller commented Apr 10, 2024

openshift-ci bot commented May 11, 2024

codecov bot commented Mar 5, 2024 •

edited