-
Notifications
You must be signed in to change notification settings - Fork 194
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
OCM-4969 | feat: Add deprecation warning for login using offline token and disable oauth tokens for FedRAMP #1821
base: master
Are you sure you want to change the base?
Conversation
…n and disable oauth tokens for FedRAMP
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tirthct The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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 Once the patch is verified, the new status will be reflected by the 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. |
cmd/login/cmd.go
Outdated
cfg = new(config.Config) | ||
} | ||
|
||
if (cfg.FedRAMP || fedramp.HasFlag(cmd) || fedramp.IsGovRegion(arguments.GetRegion())) && (args.useAuthCode || args.useDeviceCode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
cmd/login/cmd.go
Outdated
@@ -325,6 +335,10 @@ func run(cmd *cobra.Command, argv []string) { | |||
clientID = args.clientID | |||
} | |||
} | |||
} else { | |||
if !args.useDeviceCode && !args.useDeviceCode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack. Added offline token condition.
cmd/login/cmd.go
Outdated
r.Reporter.Errorf("Failed to load config file: %v", err) | ||
os.Exit(1) | ||
} | ||
if cfg == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tirthct You don't need this check as the config.Load()
method will always return an empty Config
if one doesn't exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS. ignore the double warnings. For some reason my machine sometimes does that.
@tylercreller has verified and confirmed the message is only shown once
cmd/login/cmd.go
Outdated
cfg = new(config.Config) | ||
} | ||
|
||
if (cfg.FedRAMP || fedramp.HasFlag(cmd) || fedramp.IsGovRegion(arguments.GetRegion())) && (args.useAuthCode || args.useDeviceCode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
/ok-to-test |
…exclusion of offline and oauth login
We removed that combined condition, so a separate function is no longer needed |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
Hold this PR until we get the green-light from PM |
@tylercreller : Converted to draft |
@tirthct What's the status of this PR please? cheers, |
@tylercreller should we merge this? |
@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. |
@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. |
@tirthct / @tylercreller What's our status on this one please? |
@robpblake Still pending green-light from PM - expecting more information following the disablement of internal offline tokens. |
Changed
Tested