Oauth improvements #1867
Open
Oauth improvements #1867
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…les" This reverts commit 64d3fb7.
…ckage Signed-off-by: 7h3-3mp7y-m4n <emailtorash@gmail.com>
Signed-off-by: 7h3-3mp7y-m4n <emailtorash@gmail.com>
* separately test gofr module * exclude unnecessary grpc files from examples test coverage * fix tests and remove failing ones * skip two more flaky tests * fix linters * fix linters * resolve issues in cron tests * remove unwanted changes * resolve review comments
…3 in /pkg/gofr/datasource/arangodb (gofr-dev#1765) * build(deps): bump github.com/arangodb/go-driver/v2 Bumps [github.com/arangodb/go-driver/v2](https://github.com/arangodb/go-driver) from 2.1.2 to 2.1.3. - [Release notes](https://github.com/arangodb/go-driver/releases) - [Changelog](https://github.com/arangodb/go-driver/blob/master/CHANGELOG.md) - [Commits](arangodb/go-driver@v2.1.2...v2.1.3) --- updated-dependencies: - dependency-name: github.com/arangodb/go-driver/v2 dependency-version: 2.1.3 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> * update mock arango interface --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Umang Mundhra <mundhraumang.02@gmail.com> Co-authored-by: Aryan Mehrotra <aryan.mehrotra@gofr.dev>
Bumps the actions group with 1 update: [ls-lint/action](https://github.com/ls-lint/action). Updates `ls-lint/action` from 2.3.0 to 2.3.1 - [Commits](ls-lint/action@v2.3.0...v2.3.1) --- updated-dependencies: - dependency-name: ls-lint/action dependency-version: 2.3.1 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: actions ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [github.com/XSAM/otelsql](https://github.com/XSAM/otelsql) from 0.38.0 to 0.39.0. - [Release notes](https://github.com/XSAM/otelsql/releases) - [Changelog](https://github.com/XSAM/otelsql/blob/main/CHANGELOG.md) - [Commits](XSAM/otelsql@v0.38.0...v0.39.0) --- updated-dependencies: - dependency-name: github.com/XSAM/otelsql dependency-version: 0.39.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [github.com/redis/go-redis/extra/redisotel/v9](https://github.com/redis/go-redis) from 9.9.0 to 9.10.0. - [Release notes](https://github.com/redis/go-redis/releases) - [Changelog](https://github.com/redis/go-redis/blob/master/CHANGELOG.md) - [Commits](redis/go-redis@v9.9.0...v9.10.0) --- updated-dependencies: - dependency-name: github.com/redis/go-redis/extra/redisotel/v9 dependency-version: 9.10.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [golang.org/x/sync](https://github.com/golang/sync) from 0.14.0 to 0.15.0. - [Commits](golang/sync@v0.14.0...v0.15.0) --- updated-dependencies: - dependency-name: golang.org/x/sync dependency-version: 0.15.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [google.golang.org/api](https://github.com/googleapis/google-api-go-client) from 0.235.0 to 0.236.0. - [Release notes](https://github.com/googleapis/google-api-go-client/releases) - [Changelog](https://github.com/googleapis/google-api-go-client/blob/main/CHANGES.md) - [Commits](googleapis/google-api-go-client@v0.235.0...v0.236.0) --- updated-dependencies: - dependency-name: google.golang.org/api dependency-version: 0.236.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.72.2 to 1.73.0. - [Release notes](https://github.com/grpc/grpc-go/releases) - [Commits](grpc/grpc-go@v1.72.2...v1.73.0) --- updated-dependencies: - dependency-name: google.golang.org/grpc dependency-version: 1.73.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [github.com/alicebob/miniredis/v2](https://github.com/alicebob/miniredis) from 2.34.0 to 2.35.0. - [Release notes](https://github.com/alicebob/miniredis/releases) - [Changelog](https://github.com/alicebob/miniredis/blob/master/CHANGELOG.md) - [Commits](alicebob/miniredis@v2.34.0...v2.35.0) --- updated-dependencies: - dependency-name: github.com/alicebob/miniredis/v2 dependency-version: 2.35.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.72.2 to 1.73.0. - [Release notes](https://github.com/grpc/grpc-go/releases) - [Commits](grpc/grpc-go@v1.72.2...v1.73.0) --- updated-dependencies: - dependency-name: google.golang.org/grpc dependency-version: 1.73.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [golang.org/x/text](https://github.com/golang/text) from 0.25.0 to 0.26.0. - [Release notes](https://github.com/golang/text/releases) - [Commits](golang/text@v0.25.0...v0.26.0) --- updated-dependencies: - dependency-name: golang.org/x/text dependency-version: 0.26.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [modernc.org/sqlite](https://gitlab.com/cznic/sqlite) from 1.37.1 to 1.38.0. - [Commits](https://gitlab.com/cznic/sqlite/compare/v1.37.1...v1.38.0) --- updated-dependencies: - dependency-name: modernc.org/sqlite dependency-version: 1.38.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
This reverts commit 24f8859.
docs/quick-start/connecting-mysql/page.md : fixed docker container naming for mysql
vikash
reviewed
Jun 17, 2025
Umang01-hash
previously approved these changes
Jun 19, 2025
ccoVeille
reviewed
Jun 19, 2025
pkg/gofr/service/errors.go
Outdated
| @@ -0,0 +1,20 @@ | |||
| package service | |||
|
|
|||
| import "github.com/pkg/errors" | |||
There was a problem hiding this comment.
This sounds like a bad idea to use something else than the std error
There was a problem hiding this comment.
need this for errors.Wrap, similar to datasource.ErrorDB
There was a problem hiding this comment.
See this
There was a problem hiding this comment.
are we moving away from the convention used in datasource.ErrorDB? If yes, will make the change here as well as datasource.ErrorDB.Error()
There was a problem hiding this comment.
There is one slight difference
Yes, datasource/errors.go is doing something strange.
But this file also uses a method errors.WithStack that is only available in github.com/pkg/errors # package.
So you could and should change the Wrap().Error oddity, but you won't be able to remove pkg/errors package unfortunately
There was a problem hiding this comment.
Updated the implementation here. Left the datasource.ErrorDB as is for now - can take that up separately.
pkg/gofr/service/errors.go
Outdated
Comment on lines
11
to
17
| if o.Message == "" && o.Err == nil { | ||
| return "unknown error" | ||
| } else if o.Message == "" { | ||
| return o.Err.Error() | ||
| } else if o.Err == nil { | ||
| return o.Message | ||
| } |
There was a problem hiding this comment.
This should be preferred
Suggested change
| if o.Message == "" && o.Err == nil { | |
| return "unknown error" | |
| } else if o.Message == "" { | |
| return o.Err.Error() | |
| } else if o.Err == nil { | |
| return o.Message | |
| } | |
| if o.Message == "" && o.Err == nil { | |
| return "unknown error" | |
| } | |
| if o.Message == "" { | |
| return o.Err.Error() | |
| } | |
| if o.Err == nil { | |
| return o.Message | |
| } |
There was a problem hiding this comment.
But consider this instead
pkg/gofr/service/errors.go
Outdated
Comment on lines
11
to
17
| if o.Message == "" && o.Err == nil { | ||
| return "unknown error" | ||
| } else if o.Message == "" { | ||
| return o.Err.Error() | ||
| } else if o.Err == nil { | ||
| return o.Message | ||
| } |
There was a problem hiding this comment.
The logic is strange, try this
Suggested change
| if o.Message == "" && o.Err == nil { | |
| return "unknown error" | |
| } else if o.Message == "" { | |
| return o.Err.Error() | |
| } else if o.Err == nil { | |
| return o.Message | |
| } | |
| if o.Message != "" { | |
| return o.Message | |
| } | |
| if o.Err != nil { | |
| return o.Err.Error() | |
| } | |
| return "unknown error" |
There was a problem hiding this comment.
How do we handle the scenario where both Message and Err are non empty?
There was a problem hiding this comment.
I don't know if it can happen, but it can be done with
return fmt.Sprintf("%s: %s", o.Message, o.Err)
ccoVeille
reviewed
Jun 19, 2025
pkg/gofr/service/errors.go
Outdated
| @@ -0,0 +1,20 @@ | |||
| package service | |||
|
|
|||
| import "github.com/pkg/errors" | |||
There was a problem hiding this comment.
See this
ccoVeille
suggested changes
Jun 20, 2025
|
|
||
| func TestHttpService_RequestsOAuth(t *testing.T) { | ||
| server := setupOAuthHTTPServer(t) | ||
| defer server.httpServer.Close() |
There was a problem hiding this comment.
Please remove them all of them, and consider using a cleanup in setupOAuthHTTPServer
See #1867 (comment)
Comment on lines
+115
to
+118
| server.httpServer = httptest.NewServer(mux) | ||
|
|
||
| return &server | ||
| } |
There was a problem hiding this comment.
Suggested change
| server.httpServer = httptest.NewServer(mux) | |
| return &server | |
| } | |
| server.httpServer = httptest.NewServer(mux) | |
| t.Cleanup(func() { | |
| err := server.httpServer.Close() | |
| assert.NoError(t, err, "error while closing http server") | |
| } | |
| return &server | |
| } |
| headers bool | ||
| tokenURL string | ||
| err error | ||
| statusCode int |
There was a problem hiding this comment.
Don't add statusCode to the struct, you are using the same value for all of them.
Suggested change
| statusCode int |
See #1867 (comment)
| return httpSvc | ||
| } | ||
| if resp != nil { | ||
| assert.Equalf(t, tc.statusCode, resp.StatusCode, "failed test case #%d", i) |
10000
There was a problem hiding this comment.
Suggested change
| assert.Equalf(t, tc.statusCode, resp.StatusCode, "failed test case #%d", i) | |
| assert.Equalf(t, http.StatusOK, resp.StatusCode, "failed test case #%d", i) |
Comment on lines
+15
to
+17
| var err1 = errors.New(`unsupported protocol scheme ""`) | ||
|
|
||
| var err2 = errors.New(`unsupported protocol scheme "abc"`) |
There was a problem hiding this comment.
These names are not useful, also it leads to confusion when reading the map used for tests
Suggested change
| var err1 = errors.New(`unsupported protocol scheme ""`) | |
| var err2 = errors.New(`unsupported protocol scheme "abc"`) | |
| const invalidURL := "abc://invalid-url" | |
| var ( | |
| errTokenURL = &url.Error{Op: "Post", URL: "", Err: errors.New(`unsupported protocol scheme ""`)} | |
| errInvalidURL = &url.Error{Op: "Post", URL: invalidURL, Err: err2} | |
| ) |
Then use errTokenURL and errInvalidURL everywhere
Comment on lines
+34
to
+38
| {http.MethodGet, false, tokenURL, nil, http.StatusOK}, | ||
| {http.MethodPost, false, tokenURL, nil, http.StatusOK}, | ||
| {http.MethodPut, false, tokenURL, nil, http.StatusOK}, | ||
| {http.MethodDelete, false, tokenURL, nil, http.StatusOK}, | ||
| {http.MethodPatch, false, tokenURL, nil, http.StatusOK}, |
There was a problem hiding this comment.
a struct should be used with the fields name, not like this
Comment on lines
+102
to
110
| func callHTTPServiceGet(ctx context.Context, service HTTP, headers bool) (resp *http.Response, err error) { | ||
| if headers { | ||
| resp, err = service.GetWithHeaders(ctx, "test", nil, nil) | ||
| } else { | ||
| resp, err = service.Get(ctx, "test", nil) | ||
| } | ||
|
|
||
| _ = resp.Body.Close() | ||
| return resp, err | ||
| } |
There was a problem hiding this comment.
Please consider doing something like this
func callHTTPService(t *testing.T, ctx context.Context, service HTTP, method string, headers bool) (resp *http.Response, err error) {
t.Helper()
if headers {
resp, err = callHTTPServiceWithHeaders(t, ctx, service, method)
} else {
resp, err = callHTTPServiceWithoutHeaders(t, ctx, service, method)
}
if resp != nil {
t.Cleanup(func() {
require.NoError(t, resp.Body.Close())
})
}
return resp, err
}
func callHTTPServiceWithHeaders(t *testing.T, ctx context.Context, service HTTP, method string, headers bool) (resp *http.Response, err error) {
switch method {
case http.MethodGet:
return resp, err = service.GetWithHeaders(ctx, "test", nil, nil)
case http.Method:Post:
return resp, err = service.PostWithHeaders(ctx, "test", nil, nil, nil)
case http.MethodDelete:
return resp, err = service.DeleteWithHeaders(ctx, "test", nil, nil, nil)
case http.MethodGet:
return resp, err = service.GetWithHeaders(ctx, "test", nil, nil, nil)
}
t.Fatalf("Unsupported method: %s", method)
}
func callHTTPServiceWithoutHeaders(t *testing.T, ctx context.Context, service HTTP, method string, headers bool) (resp *http.Response, err error) {
switch method {
case http.MethodGet:
return resp, err = service.Get(ctx, "test", nil)
case http.Method:Post:
return resp, err = service.Post(ctx, "test", nil, nil)
case http.MethodDelete:
return resp, err = service.Delete(ctx, "test", nil, nil)
case http.MethodPatch:
return resp, err = service.Patch(ctx, "test", nil, nil)
}
t.Fatalf("Unsupported method: %s", method)
}This way, you don't have to play with a switch everywhere
Comment on lines
+256
to
271
| for i, tc := range testCases { | ||
| service, ok := getOAuthService(server.httpService(), server.clientID, server.clientSecret, tc.tokenURL, server.audienceClaim).(*oAuth) | ||
| assert.True(t, ok, "unable to get oAuth object for test case #%d", i) | ||
|
|
||
| headers, err := service.addAuthorizationHeader(tc.ctx, tc.headers) | ||
| assert.Equal(t, tc.err, err, "failed test case #%d", i) | ||
|
|
||
| if err == nil { | ||
| authHeader, ok := headers[AuthHeader] | ||
| assert.True(t, ok, "failed test case #%d", i) | ||
| assert.NotEmptyf(t, authHeader, "failed test case #%d", i) | ||
| assert.True(t, strings.HasPrefix(authHeader, "Bearer")) | ||
| delete(headers, AuthHeader) | ||
| assert.Equal(t, tc.response, headers, "failed test case #%d", i) | ||
| } | ||
| } |
There was a problem hiding this comment.
the way to do it to use a t.Run here
Suggested change
| for i, tc := range testCases { | |
| service, ok := getOAuthService(server.httpService(), server.clientID, server.clientSecret, tc.tokenURL, server.audienceClaim).(*oAuth) | |
| assert.True(t, ok, "unable to get oAuth object for test case #%d", i) | |
| headers, err := service.addAuthorizationHeader(tc.ctx, tc.headers) | |
| assert.Equal(t, tc.err, err, "failed test case #%d", i) | |
| if err == nil { | |
| authHeader, ok := headers[AuthHeader] | |
| assert.True(t, ok, "failed test case #%d", i) | |
| assert.NotEmptyf(t, authHeader, "failed test case #%d", i) | |
| assert.True(t, strings.HasPrefix(authHeader, "Bearer")) | |
| delete(headers, AuthHeader) | |
| assert.Equal(t, tc.response, headers, "failed test case #%d", i) | |
| } | |
| } | |
| for i, tc := range testCases { | |
| t.Run(fmt.Sprintf("test case #%d", d), func(t *testing) { | |
| service, ok := getOAuthService(server.httpService(), server.clientID, server.clientSecret, tc.tokenURL, server.audienceClaim).(*oAuth) | |
| assert.True(t, ok, "unable to get oAuth object") | |
| headers, err := service.addAuthorizationHeader(tc.ctx, tc.headers) | |
| assert.Equal(t, tc.err, err) | |
| if err == nil { | |
| authHeader, ok := headers[AuthHeader] | |
| assert.True(t, ok) | |
| assert.NotEmptyf(t, authHeader) | |
| assert.True(t, strings.HasPrefix(authHeader, "Bearer")) | |
| delete(headers, AuthHeader) | |
| assert.Equal(t, tc.response, headers) | |
| } | |
| }) | |
| } |
Comment on lines
+169
to
+179
| {"", "", "", nil, nil, 0, OAuthErr{nil, "client id is mandatory"}}, | ||
| {clientID, "", "", nil, nil, 0, OAuthErr{nil, "client secret is mandatory"}}, | ||
| {clientID, "", tokenURL, nil, nil, 0, OAuthErr{nil, "client secret is mandatory"}}, | ||
| {clientID, clientSecret, "", nil, nil, 0, OAuthErr{nil, "token url is mandatory"}}, | ||
| {clientID, clientSecret, "invalid_url_format", nil, nil, 0, OAuthErr{nil, "empty host"}}, | ||
| {clientID, clientSecret, tokenURL, nil, nil, 0, nil}, | ||
| {clientID, "some_random_client_secret", tokenURL, nil, nil, 0, nil}, | ||
| {"some_random_client_id", clientSecret, tokenURL, nil, nil, 0, nil}, | ||
| {clientID, clientSecret, tokenURL, nil, nil, 1, nil}, | ||
| {clientID, "some_random_client_secret", tokenURL, nil, nil, 1, nil}, | ||
| {"some_random_client_id", clientSecret, tokenURL, nil, nil, 2, nil}, |
There was a problem hiding this comment.
Here is why you shouldn't use a struct without field names, you are providing a lot of fields with their zero values
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
Change 1: Added function NewOAuthConfig()
Background:
In case the tokenURL is not provided in the OAuthConfig, an error is encountered while trying to fetch the token. The error returned is from the url package itself and does not provide much guidance to the user on what needs to be fixed. Error message: Post "" unsupported protocol scheme ""
Fix Made:
A new function NewOAuthConfig() has been added which is responsible for the validation of OAuth config. Users are required to use this method instead of directly initialising the OAuthConfig struct.
Change 2: AuthStyle Made Configurable
Previously AuthStyle was hardcoded to
oauth2.AuthStyleInHeaderwhich has now been made configurable. In case no value is passed, it defaults to oauth2.AuthStyleAutoDetect.Breaking Changes (if applicable):
None
Additional Information:
We should consider converting the OAuthConfig struct to un-exported to enforce the usage of NewOAuthConfig() function
Checklist:
goimportandgolangci-lint.