Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions app/cli/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,10 @@ func errorInfo(err error, logger zerolog.Logger) (string, int) {
msg = "your authentication token has expired, please run chainloop auth login again"
case isWrappedErr(st, jwtMiddleware.ErrMissingJwtToken):
msg = "authentication required, please run \"chainloop auth login\""
case v1.IsUserNotMemberOfOrgErrorNotInOrg(err), v1.IsUserWithNoMembershipErrorNotInOrg(err):
msg = "the organization you are trying to access does not exist, please run \"chainloop auth login\""
case v1.IsUserNotMemberOfOrgErrorNotInOrg(err):
msg = "the organization you are trying to access does not exist or you are not part of it, please run \"chainloop auth login\""
case v1.IsUserWithNoMembershipErrorNotInOrg(err):
msg = "you are not part of any organization, please run \"chainloop organization create --name ORG_NAME\" to create one"
Copy link
Member

Choose a reason for hiding this comment

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

thanks for the PR!

I think we might need to split the switch into the two cases, v1.IsUserNotMemberOfOrgErrorNotInOrg(err), might not mean that tit doesn't have any org but instead that it's not part of this specific org.

Let me review the current custom error throwing, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Hi, thanks again for providing a PR but I although I think it fixes the issue, it adds a regression where we will tell users that they do not have an organization when they just don't have access to it.

The following change, indeed fixes the issue

diff --git a/app/cli/main.go b/app/cli/main.go
index ff807fce..ca1b9540 100644
--- a/app/cli/main.go
+++ b/app/cli/main.go
@@ -89,8 +89,10 @@ func errorInfo(err error, logger zerolog.Logger) (string, int) {
                msg = "your authentication token has expired, please run chainloop auth login again"
        case isWrappedErr(st, jwtMiddleware.ErrMissingJwtToken):
                msg = "authentication required, please run \"chainloop auth login\""
-       case v1.IsUserNotMemberOfOrgErrorNotInOrg(err), v1.IsUserWithNoMembershipErrorNotInOrg(err):
-               msg = "the organization you are trying to access does not exist, please run \"chainloop auth login\""
+       case v1.IsUserNotMemberOfOrgErrorNotInOrg(err):
+               msg = "the organization you are trying to access does not exist or you are not part of it, please run \"chainloop auth login\""
+       case v1.IsUserWithNoMembershipErrorNotInOrg(err):
+               msg = "you are not part of any organization, please run \"chainloop organization create --name ORG_NAME\" to create one"
        case errors.As(err, &cmd.GracefulError{}):
                // Graceful recovery if the flag is set and the received error is marked as recoverable
                if cmd.GracefulExit {

what I do is to make sure the error handling is split between the two cases

a) when the user doesn't have any organization
b) when the user doesn't have access to the provided organization

The original issue #2088 was about making sure that during the attestation process, we show a) not b), and this change fixes it.

Feel free to apply those changes in your branch and let me know.

Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the feedback! You're right, that was causing a misleading error. I've applied your suggested changes to handle both cases separately.

case errors.As(err, &cmd.GracefulError{}):
// Graceful recovery if the flag is set and the received error is marked as recoverable
if cmd.GracefulExit {
Expand Down
Loading