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

fix: access_denied error handling for OAuth destinations #3853

Merged
merged 26 commits into from
Oct 9, 2023

Conversation

sanpj2292
Copy link
Contributor

@sanpj2292 sanpj2292 commented Sep 11, 2023

Description

https://github.com/rudderlabs/rudder-config-backend/pull/3643

Linear Ticket

Resolves INT-367
https://linear.app/rudderstack/issue/INT-367/testing-review-and-release

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

Sai Sankeerth added 2 commits September 12, 2023 07:57
Signed-off-by: Sai Sankeerth <sanpj2292@github.com>
@codecov
Copy link

codecov bot commented Sep 12, 2023

Codecov Report

Attention: 17 lines in your changes are missing coverage. Please review.

Comparison is base (1e415e0) 70.40% compared to head (c834a57) 70.67%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3853      +/-   ##
==========================================
+ Coverage   70.40%   70.67%   +0.26%     
==========================================
  Files         357      357              
  Lines       53646    53671      +25     
==========================================
+ Hits        37771    37933     +162     
+ Misses      13612    13477     -135     
+ Partials     2263     2261       -2     
Files Coverage Δ
regulation-worker/internal/delete/api/api.go 84.02% <100.00%> (+18.71%) ⬆️
services/oauth/oauth.go 87.43% <96.82%> (+30.66%) ⬆️
router/handle.go 72.36% <0.00%> (+0.47%) ⬆️

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sanpj2292 sanpj2292 self-assigned this Sep 12, 2023
@sanpj2292 sanpj2292 marked this pull request as ready for review September 12, 2023 13:29
},
}
stCode, response := api.OAuth.UpdateAuthStatusToInactive(dest, job.WorkspaceID, oAuthDetail.id)
jobStatus.Status = model.JobStatusAborted
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be JobStatusUndefined if stCode != http.StatusOK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does JobStatusUndefined signify in terms of regulation functionality ?

// Lint error fix
_, err := w.Write([]byte(cpResp.response))
if err != nil {
fmt.Printf("I'm here!!!! Some shitty response!!")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
fmt.Printf("I'm here!!!! Some shitty response!!")

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sanpj2292 this is still here 🙂

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 searching in my IDE, this is not found. Not sure if I'm missing something here 😅


srvMux.HandleFunc("/workspaces/{workspaceId}/destinations/{destinationId}/authStatus/toggle", func(w http.ResponseWriter, req *http.Request) {
if req.Method != http.MethodPut {
w.WriteHeader(http.StatusNotFound)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't "Method Not Allowed" make more sense? I know it's a test, but is this test reflecting real behaviour?

Suggested change
w.WriteHeader(http.StatusNotFound)
w.WriteHeader(http.StatusMethodNotAllowed)

param := chi.URLParam(req, reqParam)
if param == "" {
// This case wouldn't occur I guess
http.Error(w, fmt.Sprintf("Wrong url being sent: %v", reqParam), http.StatusInternalServerError)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the server gets an empty parameter, isn't that a Bad Request rather than a 500?

Sai Sankeerth added 6 commits October 8, 2023 21:06
@fracasula fracasula merged commit 0d30d3b into master Oct 9, 2023
35 checks passed
@fracasula fracasula deleted the feat.oauth-access-denied-handling branch October 9, 2023 15:01
sanpj2292 added a commit that referenced this pull request Oct 10, 2023
fracasula pushed a commit that referenced this pull request Oct 10, 2023
…ions (#3853) (#3959)

Revert "fix: access_denied error handling for OAuth destinations (#3853)"

This reverts commit 0d30d3b.
@sanpj2292 sanpj2292 restored the feat.oauth-access-denied-handling branch October 10, 2023 08:35
This was referenced Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants