Conversation
…ntViolation, ErrNoCredentials, ErrNoValidRole, ErrNotAuthorized, ErrNoAuthzConfigured
| } | ||
| if err := verifyNoPrivilegeEscalation(id.Roles(), roles); err != nil { | ||
| return nil, errox.NewErrNotAuthorized(err.Error()) | ||
| return nil, errox.NotAuthorized.CausedBy(err.Error()) |
There was a problem hiding this comment.
| return nil, errox.NotAuthorized.CausedBy(err.Error()) | |
| return nil, errox.NotAuthorized.CausedBy(err) |
Might go to a separate PR with refactorings.
There was a problem hiding this comment.
@0x656b694d I would do this in separate PR.
|
Tag for build #479203 is 💻 For deploying this image using the dev scripts, run the following first: export MAIN_IMAGE_TAG='3.69.x-527-g9cde10b0e3'🕹️ A |
.golangci.yml
Outdated
|
|
||
| exclude-rules: | ||
| - path: "(central|compliance|integration-tests|local|migrator|operator|pkg|sensor|tests|tools|scale)/" | ||
| - path: "(central|compliance|integration-tests|local|migrator|operator|pkg|roxctl|sensor|tests|tools|scale)/" |
There was a problem hiding this comment.
Why do we need that? If we exclude it here then we can disable this linter
There was a problem hiding this comment.
@janisz, Thank you for pointing this out!
I thought that other parts were excluded because they use CausedBy. But then I have checked commit history, and actually, it was like that from the beginning. I have changed Regex because it was there for wrappers that we have replaced.
✅
| func (s *serviceImpl) GetAuthProvider(_ context.Context, request *v1.GetAuthProviderRequest) (*storage.AuthProvider, error) { | ||
| if request.GetId() == "" { | ||
| return nil, errox.NewErrInvalidArgs("auth provider id is required") | ||
| return nil, errox.InvalidArgs.CausedBy("auth provider id is required") |
There was a problem hiding this comment.
This does not read right. auth provider id is required hints remediation whereas no auth provider id provided is the cause.
| } | ||
| if providerReq.GetId() != "" { | ||
| return nil, errox.NewErrInvalidArgs("auth provider id must be empty") | ||
| return nil, errox.InvalidArgs.CausedBy("auth provider id must be empty") |
There was a problem hiding this comment.
Same as above. Consider adjusting the msgs for all such instances.
There was a problem hiding this comment.
@md2119 I have adjusted messages for this file. ✅
I understand your concerns, and they are absolutely valid! But we should restrain ourselves from further such changes because they are out of the scope of this issue, and it would just expand the PR and make the review complicated. We should instead collect such cases and make follow-ups to address them.
There was a problem hiding this comment.
@md2119 I have adjusted messages for this file. ✅
Thanks for addressing the comments!
I understand your concerns, and they are absolutely valid! But we should restrain ourselves from further such changes because they are out of the scope of this issue, and it would just expand the PR and make the review complicated. We should instead collect such cases and make follow-ups to address them.
Yea, you make a valid argument. Cascading changes are not encouraged. In this case, we are changing the semantics, hence, the things that the new framework applies to, or what new framework consumes, or what the framework depends become conflicting. For example, if it were a Wrap() func, the relevance of the string wouldn't be an issue.
|
| GitGuardian id | Secret | Commit | Filename | |
|---|---|---|---|---|
| 3038029 | Generic High Entropy Secret | 138264d | qa-tests-backend/src/main/groovy/services/NotifierService.groovy | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Our GitHub checks need improvements? Share your feedbacks!
| func (s *serviceImpl) GetAuthProvider(_ context.Context, request *v1.GetAuthProviderRequest) (*storage.AuthProvider, error) { | ||
| if request.GetId() == "" { | ||
| return nil, errox.NewErrInvalidArgs("auth provider id is required") | ||
| return nil, errox.InvalidArgs.CausedBy("auth provider id is required") |
There was a problem hiding this comment.
This does not read right. auth provider id is required hints remediation whereas no auth provider id provided is the cause.
4 participants
Description
Next step of unifying error handling.
We have removed the following helper functions:
We have also excluded
rxoctlfromwrapchecklinting step. Otherwise, we are getting errors onfunc (e *RoxError) CausedBy().Note: This PR is based on
mtodor/ROX-10259-remove-deprecated-errorsbranch. After PR #1372 is merged, we will change the base branch.Checklist
[ ] Unit test and regression tests added- nothing is added[ ] Evaluated and added CHANGELOG entry if required- not relevant[ ] Determined and documented upgrade steps- nothing relevant for upgrade steps[ ] Documented user facing changes (create PR based on stackrox/openshift-docs and merge into rhacs-docs)- no user-facing changesTesting Performed
The following things are tested locally:
make imageworksmake styleworks so that the linting of all go files is correct