Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
juju offers supports filtering by user #7906
Conversation
jameinel
reviewed
Oct 4, 2017
I'm a little curious why the CLI is --users and --consumers, but the internal names are AllowedUsers and ConnectedUsers. I think those line up 1:1, but it would be clearer if we used similar names.
It also seems a little unclear what should happen if you specify both. Is
juju offers --user foo --consumer bar
an 'and' operation (show me everything that foo is allowed to access that bar is currently connected to), is it an 'or' (show me everything foo has access to and everything that bar is currently using) should they be exclusive, etc.
The overall filtering seems ok, but I'm quite concerned that the CLI flags are backwards from my intuition, and the doc around them doesn't seem particularly clear.
| Examples: | ||
| $ juju offers | ||
| $ juju offers -m model | ||
| $ juju offers --interface db2 | ||
| $ juju offers --application mysql | ||
| + $ juju offers --user connected-user | ||
| + $ juju offers --consumer allowed-user |
jameinel
Oct 4, 2017
Owner
This seems backwards in my head. 'connected-user' seems to be 'consumer' (who is actively consuming it, vs who is allowed to connect to it).
It seems like there would also be a case for filtering on who is offering the endpoints, which could cause confusion around --user.
Regardless, I certainly matched them the other way around.
wallyworld
Oct 4, 2017
Owner
Ok, if you're confused then clearly something needs to be fixed. The intent is to allow filtering by
- who is connected now to the offer (ie all offers with active or suspended relations made by that user)
- who has permission to consumer the offer
I tried to keep the CLI args to one word. I considered something like:
$ juju offers --allowed-user fred
$ juju offers --connected-user mary
But thought those arg names were getting a bit long. But maybe not if it aids clarity.
| @@ -96,6 +114,8 @@ func (c *listCommand) SetFlags(f *gnuflag.FlagSet) { | ||
| c.ModelCommandBase.SetFlags(f) | ||
| f.StringVar(&c.applicationName, "application", "", "return results matching the application") | ||
| f.StringVar(&c.interfaceName, "interface", "", "return results matching the interface name") | ||
| + f.StringVar(&c.userName, "user", "", "return results matching the connected user name") | ||
| + f.StringVar(&c.consumerName, "consumer", "", "return results matching the consumer user name") |
jameinel
Oct 4, 2017
Owner
these help strings don't feel like they really disambiguate when user should be used vs consumer. And given my intuition was the opposite of what you programmed, we probably should be clearer here.
| AllowedUsers []string | ||
| + | ||
| + // ConnectedUsers are the users currently related to the offer. | ||
| + ConnectedUsers []string |
jameinel
Oct 4, 2017
Owner
The doc strings here seem to match with my understanding, but you clearly have a test case that --user goes to ConnectedUsers and --consumer goes to AllowedUsers.
wallyworld
Oct 4, 2017
Owner
--user does go to connected user (ie the offers where that --user has at least one relation to the offer)
--consumer does go to allowed user (ie the offers where the specified user is allowed to be a consumer)
| + | ||
| + var out []applicationOfferDoc | ||
| + for _, doc := range in { | ||
| + offerUsers, err := s.st.GetOfferUsers(doc.OfferUUID) |
jameinel
Oct 4, 2017
Owner
Is this going to scale poorly if most offers are public?
Would it be better to only load the UUIDs that we haven't filtered so far and check if the user is allowed to access them?
wallyworld
Oct 4, 2017
Owner
For Juju 2.3, all offers will be controller via ACLs. GetOfferUsers() is an existing API call that is being re-used here. We need to adjust things anyway when the JAAS model is supported.
wallyworld
Oct 4, 2017
Owner
because this is the last filter step done, the overall offers list has already been filtered down using the other 2 filters
| + AllowedUsers: []string{"mary"}, | ||
| + }) | ||
| + c.Assert(err, jc.ErrorIsNil) | ||
| + c.Assert(len(offers), gc.Equals, 1) |
jameinel
Oct 4, 2017
Owner
This doesn't seem correct to me. You've created 3 offers, allowed Mary to access 2 of them, but ListOffers(AllowedUsers:mary) is only giving you 1 offer back?
wallyworld
Oct 4, 2017
Owner
Getting 1 back is correct - the filtering is done on who has consume permission. The test sets up read permission as well to demonstrate that is filtered out
| + c.Assert(len(offers), gc.Equals, 1) | ||
| + c.Assert(offers[0], jc.DeepEquals, offer) | ||
| +} | ||
| + |
jameinel
Oct 4, 2017
Owner
This seems odd that you haven't allowed her to have access to the offer, but still connected to it. AddOfferConnection feels a bit like it should fail under this circumstance.
wallyworld
Oct 4, 2017
Owner
Adding a connection is orthogonal to permission - it simply records the existence of a connection for accounting purposes. The actual connection is made via a RegisterRelation() call where permission is indeed checked.
| + for i, oc := range connDocs { | ||
| + conns[i] = newOfferConnection(st, &oc) | ||
| + } | ||
| + return conns, nil |
jameinel
Oct 4, 2017
Owner
This doesn't appear to support the notion of "allow everyone to access this" or even "allow a group to access this", but only supports explicit grants to specific people. Is that correct?
wallyworld
Oct 4, 2017
Owner
everyone@external is a valid ACL user. But this offer connection entity is not the ACL, it is simply used to record a connection. The standard ACL model is used for permissions.
| + c.Assert(err, jc.ErrorIsNil) | ||
| + c.Assert(obtained, gc.HasLen, 1) | ||
| + c.Assert(obtained[0].OfferUUID(), gc.Equals, oc.OfferUUID()) | ||
| + c.Assert(obtained[0].UserName(), gc.Equals, oc.UserName()) |
jameinel
Oct 4, 2017
Owner
Should you have one more step where you grant the offer to mary and see that her list of connections changes from 0 to 1?
wallyworld
Oct 4, 2017
Owner
The offer connections collection doesn't record ACLs; it just records what relations are connected to an offer and the user who initiated the connection.
wallyworld
Oct 4, 2017
Owner
The test shows how filtering on connected users correctly returns 0 results and > 0 results
|
$$merge$$ |
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju |
wallyworld commentedOct 4, 2017
Description of change
The juju offers command supports filtering by consumer and connected user, ie users who are allowed to consumer the offer or users who are connected to the offer.
QA steps
bootstrap, create an offer, grant consumer to a user
make a connection to the offer
check that
$ juju offers --user fred
$ juju offers --consumer mary
behave as expected, and
$ juju offers --user admin works out of the box without an explicit grant
Documentation changes
CMR offers doc will be updated