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

Update to the new auth workflow #328

Merged
merged 6 commits into from Mar 29, 2018
Merged

Update to the new auth workflow #328

merged 6 commits into from Mar 29, 2018

Conversation

@Soulou
Copy link
Member

@Soulou Soulou commented Jan 26, 2018

No description provided.

johnsudaar added 2 commits Dec 14, 2017
@Soulou Soulou force-pushed the fix/320/new_auth branch from 55aa468 to 393ea21 Jan 26, 2018
Soulou added 3 commits Mar 28, 2018
@EtienneM EtienneM self-requested a review Mar 29, 2018
return -1, errgo.Notef(err, "fail to generate auth")
}

req.SetBasicAuth("", token)

This comment has been minimized.

@johnsudaar

johnsudaar Mar 29, 2018
Member

Does this work?
The JWT are use with Bearer auth not Basic auth.

This comment has been minimized.

@Soulou

Soulou Mar 29, 2018
Author Member

└> ./scalingo -a db run bash
-----> Connecting to container [one-off-1598]...  
-----> Process 'bash' is starting...  

   _____               __ _                    
  / ___/ _____ ____ _ / /(_)____   ____ _ ____ 
  \__ \ / ___// __ `// // // __ \ / __ `// __ \
 ___/ // /__ / /_/ // // // / / // /_/ // /_/ /
/____/ \___/ \__,_//_//_//_/ /_/ \__, / \____/ 
                                /____/         

This comment has been minimized.

@Soulou

Soulou Mar 29, 2018
Author Member

Authentication service too permissive?

This comment has been minimized.

@Soulou

Soulou Mar 29, 2018
Author Member

Oh I know actually

This comment has been minimized.

@Soulou

Soulou Mar 29, 2018
Author Member

The token is sent to the run service (one-off.scalingo.com) which still expect basic auth

if err != nil {
return nil, nil, errgo.Notef(err, "fail to generate auth")
}
req.SetBasicAuth("", token)

This comment has been minimized.

@johnsudaar

johnsudaar Mar 29, 2018
Member

Same here

This comment has been minimized.

@Soulou

Soulou Mar 29, 2018
Author Member

Explanation above

appio.Errorf("Fail to login (%d/3): %v\n\n", i+1, err)
}
}
}
if err == ErrAuthenticationFailed {
return nil, errors.New("Forgot your password? https://my.scalingo.com")
return nil, nil, errors.New("Forgot your password? https://my.scalingo.com")

This comment has been minimized.

@johnsudaar

johnsudaar Mar 29, 2018
Member

auth.scalingo.com

This comment has been minimized.

@Soulou

Soulou Mar 29, 2018
Author Member

Done

return -1, errgo.Notef(err, "fail to generate auth")
}

req.SetBasicAuth("", token)

This comment has been minimized.

@EtienneM

EtienneM Mar 29, 2018
Member

Shouldn't we use the bearer token now?

This comment has been minimized.

@Soulou

Soulou Mar 29, 2018
Author Member

Explanation above

AuthenticatedUser, err = Authenticator.LoadAuth()
if err == nil {
C.apiToken = AuthenticatedUser.AuthenticationToken
user, tokenGenerator, err := Authenticator.LoadAuth()

This comment has been minimized.

@EtienneM

EtienneM Mar 29, 2018
Member

What happens if err != nil?

This comment has been minimized.

@Soulou

Soulou Mar 29, 2018
Author Member

user and tokenGenerator will be nil, so no authentication will be loaded and the CLI will run in "unauthenticated" mode. So if a request requires auth, the CLI will ask the user to login

@EtienneM EtienneM merged commit 7e2a971 into master Mar 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants