TrueCloudLab/frostfs-s3-gw
17
3
Fork 19
Code Issues 28 Pull requests 6 Projects Releases 18 Packages Activity Actions

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

Merged
dkirillov merged 1 commit from pogpp/frostfs-s3-gw:feature/add_user_tags_in_query into master 2024-04-22 12:18:06 +00:00
Conversation 3 Commits 1 Files changed 4 +33 -10
pogpp commented 2024-04-12 09:00:02 +00:00
Member
Copy link

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

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
api/middleware/policy.go Outdated
@ -385,0 +384,4 @@
reqInfo := GetReqInfo(ctx)
queries := reqInfo.URL.Query()
for _, v := range reqInfo.GetTags() {
dkirillov commented 2024-04-12 14:24:50 +00:00
Member
Copy link

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
👍 1
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
api/router_test.go Outdated
@ -250,6 +250,14 @@ func TestDefaultBehaviorPolicyChecker(t *testing.T) {
createBucketErr(chiRouter, ns, bktName, apiErrors.ErrAccessDenied)
}
func TestDefaultPolicyCheckerWithUserTags(t *testing.T) {
dkirillov commented 2024-04-17 06:43:14 +00:00
Member
Copy link

Could you clarify what this test does?

Could you clarify what this test does?
dkirillov commented 2024-04-17 06:44:25 +00:00
Member
Copy link

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
internal/frostfs/frostfsid/frostfsid.go Outdated
@ -127,1 +127,3 @@
return res, nil
tags := make(map[string]string)
for k, v := range subjExt.KV {
if strings.HasPrefix(k, "tag-") {
dkirillov commented 2024-04-17 06:42:12 +00:00
Member
Copy link

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 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
api/router_test.go Outdated
@ -253,0 +255,4 @@
ns, bktName := "", "bucket"
router.middlewareSettings.denyByDefault = true
allowOperations(router, ns, []string{"s3:CreateBucket"}, nil)
createBucket(router, ns, bktName)
dkirillov commented 2024-04-18 06:38:34 +00:00
Member
Copy link

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)
👍 1
dkirillov marked this conversation as resolved
dkirillov commented 2024-04-18 06:38:50 +00:00
Member
Copy link

Please, rebase

Please, rebase
👍 1
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
api/router_mock_test.go Outdated
@ -125,3 +126,3 @@
func (h *handlerMock) HeadObjectHandler(http.ResponseWriter, *http.Request) {
//TODO implement me
// TODO implement me
dkirillov commented 2024-04-18 11:40:58 +00:00
Member
Copy link

Do we really need such changes?

Do we really need such changes?
pogpp commented 2024-04-18 14:16:04 +00:00
Author
Member
Copy link

It's GoLand autoformating - gimme just a moment

It's GoLand autoformating - gimme just a moment
dkirillov marked this conversation as resolved
dkirillov commented 2024-04-18 11:44:16 +00:00
Member
Copy link

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
👀 1
pogpp force-pushed feature/add_user_tags_in_query from 608fc3d09b to e6bdcb4228 2024-04-18 14:23:58 +00:00 Compare
dkirillov commented 2024-04-18 14:47:51 +00:00
Member
Copy link

Rebase, please

Rebase, please
👍 1
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
internal/frostfs/frostfsid/frostfsid.go Outdated
@ -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") {
dkirillov commented 2024-04-19 06:57:45 +00:00
Member
Copy link

What is the purpose of this if ?

What is the purpose of this `if` ?
👍 1
dkirillov marked this conversation as resolved
dkirillov reviewed 2024-04-19 07:00:54 +00:00
internal/frostfs/frostfsid/frostfsid.go Outdated
@ -127,2 +127,4 @@
return userKey, nil
}
func (f *FrostFSID) GetUserGroupIDsAndClaims(userHash util.Uint160) ([]string, map[string]string, error) {
dkirillov commented 2024-04-19 07:00:54 +00:00
Member
Copy link

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)
👍 1
pogpp commented 2024-04-19 08:29:55 +00:00
Author
Member
Copy link

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
internal/frostfs/frostfsid/frostfsid.go Outdated
@ -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
dkirillov commented 2024-04-19 11:10:03 +00:00
Member
Copy link

We must not return error

We must not return error
👍 1
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.
Reviewers
No reviewers
alexvanin
dkirillov
mbiryukova
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
good first issue

Good for newcomers

  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

No labels
P0
P1
P2
P3
good first issue
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
Milestone
Clear milestone
No items
No milestone
v0.30.0
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-s3-gw#365
No description provided.
Delete branch "pogpp/frostfs-s3-gw:feature/add_user_tags_in_query"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?