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(HMS-3185): fail with 403 on failed assume role #759

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

ezr-ondrej
Copy link
Member

@ezr-ondrej ezr-ondrej commented Nov 28, 2023

AssumeRole is supposed to assume the role that client is giving us. This fixes the error code when this operation fails.

It could have also been 401 as the role might be considered an input. Given this is a permissions operation, I went with 403.

Let me know if you think 401 (or any other code) would be more fitting.

I've went bit overboard to mimic the error that AWS returns in our stub.
But now it should test the real thing pretty closely.

@ezr-ondrej ezr-ondrej force-pushed the assume_role_forbidden branch 2 times, most recently from c130e54 to f1e53f7 Compare November 28, 2023 18:03
Copy link
Member

@lzap lzap left a comment

Choose a reason for hiding this comment

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

Tests failed, irrelevant tho I think.

internal/payloads/error_payload.go Show resolved Hide resolved
@ezr-ondrej
Copy link
Member Author

/retest

1 similar comment
@akhil-jha
Copy link
Member

/retest

AssumeRole is supposed to assume the role that client is giving us.
This fixes the error code when this operation fails.

It could have also been 401 as the role might be considered an input.
Given this is a permissions operation, we went with 403.
@ezr-ondrej
Copy link
Member Author

/retest

@ezr-ondrej
Copy link
Member Author

We are 🍏 lets get it in

@ezr-ondrej ezr-ondrej merged commit f63be27 into RHEnVision:main Nov 30, 2023
10 checks passed
@ezr-ondrej ezr-ondrej deleted the assume_role_forbidden branch November 30, 2023 14:44
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