chat: handle suffixes and readers when adding username to TLF

[strib]

Otherwise a valid TLF name like "a,b (conflicted copy 2019-02-14 #1)" becomes "a,b (conflicted copy 2019-02-14 #1),a" and everything breaks.

[mmaxim]

Why is this needed? I was hoping to not have to think about these suffixes again.

[strib]

@mmaxim so we can get the kbfsfileedits inbox for say /keybase/private/a,b (conflicted copy 2017-04-21). With the current code, GetInboxAndUnboxLocal returns an error. This happened today to someone (see the keybase#kbfs channel).

[mmaxim]

Maybe all this stuff could get moved to a helper function that only runs if membersType == chat1.ConversationMembersType_KBFSFILEEDIT. I'd like to keep any change here narrow for this fix.

[strib]

@mmaxim I can do that, but it seems like this function is just broken, and if any future users try to use it for non-standard TLF names under a different chat type, it will just break. I would prefer not to have broken code in the codebase.

Note that the changes I made were ported over directly from assertion.go:

client/go/libkb/assertion.go

Lines 789 to 811 in 4aeb542

	split1 := strings.SplitN(s, " ", 2) // split1: [assertions, ?conflict]
	split2 := strings.Split(split1[0], "#") // split2: [writers, ?readers]
	if len(split2) > 2 {
	return ret, NewImplicitTeamDisplayNameError("can have at most one '#' separator")
	}
	seen := make(map[string]bool)
	var readers, writers keybase1.ImplicitTeamUserSet
	writers, err = parseImplicitTeamUserSet(ctx, split2[0], seen)
	if err != nil {
	return ret, err
	}
	if writers.NumTotalUsers() == 0 {
	return ret, NewImplicitTeamDisplayNameError("need at least one writer")
	}
	if len(split2) == 2 {
	readers, err = parseImplicitTeamUserSet(ctx, split2[1], seen)
	if err != nil {
	return ret, err
	}
	}

So the code is already being exercised on probably every chat path anyway, just not in this function, making it inconsistent.

The other alternative is just not calling this function at all for kbfs RPCs, because we'll always give you a TLF name that includes the logged-in user. I guess that would be the most efficient thing, but it still leaves the code very broken.

[mmaxim]

Alternative sounds great, just exit this function if membersType == chat1.ConversationMembersType_KBFSFILEEDIT.

[strib]

Ok. I'll leave a big comment that this function is completely broken for non-standard TLFs.

[mmaxim]

The notion of a TLF name in chat isn't really a thing anymore. They are called that, but only because of the now defunct dependency on KBFS. There is no way to create anything other than a list of usernames, which is all this function is trying to do. The hope going forward is to not have to think about suffixes or readers or whatever else (stuff that would have never been a factor in a program that didn't have a connection to KBFS), and that all works except for this particular case where this other force is putting these names in there (and we aren't stopping it).

[strib]

Actually @mmaxim on second thought, will bailing out of this function early break command line API calls for kbfsfileedit content?

I assume there are still old conversations with suffixes etc that still could get tripped up by this function.

[strib]

@mmaxim ping on above question.

Also, I just tried creating a conflicting conversation by making one conversation with a user, one conversation with a social assertion (and the same thing with the corresponding folders), and then later proving the social assertion. Not only does it give me two duplicate chats in my inbox (probably known and maybe desired?), but if I click the chat button from the folder with the "conflicting" suffix, it creates yet a third entry. Interestingly, it does not seems to exercise the function I changed here (doing it with or without my PR yields the same behavior, and the "malformed suffix" error doesn't show up in the log).

So I'm still not sure why we shouldn't change this function -- it doesn't seem to affect current behavior except in the kbfs case, and it's very broken. But I'll change it to have my alternate version above if you don't think it will break API calls for kbfsfileedit chat content.

[strib]

Another option might be to factor out the existing code of the assertion parser and re-use it here, if you think that will be safer.

Also happy to add like 20 TLF-name-parsing tests to the test file here, if you want. Should be pretty easy.

[mmaxim]

It is intended to give you two conversations. I don't know what the folder icon stuff is doing, but likely it is creating one with the suffix on it to get the third, which isn't great. That stuff should be dropping the suffix on whatever operation it is performing.

I think the real mistake on my part is not just rejecting these TLF names with suffixes on them for any of the implicit team backed conversations. Right now it just sends them through the implicit team name parser which will just accept it, which is unfortunate, but seemingly required for the KBFS file edit stuff to work now.

[strib]

Yes, it's required to get the edit history for conflict folders

What does that mean it terms of this PR? Which version do you prefer? Options:

1. This PR as is. (I can add tests if that helps.)
2. A helper function that's only called for kbfsfileedit types, which addresses the suffix issue.
3. Same as above, but that helper is shared with assertion.go.

[mmaxim]

Ok we can just do this, although a test wouldn't hurt. It would also be great to put a comment above all this logic that mentions that KBFS can create chats with these kinds of names and this helps support that.