Some tests only go green because they happen to run as UID1

Hopefully this doesn't open up a whole new can of worms because we're separating the real cause (UID 1) from the symptoms

My bad...

FieldFieldTest would also break but fixing that here might be overly tedious given it's fix:

     // Bypass any field access.
-    $this->adminUser = User::create(['name' => $this->randomString()]);
+    $this->adminUser = User::create(['name' => $this->randomString(), 'roles' => [RoleInterface::ADMINISTRATOR_ID]]);
     $this->adminUser->save();

If I were to fix that here I'd have to set up an is_admin role instead and immediately add a @todo saying "Remove this again with the main patch."

RowRenderCacheText was all sorts of broken, as explained in the parent issue, so leaving that one there given its small footprint on the overall size of the parent issue's patch.

The CommentDefaultFormatterCacheTagsTest is indeed testing with user 1, causing it to go green for the wrong reasons. By assigning it the right permissions, nothing changes now because UID 1 is still a superuser, but it will once UID 1 has to behave like a regular user.

The only way to properly change this is to make the test run as UID 2, which defeats the purpose of the parent issue where we are trying to do away with code that requires a specific UID, whether that be 1, 2 or whatever. It's the same reason as comment #4: we'd be adding something here to prove it's broken, only to immediately remove it again in the parent issue. It can be done, but as I stated above it does seem a bit overly tedious when the parent issue shows the patch went red first and green after all of these fixes.

CommentAdminViewTest had its numbers changed because it was manipulating and leaking state between tests. This caused the numbers to be off. The only reason it still went green was because UID 1 can see everything all the time and so happened to match the manipulations that were being made in that test. Now that we set up all of the translations in the setUp, we need to account for those numbers in the assertions.

@vaplas, does that test also go green in combination with the parent issue's patch? I'd prefer to keep it as in #2's patch because, as it stands, it will go green all the time in this issue because we're not fixing UID 1's god mode here. The real issue with that test was that it was leaking state and UID1 was hiding that. So I do believe we need the patch from #2 because it actually stopped that test from leaking state.

If we commit your patch from #7, it might go red again in the parent issue and we'd have to fix the state leak regardless. So it's the same thing I mentioned above: we'd be changing a working patch to only have it go red in the parent issue and have to change it back to the parent version's copy.

Well I'll be damned.

I must have done something wrong because I couldn't get that test to go green unless I fixed the leaking state. Guess I can open up a separate issue to fix the rest of that test then. Cheers vaplas!

RTBC patch from #7 as far as I am concerned, but given the low amount of people involved here, it's probably be up to catch to decide. Will RTBC to put it on his radar, but am fully aware he may want us to do more work here.

Still looks good to me

@xjm: UserViewsFieldAccessTest is having assertions ripped out because those were all kinds of broken. They were covering some fields no-one should ever get access to. User one did because they get to ignore the entire access layer.

From the comment regarding that test linked in the issue summary (thanks vaplas!):

UserViewsFieldAccessTest was a doozy because it was actually trying to read out user properties such as 'status' which are explicitly forbidden by UserAccessControlHandler. So I removed all of the offending properties from that test.

I'm somewhat agreeing the way forward is to write these tests to use a random user ID other than 1. I still think that special casing should be removed again when the parent issue goes in, though, as it makes no sense for a test set up to do something weird like: "Let's by all means try to avoid this magic number".

But in order to prove the tests are in fact functioning as UID non-1, we might actually need to go through said effort and then remove it again. Even if I have reservations about the effort going into writing something that will soon be removed again, I guess there's no other way to make this issue easy to review and commit.

The whole point of UserViewsFieldAccessTest is to test for field access based on the minimum required permissions. Assigning the user the 'administer users' permission is not a valid solution as that will grant access to all properties regardless of name. See
UserAccessControlHandler::checkFieldAccess().

    // Administrative users are allowed to edit and view all fields.
    if (!in_array($field_definition->getName(), $explicit_check_fields) && $account->hasPermission('administer users')) {
      return AccessResult::allowed()->cachePerPermissions();
    }
   // ... Actual field checks only start here ...

So we can't set that permission and by not doing so, we will have failing assertions at all times because of this code:

      case 'roles':
      case 'status':
      case 'access':
      case 'login':
      case 'init':
        return AccessResult::forbidden();

Which is why I removed said assertions in the first place, because they were never functional to begin with. Only admin users would pass that test and I doubt we want to test for admins instead of regular users. Instead, we should add an admin test alongside the regular one and change the assertions to reflect the denial of access.

This properly expands UserViewsFieldAccessTest to test both with and without admin permission. It also adds some more reminders and fixes a typo.

Off topic: Hoping it goes green as my local D8 patch environment is suffering from https://github.com/hashicorp/vagrant/issues/8788#issuecomment-335260145 so I'm getting the error mentioned here: https://github.com/acquia/lightning/issues/455

But I suggest staying in the same test. This will help not go beyond the scope of the issue, and not create double check of permissions (additional role is not remove prevent permissions)

I disagree here. The whole point of testing something is to be explicit in what you're testing. Repetition isn't necessarily a bad thing if we want to prove that both users can access the basic fields but only admins can access the special ones. Playing the devil's advocate: What if one day an additional role not being able to remove permissions no longer works that way? Then we're going to be happy we were explicit about our assertions instead of implicit.

Re 1.: The patches in that issue add way more property checks than the commented ones so one could argue that removing them here actually cleans up our code until the other issue lands. But yeah, we could leave in the commented lines, sure.

Re 2.: Sure. Seems like that patch already fixes it.

Also, it seems to me more and more, that we need return FileManagedUnitTestBase back to parent issue. Because we can fix tests inside the core (like SaveTest), but what to do with external tests that can break?

You're right, this could break contrib for as long as the parent issue doesn't land to revert the UID 2 change.

I'll attach a patch that keeps in the out-of-scope reverts, but removes your UserViewsFieldAccessTest revert because of what I wrote above. I'll also move the FileManagedUnitTestBase back to the parent patch.

Interdiffing vs 22 to avoid an interdiff that reverts a revert :D

Yup, looks good.

@vaplas I think that instead of uploading an exact copy of the same patch, you can let the test-only patch fail first and then click "Add test / retest" on the one in the comment above to make the last patch go green again.

Yup, as the two comments above mentioned: We inject UID 1 to prevent $this->createUser() from accidentally creating a user 1 account. It is done in a way that is also easily reverted by the parent issue: Simply remove the line and the test runs as UID 1 again, which is fine after the parent issue lands.

Back to RTBC.

Small edit re borisson_:

I think this user is created to make sure that the next user that's created (and used in the test) is not uid1

Done, leaving at RTBC due to the nature of the changes.

missing a docblock?

It never had one to begin with and when I also fixed a docblock about "an user" vs "a user", I got told to remove it again because it was out of scope. So not touching that for now.

let's remove this - if they're to be added there - let's add them there. We don't normally include commented out code.

Same answer: Out of scope. And there is another issue which actually removes all of those commented out lines of code in one go. Removing them here might cause that issue to miss them when they re-roll their patch.

No docblock?

I could add one as it's a newly introduced method. For consistency's sake with testUserFields(), I didn't. I'd be glad to open a follow-up issue to clean that up for both methods at the same time.

There has been a lot of back-and-forthing over what is and isn't in scope for this issue and I'd like to see it move ahead. So I hope you don't mind my putting it back to RTBC so that it can either get committed or the person who's supposed to commit this can tell me exactly what minor docblock/comment changes there still need to be for it to be ready.

If that person is you, larowlan, I'll add the changes. But if you look at the previous comments you can see the patch has gone through a lot of items being removed only to be put back in or vice versa. So I'd really like to lock down the scope now and get a patch following that scope definition.

Cool, I meant no disrespect if that's how it came across. Is it still fine to move the commented out lines to the bottom of that method? Leaving them in place looks a bit weird compared to the new method.

• I've added a docblock to the new method as suggested by #36.3
• I've added a docblock to the old method as suggested by #36.1, even though that is technically out of scope here
• I've not removed the commented out lines as suggested in #36.2 because that is definitely out of scope and, as vaplas pointed out in #40, was deliberately committed that way in the past.

@xjm, @catch, @larowlan: Any objections left for this patch to go in?

This only contains the "Any user created after this is not user one" logic. Should fail all tests.

Updated the issue summary. Maybe we also need to relocate the changes to CommentViewsKernelTestBase to the original UID 1 patch, for the same reason we moved FileManagedUnitTestBase back?

We can't know for sure who will extend it and we should therefore only change it when we change how UID 1 works. Because then we will have a CR explaining that some tests might break and why they are breaking.