-
Notifications
You must be signed in to change notification settings - Fork 928
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
if cf misconfigured, still allow user to 'cf login --sso' #1624
Conversation
✅ Hey drnic! The commit authors and yourself have already signed the CLA. |
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/165626330 The labels on this github issue will be updated when the story is started. |
Hi @drnic could you please provide more information about this pull request: Description of the ChangeWe must be able to understand the design of your change from this description. Why Is This PR Valuable? What benefits will be realized by the code change? What users would want this change? What user need is this change addressing? -->Explain why this functionality should be in the cf CLI, as opposed to a plugin. --> Applicable IssuesHow Urgent Is The Change? Other Relevant Parties |
Ah, sorry for deleting all the text. The start of it said "you must have signed the CLA" and I assumed I could ignore it. |
@abbyachau FYI - your prompts for me above are (very?) different from https://github.com/cloudfoundry/cli/blob/master/.github/PULL_REQUEST_TEMPLATE.md |
@abbyachau I've updated the description with your questions and my answers. Thanks + sorry. |
Ah thanks @drnic I've updated the pull request template unsure why so much of it was obfuscated. |
Thanks for updating @drnic; we are currently rewriting the I'm struggling to understand how I would accept this story. Would you mind providing acceptance criteria so that I understand what the previous workflow looks like and what your proposed changes are. Something similar to: Current workflow |
Acceptance steps:
Err, sorry I didn't write them like cucumber steps. |
Thank you. No problem, it was just an example - appreciate the acceptance steps. |
Hi @drnic thank you for this pull request. We were planning to review/move forward with efforts detailed in #1617 and as such will not be pulling in this feature. As you know, we hope to finish rewriting |
I would appreciate this PR being pulled into current cf cli please. I don’t feel like the explanation above included an explanation why this isn’t a valuable bug fix for current users. :( |
After some digging, this request is not to enhance I'm tagging UAA PMs (@cwang-pivotal, @dbeneke, @aramprice) and I'll try to summarize what's going on here to get your feedback. Currently, even if users do not have an identity provider set up, they can use
And when they go to that url and enter their credentials, they can retrieve the passcode to log into their cf instance. It appears that this is unintended behavior, according to the UAA: However this is a workflow the CF CLI supports, and we cannot remove support for in the V6 CLI. Furthermore, we believe this is a workflow that some users might use for password managers like LastPass - although we have not validated this assumption. The question here is whether this is a workflow that the UAA team supports as this pull request makes that workflow more explicit, and more of a 'real' feature. |
I believe this issue has appeared on someone’s SO issue https://stackoverflow.com/questions/60210238/cloudfoundry-cli-login-not-working-credentials-were-rejected-please-try-again/60217563#60217563 Could we revisit this? |
If a target CF has not been configured correctly, this PR still allows a user to
cf login --sso
. This is a "good thing". We should encourage authentication via browsers and password vaults.Why Is This PR Valuable?
We should encourage all users to move away from providing passwords via CLIs in terminals. An oauth sequence into the browser allows the user to use their 1password integration.
Currently some users cannot use
cf login --sso
because their platform operator misconfigured their UAA prompts. This PR fixes this issue for those users.What benefits will be realized by the code change? What users would want this change? What user need is this change addressing?
I am doing a video on
cf login
tips & tricks. I'll be promotingcf login --sso
as my preferred method for ppl to login. Currently it's broken for some users.Explain why this functionality should be in the cf CLI, as opposed to a plugin.
Its a fix to
cf login
.
Applicable Issues
Initial PR does not have i18n. I'll do this in a follow up PR.
How Urgent Is The Change?
I'd like to have this feature fixed for my video.
Other Relevant Parties
UAA