Skip to content

feat(acl): add rules for ACL#306

Merged
braginini merged 5 commits intonetbirdio:mainfrom
gigovich:feat-access-control-rules
May 21, 2022
Merged

feat(acl): add rules for ACL#306
braginini merged 5 commits intonetbirdio:mainfrom
gigovich:feat-access-control-rules

Conversation

@gigovich
Copy link
Contributor

@gigovich gigovich commented May 5, 2022

No description provided.

@mlsmaycon mlsmaycon requested a review from braginini May 5, 2022 19:51
@gigovich gigovich force-pushed the feat-access-control-rules branch 2 times, most recently from 6df0bf0 to 21bb71a Compare May 15, 2022 14:39
@gigovich gigovich force-pushed the feat-access-control-rules branch from 21bb71a to 6bbb67f Compare May 15, 2022 14:52
@gigovich gigovich force-pushed the feat-access-control-rules branch from 6bbb67f to 37744b7 Compare May 15, 2022 15:07
@gigovich gigovich force-pushed the feat-access-control-rules branch 2 times, most recently from df66557 to f98b5de Compare May 17, 2022 15:00
@gigovich gigovich force-pushed the feat-access-control-rules branch from f98b5de to bcb5982 Compare May 18, 2022 15:13
mlsmaycon
mlsmaycon previously approved these changes May 18, 2022
return
}

writeJSONObject(w, account.Rules)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any particular reason why we return a map not a list of rules?
All other endpoints return [peers] or [users].
This one returns a map.

{
  "ca2im93lo1hs0mlug03g": {
    "ID": "ca2im93lo1hs0mlug03g",
    "Name": "Default",
    "Source": [
      "ca2im93lo1hs0mlug030"
    ],
    "Destination": [
      "ca2im93lo1hs0mlug030"
    ],
    "Flow": 0
  }
}

I propose returning rules in such a structure:

[
   {
      "ID":"ca2im93lo1hs0mlug03g",
      "Name":"Default",
      "Source":[
         "ca2im93lo1hs0mlug030"
      ],
      "Destination":[
         "ca2im93lo1hs0mlug030"
      ],
      "Flow":0
   }
]

It will be much easier to handle on the UI side because all the Table components are built to handle an array.
Also this way we are more consistent with our API

P.S. The same applies to the /api/groups endpoint

Copy link
Collaborator

@braginini braginini May 18, 2022

Choose a reason for hiding this comment

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

I also think that returning account.Rules is a bit dangerous because it is our internal structure.
It might be that we rename fields internally or would like to change how fields are returned.
I'd suggest creating a RuleResponse struct to return (see PeerRersponse for example). Let's be consistent here as well.

P.S. The same applies to the /api/groups endpoint

Copy link
Collaborator

Choose a reason for hiding this comment

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

See proposed changes below

return
}

writeJSONObject(w, account.Rules)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
writeJSONObject(w, account.Rules)
var respBody []*RuleResponse
for _, rule := range account.Rules {
respBody = append(respBody, toRuleResponse(rule))
}
writeJSONObject(w, respBody)

authAudience string
jwtExtractor jwtclaims.ClaimsExtractor
}

Copy link
Collaborator

@braginini braginini May 18, 2022

Choose a reason for hiding this comment

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

Suggested change
type RuleResponse struct {
ID string
Name string
Sources []string
Destinations []string
Flow server.TrafficFlowType
Enabled bool
}

}

return account, nil
}
Copy link
Collaborator

@braginini braginini May 18, 2022

Choose a reason for hiding this comment

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

Suggested change
}
}
func toRuleResponse(rule *server.Rule) *RuleResponse {
return &RuleResponse{
Name: rule.Name,
ID: rule.ID,
Destinations: rule.Destination,
Sources: rule.Source,
Flow: rule.Flow,
Enabled: true,
}
}

Copy link
Collaborator

@braginini braginini left a comment

Choose a reason for hiding this comment

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

proposed changes applied

See
#306 (comment)

@braginini
Copy link
Collaborator

Another point.

We return Source and Destination groups IDs but no Name.
In the UI we will need to show the Name of the group in Source and Destination.
I suggest combining it already on the server-side. Any thoughts?

This:

[
  {
    "ID": "ca2im93lo1hs0mlug03g",
    "Name": "Default",
    "Source": [
      "ca2im93lo1hs0mlug030"
    ],
    "Destination": [
      "ca2im93lo1hs0mlug030"
    ],
    "Flow": 0
  }
]

Replace With this

[
  {
    "ID": "ca2im93lo1hs0mlug03g",
    "Name": "Default",
    "Source": [
     { "ca2im93lo1hs0mlug030", "Name": "All" }
    ],
    "Destination": [
      { "ca2im93lo1hs0mlug030", "Name": "All" }
    ],
    "Flow": 0
  }
]

@gigovich gigovich force-pushed the feat-access-control-rules branch from cc13eb9 to b6799c4 Compare May 20, 2022 15:48
Copy link
Collaborator

@braginini braginini left a comment

Choose a reason for hiding this comment

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

It seems like a peer is not removed from the groups when peer is deleted from the account

management Server fails with NPE when:

  1. add 2 peers
  2. remove 1 peer
  3. add 1 peer <-NPE on am.GetNetworkMap in
		for _, pid := range g.Peers {
			peer := account.Peers[pid]
			// exclude original peer
			if peer.Key != peerKey {
				res = append(res, peer.Copy())
			}
		}
	}

@braginini braginini merged commit 3ce3ccc into netbirdio:main May 21, 2022
@gigovich gigovich deleted the feat-access-control-rules branch June 2, 2023 06:52
pulsastrix pushed a commit to pulsastrix/netbird that referenced this pull request Dec 24, 2023
Add rules HTTP endpoint for frontend - CRUD operations.
Add Default rule - allow all.
Send network map to peers based on rules.
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