New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Protocol Change] Allow specified users/groups to replace domain content from within Interface #11056

Merged
merged 27 commits into from Aug 16, 2017

Conversation

Projects
None yet
9 participants
@misslivirose
Contributor

misslivirose commented Jul 26, 2017

This is a first pass PR that enables specific users to replace domain content sets (models.json.gz of an entity server) directly from Interface.

  • Adds a new permission level to the domain server, 'Replace Content', which specifies which nodes are allowed to replace entire domain content sets by default. Currently, this defaults to false for all users and groups.

  • Add a new function to OctreeServer to handle being passed a URL and download a models.json.gz file that is not on the file system & does not come from the domain server.

  • Enable Interface to detect a .json.gz file as an allowed file type, and attempt to replace domain content. This will have an additional permission layer on it to only work for urls that are from Marketplace per spec.

  • A new sourced packet type to handle Octree Replacement from a URL

  • Addition of a DomainManagement API, which enables components in Interface to check on permissions related to the domain settings.

@misslivirose

This comment has been minimized.

Contributor

misslivirose commented Jul 26, 2017

Testing Notes

Install both Sandbox and Interface from this PR. Do not copy settings over from production install.

DomainManagement API Testing

  • Test 1: Confirm that by default, no users or groups have the ability to Replace Content, as specified in the domain server settings (localhost:40100/settings/)
  • Test 2: In Interface, open up the console and type: DomainManagement.canReplaceDomainContent(). Ensure that this returns false. Type: DomainManagement.canReplaceDomainContentChanged.connect(function(){print("Changed Ability to Replace Domain Content")}); This will return undefined, which is normal.
  • Test 3: Enable localhost to Replace Content in the Domain and restart the server. Confirm that the console window detects and prints that the ability to replace domain content has been changed.
  • Test 4: Repeat of test 2, but confirm that this is now returning true

Feature Testing - Stage 1
This test will wipe out domain content sets. You shouldn't be able to do this if you're not running the Sandbox for this PR, but take caution with it and keep that in mind when testing. Back up things... which you should do even when you're not testing powerful content replacement features.

Test Models.json.gz link: https://hifi-content.s3.amazonaws.com/liv/dev/models.json.gz
Test Blank.json.gz link: https://hifi-content.s3.amazonaws.com/liv/dev/blank.json.gz

  • Test 1: With permissions to Replace Content, open up the private browser (CTRL+B) and enter the test Models.json.gz file. Confirm that you see the warning text. Click cancel. Confirm that your content set does not load.
  • Test 2: Repeat test 1, but this time, confirm that you want to replace your content set. Confirm that you move to /0,0,0 and that the content loads in. You should have 1 Light, 1 Zone, and 6 models in the Entity List.
  • Test 3: Download Blank.json.gz and save it you your local file system. Drag it into Interface (do not unzip first). Confirm that you see the confirmation. Click cancel. Confirm that you do not wipe out the content set.
  • Test 4: Repeat Test 3 and confirm. Ensure that all models are cleared out from your domain.
  • Test 5: Change your permissions back in the Domain Settings so that you can no longer replace domain content sets. Repeat Feature Testing - Test 1 and confirm that you get a permission denied message, and that the content set is not replaced.

Feature Notes & Additional Details
Final warning text is TBD.

Additional testing notes to come to ensure that permissions work properly in other domains with this build.

The product version of this feature will require that the models.json.gz files that are used with domain content replacement sets are exclusively enabled from Marketplace. However, in order to test the current PR functionality in isolation before adding new content types to Marketplace, this requirement is not yet in place.

Do not merge - requires changes to restrict URLS to Marketplace CDN; contains protocol change (new packet header type)

@@ -160,9 +160,7 @@ DomainServer::DomainServer(int argc, char* argv[]) :
getTemporaryName();
}
_gatekeeper.preloadAllowedUserPublicKeys(); // so they can connect on first request

This comment has been minimized.

@ZappoMan

ZappoMan Jul 26, 2017

Contributor

accidental delete - or bad merge?

This comment has been minimized.

@misslivirose

misslivirose Jul 26, 2017

Contributor

I think it was a bad merge - looks to have been deleted in: 2d0c5ff

@misslivirose

This comment has been minimized.

Contributor

misslivirose commented Jul 27, 2017

build this please

@highfidelity highfidelity deleted a comment from hifi-gustavo Jul 27, 2017

@marko8904

This comment has been minimized.

Contributor

marko8904 commented Jul 27, 2017

build this please

@OmegaHeron

This comment has been minimized.

Contributor

OmegaHeron commented Jul 31, 2017

QA In progress.

@OmegaHeron

This comment has been minimized.

Contributor

OmegaHeron commented Jul 31, 2017

Windows 10
No decision on QA status until feedback received.

DS Tests

Test 1 - No permissions for replace content is default. Pass
Test 2 - Returns False. Pass
Test 3 - Console emits message on state change as expected. Pass
Test 4 - Repeat test 2, should return true. Pass

One issue - likely not a fail, but, possibly should be investigated.

Given scenario of;
User connects to domain not logged in.
User logs in.
User as logged in has replace permission (but no other form of permission -- i.e. via group, IP, standard permissions or any other).
User will not have replace permission until restarting interface or leaving/returning to domain.

Feature testing.

Test 1 - Pass
Test 2 - Pass -- Moved to 0,0,0. Have 1 light, 6 models and 1 zone.

One really odd thing -- I had began with standard sandbox content and after replacing its content in Test 2 the "wind chimes" from garden location (in standard default sandbox install) were still playing at 0,0,0. Until I restarted DS sounds continued even after restarting Interface. This points to something not being cleared when Test 2 replaces existing content.

Test 3 - Pass
Test 4 - Pass
Test 5 - Pass

Overall this works as expected. I'm reluctant to give a full pass though due to "wind chimes" issue noted in Feature Testing matrix, second test.

@YuppyPlays

This comment has been minimized.

YuppyPlays commented Aug 11, 2017

Testing complete:

  1. When you install the PR build, check the domain server settings page and ensure that the column "Replace Content" is present and checked for localhost Pass
  2. Confirm that if you drag one of the models.json.gz files (linked in the original test notes) into interface that nothing happens Pass

@birarda birarda self-assigned this Aug 15, 2017

}
});
} else {
qDebug() << "Cannot perform octree file replacement since current persist file path is not yet known";

This comment has been minimized.

@birarda

birarda Aug 15, 2017

Member

There's too much repetition with the method above handleOctreeFileReplacement. I'd like to see both call a helper once they have the file contents to complete the process.

bool Application::askToReplaceDomainContent(const QString& url) {
QString methodDetails;
if (DependencyManager::get<NodeList>()->getThisNodeCanReplaceContent()) {
QUrl originURL{ url };

This comment has been minimized.

@birarda

birarda Aug 15, 2017

Member

space after originURL

if (agreeToReplaceContent) {
// Given confirmation, send request to domain server to replace content
qCDebug(interfaceapp) << "Attempting to replace domain content: " << url;
QByteArray _url(url.toUtf8());

This comment has been minimized.

@birarda

birarda Aug 15, 2017

Member

I think this should have a different name that _url since it's a byte array and not a URL. Also, it should not be prepended by an underscore since it is not a member variable.

auto octreeFilePacket = NLPacket::create(PacketType::OctreeFileReplacementFromUrl, _url.size(), true);
octreeFilePacket->write(_url);
limitedNodeList->sendPacket(std::move(octreeFilePacket), *octreeNode);
return true;

This comment has been minimized.

@birarda

birarda Aug 15, 2017

Member

I don't think this return true is needed?

{ "content_set_url", url }
};
UserActivityLogger::getInstance().logAction("replace_domain_content", messageProperties);
return false;

This comment has been minimized.

@birarda

birarda Aug 15, 2017

Member

In what case should this method return true? As far as I can tell it's always returning false right now.

#include "OffscreenUi.h"
DomainManagementScriptingInterface::DomainManagementScriptingInterface()
{

This comment has been minimized.

@birarda

birarda Aug 15, 2017

Member

this should be on the previous line

void canReplaceDomainContentChanged(bool canReplaceDomainContent);
};
#endif //hifi_DomainManagementScriptingInterface_h

This comment has been minimized.

@birarda

birarda Aug 15, 2017

Member

Is DomainManagementScriptingInterface used for anything? Until we're sure we need canReplaceDomainContentChanged in QML/JS I don't think we should add this.

This comment has been minimized.

@misslivirose

misslivirose Aug 15, 2017

Contributor

I've removed this - there was talk of changing the visibility of content based on permissions but I think that will be excluded for the time being.

class DomainManagementScriptingInterface : public QObject, public Dependency {
Q_OBJECT

This comment has been minimized.

@birarda

birarda Aug 15, 2017

Member

this should be tabbed over

DomainManagementScriptingInterface::~DomainManagementScriptingInterface() {
auto nodeList = DependencyManager::get<NodeList>();
disconnect(nodeList.data(), &NodeList::canReplaceContentChanged, this, &DomainManagementScriptingInterface::canReplaceDomainContentChanged);

This comment has been minimized.

@birarda

birarda Aug 15, 2017

Member

You can remove the destructor altogether since this disconnect will happen automatically for you because your object is being destructed.

@misslivirose

This comment has been minimized.

Contributor

misslivirose commented Aug 15, 2017

Final QA Test Steps:

  1. Install sandbox and interface from this PR
  2. Go to the domain settings and ensure that localhost has permission to replace domain content
  3. Open up the in-world Browser with CTRL+B while in Sandbox
  4. Paste in the following url: http://mpassets.highfidelity.com/ee5f41ec-5f3f-47b6-a371-ad4eed53ffe8-v1/club_Aug11.json.gz
  5. Check that you are prompted to replace domain content and click OK (in HMD mode, this is in the tablet)
  6. Confirm that content is replaced and you are teleported to /0,-10,0

Additional tests:
A) Confirm that if you disable the permissions for localhost in Sandbox that you are not able to replace content
B) Confirm that https://hifi-content.s3.amazonaws.com/liv/dev/models.json.gz pasted into the private browser does not work
C) Confirm that if you have permission to replace content, but say no that the content is not replaced

@birarda

Couple extra comments which are not blockers

QNetworkReply::NetworkError networkError = reply->error();
if (networkError == QNetworkReply::NoError) {
QByteArray contents = reply->readAll();
OctreeServer::replaceContentFromMessageData(contents);

This comment has been minimized.

@birarda

birarda Aug 15, 2017

Member

I'm not sure you need the OctreeServer:: part here?

This comment has been minimized.

@misslivirose

misslivirose Aug 16, 2017

Contributor

removed, thanks for pointing that out

@@ -935,45 +935,75 @@ void OctreeServer::handleOctreeFileReplacement(QSharedPointer<ReceivedMessage> m
// so here we just store a special file at our persist path
// and then force a stop of the server so that it can pick it up when it relaunches
if (!_persistAbsoluteFilePath.isEmpty()) {
OctreeServer::replaceContentFromMessageData(message->getMessage());

This comment has been minimized.

@birarda

birarda Aug 15, 2017

Member

I'm not sure you need the OctreeServer:: part here?

@YuppyPlays

This comment has been minimized.

YuppyPlays commented Aug 15, 2017

Testing.

@YuppyPlays

This comment has been minimized.

YuppyPlays commented Aug 15, 2017

Testing complete:

Final QA Test Steps:

Install sandbox and interface from this PR Check
Go to the domain settings and ensure that localhost has permission to replace domain content Check
Open up the in-world Browser with CTRL+B while in Sandbox Check
Paste in the following url: http://mpassets.highfidelity.com/ee5f41ec-5f3f-47b6-a371-ad4eed53ffe8-v1/club_Aug11.json.gz Check
Check that you are prompted to replace domain content and click OK (in HMD mode, this is in the tablet) Check
Confirm that content is replaced and you are teleported to /0,-10,0 Check

Additional tests:
A) Confirm that if you disable the permissions for localhost in Sandbox that you are not able to replace content Check
B) Confirm that https://hifi-content.s3.amazonaws.com/liv/dev/models.json.gz pasted into the private browser does not work Check, though this does not work regardless of the domain settings being enabled/disabled for localhost replacing content. Tested with other link in test plan and it says "you do not have permissions to replace domain content." So I believe that's a pass there if that's the main criteria.
C) Confirm that if you have permission to replace content, but say no that the content is not replaced Check, but again, used other URL in test plan, not the one from Additional Tests B.

I believe this is a Pass but please do let me know if my findings would negate this.

@RyanDowne

This comment has been minimized.

Member

RyanDowne commented Aug 16, 2017

@birarda is this dev approved?

@birarda

This comment has been minimized.

Member

birarda commented Aug 16, 2017

Can't add tag on mobile - approved.

@misslivirose misslivirose merged commit 36ff82d into highfidelity:master Aug 16, 2017

1 check passed

default Build finished.
Details
@misslivirose

This comment has been minimized.

Contributor

misslivirose commented Aug 16, 2017

@conklin94122 - additional note for when this makes it into an RC: test that group permissions persist across an upgrade.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment