fix(perforce) Fix support for p4breaker workaround scripts#63611
Conversation
| return nil, errors.Wrap(err, "failed to unmarshal protect line") | ||
| } | ||
|
|
||
| if parsedLine.DepotFile == "" { |
There was a problem hiding this comment.
I wonder if we should turn this on with an env var - as it requires that script to be correctly deployed anyways.
In addition, is there something we could do here to make sure things that don't parse properly (i.e. this json thing that doesn't contain any data) will not cause all perms to be dropped, and fail instead?
There was a problem hiding this comment.
We could always simply return an error if DepotFile is empty. I don't know what we would do with an empty DepotFile field anyways?
There was a problem hiding this comment.
Yeah I guess that’s good, better than this silent failure we had here
| return nil, errors.New("not a valid protects response") | ||
| } | ||
|
|
||
| protectsJson, err := url.QueryUnescape(parsedBrokerResponse.Data) |
There was a problem hiding this comment.
have you tried to fix the broker script already? was urlencoding in there easy? If not, maybe worth trying an alternative format like base64 which would be very easy to handle and be sure it handles non-ascii characters well, too.
There was a problem hiding this comment.
Yeah I adjusted the broker script. URL encoding was easy. But I'm switching to base64 now simply because I like it more
…works' of github.com:sourcegraph/sourcegraph into petrilast-src-441-perforce-broker-workaround-no-longer-works
…works' of github.com:sourcegraph/sourcegraph into petrilast-src-441-perforce-broker-workaround-no-longer-works
| } | ||
|
|
||
| type perforceBrokerJSONProtect struct { | ||
| Data string `json:"data"` // URL encoded JSON |
There was a problem hiding this comment.
This comment is wrong - and if you use []byte here I think you can save on the decode string part further down
When we switched to JSON parsing for
p4 protectscommands (adding the-Mj -ztagflags) we accidentally broke support for a work around some customers use when it comes to perforce permissions.The p4broker workaround
Sourcegraph requires a user with access to the perforce protects table. In perforce, this user needs
superaccess, which is a hard pill to swallow for certain security setups. Customers can work around this by using Helix Broker and creating a filter for the Sourcegraph user. The filter intercepts the protects command and executes a different script that performs thep4 protectscommand as a super user, and then sends the output of that command back as a message.The issue
When executing the script and sending the output of the script back as a message, it is technically no longer a Perforce server response. It's a debug message meant to be displayed to the user. Without the
-Mj -ztagflags, this is not much of an issue, because the output is displayed directly anyways. However, with the JSON flags thep4CLI now JSON encodes the debug message, NOT the actual server response, and it encodes it as{"data": "{message}", "level": 0}, with all of the response dumped in{message}. This is not what Sourcegraph expects, but it is still valid JSON, so it leads to no errors and zero perms for the user.The fix
The fix comes in two parts: this PR and changes to the script being used.
The script
-Mj -ztagflags to itsp4 protectscommand as wellp4CLI strips messages of all quotation marks ("). See here at the end of the "Filter programs" section. Stripping the quotes breaks the JSON encoding. By base64-encoding the JSON output, we can decode it again on Sourcegraph.This PR
This PR makes some changes to the way we decode the protects response. First we try to decode it the normal way. If the decoded response has empty fields, we assume it to be a broker response and try decoding the
"data"field instead. If this succeeds, we URL-decode the"data"field, and then we try to decode the result of that as before.Test plan
Unit test added for a URL-encoded JSON response.
Changelog
p4brokeris used, provided that the filter script gets adjusted as well.