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.

Sign up for GitHub

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

Add RequestProjectRole API #353

Merged
merged 12 commits into from
Mar 6, 2024
+1,897 −507
Merged

Add RequestProjectRole API #353

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
9bda66a
add api
senk8 Feb 21, 2024
614d836
fix proto
senk8 Feb 22, 2024
ceaae81
fix vallidate
senk8 Feb 26, 2024
cc06917
add message
senk8 Feb 27, 2024
be64f3b
fix mention
senk8 Feb 27, 2024
0e3af5c
fix message
senk8 Feb 27, 2024
4eb56c3
fix according to review
senk8 Feb 29, 2024
79eb78c
fix sql
senk8 Feb 29, 2024
8d64404
fix message
senk8 Feb 29, 2024
211ca64
add test
senk8 Mar 1, 2024
ffd28fd
delete return check
senk8 Mar 5, 2024
b5c1cba
delete from=slack
senk8 Mar 5, 2024
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
12 changes: 2 additions & 10 deletions pkg/server/alert/alert_notification.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,25 +226,16 @@ func (a *AlertService) RequestProjectRoleNotification(ctx context.Context, req *
return nil, err
}
notifications, err := a.repository.ListNotification(ctx, req.ProjectId, "slack", 0, time.Now().Unix())
gassara-kys marked this conversation as resolved.
Show resolved Hide resolved
notification := (*notifications)[0]
if err != nil {
Copy link
Contributor Author

@senk8 senk8 Mar 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

ここの書き方は他の部分では

	if err != nil {
		if errors.Is(err, gorm.ErrRecordNotFound) {
			return &empty.Empty{}, nil
		}
		return nil, err
	}

となっていますが、

  • このAPIは管理者に権限リクエストを送るもので、何らかのバグで送ることができなかった場合にはエラーをそのまま返してしまうのがよいと考えた

ので、errをそのまま返却するようにしました。

if errors.Is(err, gorm.ErrRecordNotFound) {
return &empty.Empty{}, nil
}
return nil, err
}
notification := (*notifications)[0]
projects, err := a.projectClient.ListProject(ctx, &project.ListProjectRequest{ProjectId: req.ProjectId})
if err != nil {
if errors.Is(err, gorm.ErrRecordNotFound) {
return &empty.Empty{}, nil
}
return nil, err
}
user, err := a.iamClient.GetUser(ctx, &iam.GetUserRequest{UserId: req.UserId})
if err != nil {
if errors.Is(err, gorm.ErrRecordNotFound) {
return &empty.Empty{}, nil
}
return nil, err
}
switch notification.Type {
Expand All @@ -256,6 +247,7 @@ func (a *AlertService) RequestProjectRoleNotification(ctx context.Context, req *
}
default:
a.logger.Warnf(ctx, "This notification_type is unimplemented. type: %v", notification.Type)
return nil, fmt.Errorf("This notification_type is unavailable. type: %v", notification.Type)
Copy link
Contributor Author

@senk8 senk8 Mar 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

コメント通りにここにエラーを追加しました。ログメッセージではunimplementedですがエラーメッセージとしてはユーザー目線でunavailableとしました。

Copy link
Collaborator

Choose a reason for hiding this comment

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

エラーメッセージの場合は最初小文字でお願いします。

}
return &empty.Empty{}, nil
}
Expand Down
158 changes: 158 additions & 0 deletions pkg/server/alert/alert_notification_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,18 @@ import (
"github.com/ca-risken/common/pkg/logging"
"github.com/ca-risken/core/pkg/db/mocks"
"github.com/ca-risken/core/pkg/test"
"github.com/golang/protobuf/ptypes/empty"
"github.com/stretchr/testify/mock"
"gorm.io/gorm"

"github.com/ca-risken/core/pkg/model"
"github.com/ca-risken/core/proto/alert"
"github.com/ca-risken/core/proto/finding"
findingmock "github.com/ca-risken/core/proto/finding/mocks"
"github.com/ca-risken/core/proto/iam"
iammock "github.com/ca-risken/core/proto/iam/mocks"
"github.com/ca-risken/core/proto/project"
projectmock "github.com/ca-risken/core/proto/project/mocks"
"github.com/jarcoal/httpmock"
)

Expand Down Expand Up @@ -327,3 +331,157 @@ func TestPutNotification(t *testing.T) {
})
}
}

func TestRequestProjectRoleNotification(t *testing.T) {
var ctx context.Context
type mockListNotification struct {
Resp *[]model.Notification
Err error
}
type mockListProject struct {
Resp *project.ListProjectResponse
Err error
}
type mockGetUser struct {
Resp *iam.GetUserResponse
Err error
}
now := time.Now()
cases := []struct {
name string
input *alert.RequestProjectRoleNotificationRequest
want *empty.Empty
Copy link
Collaborator

Choose a reason for hiding this comment

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

ここは、レスポンス内容は問わないので、削っても良いかもと思いました（各テストケースでエラーが返されるか返されないかだけ見れば良い（wantErrのみ見れば良い））

wantErr bool
listNotification mockListNotification
listProject mockListProject
getUser mockGetUser
}{
{
name: "OK Request project role",
input: &alert.RequestProjectRoleNotificationRequest{ProjectId: 1001, UserId: 1001},
want: &empty.Empty{},
wantErr: false,
listNotification: mockListNotification{
Resp: &[]model.Notification{
{ProjectID: 1001, Name: "name", Type: "slack", NotifySetting: `{"webhook_url": "https://example.com"}`, CreatedAt: now, UpdatedAt: now},
},
Err: nil,
},
getUser: mockGetUser{
Resp: &iam.GetUserResponse{
User: &iam.User{UserId: 1001, Name: "userName"},
},
Err: nil,
},
listProject: mockListProject{
Resp: &project.ListProjectResponse{
Project: []*project.Project{
{ProjectId: 1001, Name: "projectName"},
},
},
Err: nil,
},
},
{
name: "NG unimplemented notification type",
input: &alert.RequestProjectRoleNotificationRequest{ProjectId: 1001, UserId: 1001},
want: nil,
wantErr: true,
listNotification: mockListNotification{
Resp: &[]model.Notification{
{ProjectID: 1001, Name: "name", Type: "unimplemented", NotifySetting: `{"webhook_url": "https://example.com"}`, CreatedAt: now, UpdatedAt: now},
},
Err: nil,
},
listProject: mockListProject{
Resp: &project.ListProjectResponse{
Project: []*project.Project{
{ProjectId: 1001, Name: "projectName"},
},
},
Err: nil,
},
getUser: mockGetUser{
Resp: &iam.GetUserResponse{
User: &iam.User{UserId: 1001, Name: "userName"},
},
Err: nil,
},
},
{
name: "NG ListNotification (Notification Not Found)",
input: &alert.RequestProjectRoleNotificationRequest{ProjectId: 1001, UserId: 1001},
want: nil,
wantErr: true,
listNotification: mockListNotification{
Resp: &[]model.Notification{},
Err: gorm.ErrRecordNotFound,
},
},
{
name: "NG ListProject (API Error)",
input: &alert.RequestProjectRoleNotificationRequest{ProjectId: 1001, UserId: 1001},
want: nil,
wantErr: true,
listNotification: mockListNotification{
Resp: &[]model.Notification{
{ProjectID: 1001, Name: "name", Type: "slack", NotifySetting: `{"webhook_url": "https://example.com"}`, CreatedAt: now, UpdatedAt: now},
},
Err: nil,
},
listProject: mockListProject{
Resp: &project.ListProjectResponse{
Project: []*project.Project{
{ProjectId: 1001, Name: "projectName"},
},
},
Err: errors.New("api error"),
},
},
{
name: "NG GetUser (API Error)",
input: &alert.RequestProjectRoleNotificationRequest{ProjectId: 1001, UserId: 1001},
want: nil,
wantErr: true,
listNotification: mockListNotification{
Resp: &[]model.Notification{
{ProjectID: 1001, Name: "name", Type: "slack", NotifySetting: `{"webhook_url": "https://example.com"}`, CreatedAt: now, UpdatedAt: now},
},
Err: errors.New("api error"),
},
listProject: mockListProject{
Resp: &project.ListProjectResponse{
Project: []*project.Project{
{ProjectId: 1001, Name: "projectName"},
},
},
Err: nil,
},
getUser: mockGetUser{
Resp: &iam.GetUserResponse{
User: &iam.User{UserId: 1001, Name: "userName"},
},
Err: gorm.ErrRecordNotFound,
},
},
}
for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
mockDB := mocks.NewAlertRepository(t)
mockDB.On("ListNotification", test.RepeatMockAnything(5)...).Return(c.listNotification.Resp, c.listNotification.Err).Once()
mockProject := projectmock.ProjectServiceClient{}
mockProject.On("ListProject", mock.Anything, mock.Anything).Return(c.listProject.Resp, c.listProject.Err)
mockIAM := iammock.IAMServiceClient{}
mockIAM.On("GetUser", mock.Anything, mock.Anything).Return(c.getUser.Resp, c.getUser.Err)

svc := AlertService{projectClient: &mockProject, iamClient: &mockIAM, repository: mockDB, logger: logging.NewLogger()}
got, err := svc.RequestProjectRoleNotification(ctx, c.input)
if err != nil && !c.wantErr {
t.Fatalf("Unexpected error: %+v", err)
}
if !reflect.DeepEqual(got, c.want) {
t.Fatalf("Unexpected response: want=%+v, got=%+v", c.want, got)
}
})
}
}