-
Notifications
You must be signed in to change notification settings - Fork 18
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
Implement oauth flow #11
base: master
Are you sure you want to change the base?
Conversation
Thinking about this, it's probably worth testing on a Concourse deployment with only 1 auth provider. |
10f689c
to
5f78dc7
Compare
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.
Thanks so much for this. It's been on my mental TODO list for a very long time!
@@ -23,7 +42,7 @@ login() { | |||
local team=$4 | |||
local insecure=$5 | |||
local tried=$6 | |||
local target=$7 | |||
local target=main |
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.
I'd like to understand why we're restricted to the main
team with this change.
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.
b/c that variable is never passed in, that was a quick hack to make it work
Also, that's not the team, it's just the name of the target which can be absolutely anything; no effect to the user
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.
@troykinsella Can you review this when you get time, this change is required for concourse 5.2 or above.
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.
any chance on merging?
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.
@ChipWolf I think the correct course of action here is to actually pass through the target, rather than putting a band-aid on the local target variable. This has been fixed on master, so please revert this change.
Concourse 5.x is now supported on master using basic auth, but I'd still like to incorporate this PR to support oauth. Thanks for the contribution! If you can refactor it to activate upon specifying oauth parameters on the |
As fly can be downloaded anonymously, the oauth login implementation needs to move to the |
On it |
Fixes #10