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

To support token based authentication #53

Merged
merged 19 commits into from
Aug 4, 2023

Conversation

sgadisetti
Copy link
Contributor

The change is to support token based authentication using OAuth2

client.go Outdated
@@ -180,7 +184,11 @@ func (c *Client) do(method, path string, query map[string]string, body io.Reader
req.Header.Set("Content-Type", contentType)
}

req.SetBasicAuth(c.apiUser, c.apiPassword)
if c.token != nil {
c.token.SetAuthHeader(req)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that it is not worth adding a new dependency just to put down one HTTP header. This can be done like this:

if c.AuthorizationHeader != "" {
    req.Header.Set("Authorization", c.AuthorizationHeader)
} else {
    req.SetBasicAuth(c.apiUser, c.apiPassword)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @liderman Thanks for quick review!

As Oauth2 provides different popular flows like Authorization Code Flow, Client Credentials Flow.. and also supports refresh token, I thought it would be good to use it and client applications can pass the token that it gets from authorization server as is without having any conversion in the client code.

Token object as follows..

type Token struct {
// AccessToken is the token that authorizes and authenticates
// the requests.
AccessToken string json:"access_token"

// TokenType is the type of token.
// The Type method returns either this or "Bearer", the default.
TokenType string `json:"token_type,omitempty"`

// RefreshToken is a token that's used by the application
// (as opposed to the user) to refresh the access token
// if it expires.
RefreshToken string `json:"refresh_token,omitempty"`

// Expiry is the optional expiration time of the access token.
//
// If zero, TokenSource implementations will reuse the same
// token forever and RefreshToken or equivalent
// mechanisms for that TokenSource will not be used.
Expiry time.Time `json:"expiry,omitempty"`

// raw optionally contains extra metadata from the server
// when updating a token.
raw interface{}

}

Please let me know your thoughts. I am flexible either way!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sgadisetti
You are using the "SetAuthHeader" method, which only sets the "Authorization" HTTP header:
https://cs.opensource.google/go/x/oauth2/+/refs/tags/v0.9.0:token.go;l=84

// SetAuthHeader sets the Authorization header to r using the access
// token in t.
//
// This method is unnecessary when using Transport or an HTTP Client
// returned by this package.
func (t *Token) SetAuthHeader(r *http.Request) {
	r.Header.Set("Authorization", t.Type()+" "+t.AccessToken)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liderman Please check the latest changes and see if they make sense!

Copy link
Contributor

@liderman liderman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are files README.md and go.sum deleted?

README.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this file removed? Looks like a mistake

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry. Something got messed up in my local during previous pull request. Files are restored now. Please check!

go.sum Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this file removed? Looks like a mistake

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry. Something got messed up in my local during previous pull request. Files are restored now. Please check!

@liderman liderman merged commit e389c70 into citilinkru:master Aug 4, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants