[#365] Include iam user tags in query #365

Merged
dkirillov merged 1 commits from pogpp/frostfs-s3-gw:feature/add_user_tags_in_query into master 2024-04-22 12:18:06 +00:00
Collaborator

Signed-off-by: Pavel Pogodaev p.pogodaev@yadro.com

Signed-off-by: Pavel Pogodaev <p.pogodaev@yadro.com>
pogpp added 1 commit 2024-04-12 09:00:04 +00:00
/ Vulncheck (pull_request) Failing after 1m31s Details
/ DCO (pull_request) Successful in 1m42s Details
/ Builds (1.20) (pull_request) Successful in 2m12s Details
/ Builds (1.21) (pull_request) Successful in 1m39s Details
/ Lint (pull_request) Successful in 4m36s Details
/ Tests (1.20) (pull_request) Successful in 3m46s Details
/ Tests (1.21) (pull_request) Successful in 3m38s Details
80b4809ca5
[#291] Include iam user tags in query
Signed-off-by: Pavel Pogodaev <p.pogodaev@yadro.com>
pogpp force-pushed feature/add_user_tags_in_query from 80b4809ca5 to 1b562df3fc 2024-04-12 09:00:49 +00:00 Compare
pogpp changed title from [#291] Include iam user tags in query to [#365] Include iam user tags in query 2024-04-12 09:01:02 +00:00
dkirillov reviewed 2024-04-12 14:24:50 +00:00
@ -385,0 +384,4 @@
reqInfo := GetReqInfo(ctx)
queries := reqInfo.URL.Query()
for _, v := range reqInfo.GetTags() {
Collaborator

We must use user claims from frostfsid. Tags from reqInfo contain nothing.

You should extend method FrostFSIDInformer.GetUserGroupIDs that it returns not only groups but also all claims.
Also add tests please

We must use user claims from `frostfsid`. Tags from `reqInfo` contain nothing. You should extend method `FrostFSIDInformer.GetUserGroupIDs` that it returns not only groups but also all claims. Also add tests please
dkirillov marked this conversation as resolved
pogpp force-pushed feature/add_user_tags_in_query from 1b562df3fc to f0943fa580 2024-04-17 05:00:21 +00:00 Compare
pogpp force-pushed feature/add_user_tags_in_query from f0943fa580 to 2b15ead7b5 2024-04-17 05:01:55 +00:00 Compare
dkirillov reviewed 2024-04-17 06:44:29 +00:00
@ -250,6 +250,14 @@ func TestDefaultBehaviorPolicyChecker(t *testing.T) {
createBucketErr(chiRouter, ns, bktName, apiErrors.ErrAccessDenied)
}
func TestDefaultPolicyCheckerWithUserTags(t *testing.T) {
Collaborator

Could you clarify what this test does?

Could you clarify what this test does?
Collaborator

It would be nice to have tests that check if user can have access with certain claims

It would be nice to have tests that check if user can have access with certain claims
dkirillov marked this conversation as resolved
@ -127,1 +127,3 @@
return res, nil
tags := make(map[string]string)
for k, v := range subjExt.KV {
if strings.HasPrefix(k, "tag-") {
Collaborator

Let's rename this function to GetUserGroupIDsAndClaims and return not only tags but all KV claims

Let's rename this function to `GetUserGroupIDsAndClaims` and return not only tags but all KV claims
dkirillov marked this conversation as resolved
pogpp added 1 commit 2024-04-17 14:21:23 +00:00
/ DCO (pull_request) Successful in 1m23s Details
/ Vulncheck (pull_request) Failing after 2m28s Details
/ Builds (1.20) (pull_request) Failing after 2m44s Details
/ Builds (1.21) (pull_request) Failing after 2m38s Details
/ Lint (pull_request) Failing after 3m5s Details
/ Tests (1.20) (pull_request) Failing after 2m52s Details
/ Tests (1.21) (pull_request) Failing after 2m2s Details
e2e6794605
[#365] Include iam user tags in query
Signed-off-by: Pavel Pogodaev <p.pogodaev@yadro.com>
pogpp force-pushed feature/add_user_tags_in_query from e2e6794605 to e33ca77ec8 2024-04-17 14:24:08 +00:00 Compare
dkirillov reviewed 2024-04-18 06:38:34 +00:00
@ -253,0 +255,4 @@
ns, bktName := "", "bucket"
router.middlewareSettings.denyByDefault = true
allowOperations(router, ns, []string{"s3:CreateBucket"}, nil)
createBucket(router, ns, bktName)
Collaborator

The logic should be the following:

  • make denyByDefault to true
  • allow bucket creation if and only if user has specific claim
  • try create bucket (error AccessDenied must be got)
  • update FrostFSIDMock so it return appropriate tag for user
  • try create bucket (now it must be successfull)
The logic should be the following: * make denyByDefault to `true` * allow bucket creation if and only if user has specific claim * try create bucket (error `AccessDenied` must be got) * update `FrostFSIDMock` so it return appropriate tag for user * try create bucket (now it must be successfull)
dkirillov marked this conversation as resolved
Collaborator

Please, rebase

Please, rebase
pogpp force-pushed feature/add_user_tags_in_query from e33ca77ec8 to 608fc3d09b 2024-04-18 11:25:06 +00:00 Compare
pogpp requested review from alexvanin 2024-04-18 11:32:28 +00:00
pogpp requested review from mbiryukova 2024-04-18 11:32:40 +00:00
pogpp requested review from dkirillov 2024-04-18 11:32:49 +00:00
dkirillov reviewed 2024-04-18 11:40:58 +00:00
@ -125,3 +126,3 @@
func (h *handlerMock) HeadObjectHandler(http.ResponseWriter, *http.Request) {
//TODO implement me
// TODO implement me
Collaborator

Do we really need such changes?

Do we really need such changes?
Poster
Collaborator

It's GoLand autoformating - gimme just a moment

It's GoLand autoformating - gimme just a moment
dkirillov marked this conversation as resolved
Collaborator

Please, beatify commit. Now we have two commits with the same message. Squash them into one commit or separate more logically and mention such separation in commit messages

Please, beatify commit. Now we have two commits with the same message. Squash them into one commit or separate more logically and mention such separation in commit messages
pogpp force-pushed feature/add_user_tags_in_query from 608fc3d09b to e6bdcb4228 2024-04-18 14:23:58 +00:00 Compare
Collaborator

Rebase, please

Rebase, please
pogpp force-pushed feature/add_user_tags_in_query from e6bdcb4228 to e48b46d0ab 2024-04-19 05:57:47 +00:00 Compare
dkirillov reviewed 2024-04-19 06:57:45 +00:00
@ -129,0 +130,4 @@
func (f *FrostFSID) GetUserGroupIDsAndClaims(userHash util.Uint160) ([]string, map[string]string, error) {
subjExt, err := f.frostfsid.GetSubjectExtended(userHash)
if err != nil {
if strings.Contains(err.Error(), "not found") {
Collaborator

What is the purpose of this if ?

What is the purpose of this `if` ?
dkirillov marked this conversation as resolved
dkirillov reviewed 2024-04-19 07:00:54 +00:00
@ -127,2 +127,4 @@
return userKey, nil
}
func (f *FrostFSID) GetUserGroupIDsAndClaims(userHash util.Uint160) ([]string, map[string]string, error) {
Collaborator

It seems we must update GetUserGroupIDs (now it isn't used I suppose)

It seems we must update `GetUserGroupIDs` (now it isn't used I suppose)
Poster
Collaborator

Consequences of bad rebase conflict resolving

Consequences of bad rebase conflict resolving
dkirillov marked this conversation as resolved
pogpp force-pushed feature/add_user_tags_in_query from e48b46d0ab to 5e1f3a8189 2024-04-19 08:28:24 +00:00 Compare
dkirillov reviewed 2024-04-19 11:10:03 +00:00
@ -63,3 +63,3 @@
if strings.Contains(err.Error(), "not found") {
f.log.Debug(logs.UserGroupsListIsEmpty, zap.Error(err))
return nil, nil
return nil, nil, err
Collaborator

We must not return error

We must not return error
dkirillov marked this conversation as resolved
pogpp force-pushed feature/add_user_tags_in_query from 5e1f3a8189 to b902c9d999 2024-04-19 11:23:48 +00:00 Compare
pogpp force-pushed feature/add_user_tags_in_query from b902c9d999 to f206b87a93 2024-04-19 16:36:16 +00:00 Compare
dkirillov approved these changes 2024-04-22 06:31:23 +00:00
pogpp force-pushed feature/add_user_tags_in_query from f206b87a93 to 3c436d8de9 2024-04-22 07:47:56 +00:00 Compare
dkirillov approved these changes 2024-04-22 07:58:12 +00:00
mbiryukova approved these changes 2024-04-22 08:53:50 +00:00
dkirillov merged commit 3c436d8de9 into master 2024-04-22 12:18:06 +00:00
alexvanin added this to the v0.30.0 milestone 2024-05-27 11:16:30 +00:00
Sign in to join this conversation.
There is no content yet.