Points log visibilty restriction rule API #536
Comments
Capabilities API and non-logged-in usersUsing |
|
Hmmm, I believed that This may explain why some people have complained before about only the periodic points logs being displayed to logged-out users, since all of the post points logs have capability checks associated with them (which will always fail for logged-out users). (Note that I was never able to reproduce this behavior, but I don't know how hard I actually tried.) So this reveals (what looks to me to be) a flaw in WordPress's capability system: there appears to be know way to determine whether a logged out/unregistered user is permitted to perform an action. At least not that I know of. Investigation of this will be required. |
|
Scouring trac, I've only come up with two related tickets: WP#37405 and WP#19373. Neither of these addresses the fact that as an implication, technically logged out users do not have the capability to read posts, nor how to check what permissions/capabilities logged out/unregistered users ("guests" or "visitors") have. |
|
After more thought about all of this, I've created WP#38276. What to do for the time being, however, is another issue. I'm thinking that at the moment our best way forward is to:
Actually, we should probably have a higher-level filter somewhere (or possibly we'll integrate with those that we already have), and in that case the specific filter here may not be needed. |
Logs for posts that don't exist anymoreI'm working on converting the current checks over to the (mock-up of the) new API. I noticed that currently in |
Checking if rules applyRules can be generic things, and I'm currently planning to have them be registered on a per-log-type basis. However, if we just check if any rules exist for that log type to determine whether to mark a particular log as restricted, all logs of that type will be marked as restricted. Which isn't necessarily the case. Using posts as an example, most posts can be viewed by anyone, there would only be a few that wouldn't be. And I wasn't planning to mark every single post-related log as restricted viewing. Because not only would that make most of the logs restricted viewing and turn us into the boy who cried wolf, it would actually not be true for all of those logs. Most of the individual logs in question wouldn't be restricted at all. So we need to be able to check whether a rule applies to a particular log. And this is separate from checking whether the user can view a log. I had this thing going around i my head that we really only need to check if the user can view the log, and if they can't then we know that the rule applies. Which is true, but also irrelevant, since in that case we won't be displaying that log at all. We need to know whether the rule could apply to a post when the user can view the log, not when they can't. (Giving some kind of indication that some logs are or may be hidden is an interesting idea, but it is separate from this ticket and #424 I think.) |
|
Because of duplication of logic, and thus potentially database operations, between the capability checking and applicability checking methods, I think perhaps each object should be constructed with a log object. Then anything that has to be pulled from the database or results from logic that has to be computed both places can be saved in an object property. |
|
In reference to what we said in #424 (comment), we can tell the user who can view the log, just not who can view the post. (For example.) Which is OK. |
AppSince we're talking about a class registry here, that means that we should probably use the apps API as well, to make it easy to retrieve, initialize, etc., the registry. However, it kind of seems funny to have a But maybe those fears are entirely unfounded. After all, the very existence of particular sub-apps on this app doesn't prevent us from adding any sort of methods or properties, etc., to the app itself in the future. The main question would be whether we would want to follow a different app/sub-app naming convention in the future, that's all. Though really, that could probably be fixed by aliasing. Anyway, that brings us to the bigger issue, which is that if we add this as a top-level app we'll likely want to make it a sub-app of the points app in the future instead. And I think that would be more difficult to set up aliasing for (since it would involve the main app, not just the points app). So, let's go ahead and pursue the points app. But maybe that should be in a separate ticket? |
|
I'm thinking that it makes sense to make a logs app that is a child of the points app, since there will likely be other log related stuff in the future, and having it all in a single app makes it easier for it to all be interacted with though that one app object. API nameI'm just unsure whether to call this the "viewing restrictions" API or just the "restrictions" API. The former is more descriptive, but it is kind of long. Or might we have other kinds of restrictions for logs in the future? Not of this type I don't think. Editing logs isn't currently a feature, and this kind of granular restrictions probably wouldn't be necessary. Anyway, even if they were that is so far out that I don't know that we can really gauge how we'd need the API to work anyway. As it stands, the So yeah, for now let's go with "viewing restrictions", I guess. |
Broader Logs APIMaybe we are approaching this wrong though. Maybe each log type should have an object registered for it, and this should be just one of the features. That would make it possible to do other things like filtering, etc., in the future, by giving each log type a human-readable name (which currently they don't have). However, log types can be added dynamically (think the events API, particularly the post type events). But then, they can be registered dynamically as well, I guess. So the log type would:
And possibly:
Really though, this could easily turn into a rabbit trail where we rewrite the whole points API. I think that needs to happen, but how far down the path we go when we aren't ready to go the whole way is the question. I think that for right now we do need to keep it to a minimum, even though stuff like this is the sooner the better. Because otherwise we'll likely end up with a logs app as a whole that doesn't really make sense inside of a complete points API rewrite. So no logs API for now. But maybe up the priority on the hooks API rewrite to something to begin pursuing as part of 2.3. |
Old ActionsThe function has two actions:
The former, I think that we should deprecate completely. We already have special handling for it because it expects the user to always be the current user. It really won't have any use because any restriction which could use it could also use the new API. The main question is what to do about the other, generic action. Should we also deprecate it? Or would it be useful for more generic restrictions? Actually, I was initially thinking that we would let generic restrictions be registered under the After all, it may be easiest for us to just stick with a generic restriction for handling the logs from the new hooks API, as we do now. So let's deprecate both of these filters. |
|
Speaking of deprecation, we also need to consider how to best handle deprecating the old points hooks functions that were hooked to these actions. The issue is that we need to provide back-compat in the post type parent hook class, which automatically hooks these methods up. We need to continue hooking the methods up for non-core classes that extend from this and aren't using the logs restriction API yet. I was going to suggest that we just check if the method exists, but we need to keep it around for back-compat. I suppose that really the only way to achieve what we need is with some kind of a flag property or something like that. |
Entities APIOne issue with converting the hooks API related code is that there is a filter in the And I guess that really, even checking the interfaces doesn't tell us whether the particular entity in question is restricted. So maybe what we really need here is a more full-fleshed entity visibility restrictions API. Basically that would mean introducing a more generic restrictions API just like this one, and likely deprecating that filter just like we have done these. I guess we'd need to pursue that in another ticket though. |
|
Note that we don't have to have the entities tell us why a particular entity of that type is restricted (and as previously discussed, in many cases they won't be able to). All we really need to know is if the restriction applies to a particular entity of that type. So we don't actually need a full-fledged API exactly like this one, all we need is the ability to check whether an given entity of that type is restricted. |
|
In #541 (comment) we had some interesting thoughts:
In other words, we might want to make it clear that this API is about the visibility of points log text, not the log as a whole. Restricting the log as a whole would be for a separate API, though perhaps we should introduce it as part of this current work (possibly its own ticket though). |
User-related restrictions?What about user-related restrictions? These aren't applied on a per-entity basis, but a purely per-user basis. I guess that's irrelevant to logs, because they aren't on a per-user basis, but a per-log basis. But per-user can be achieved through the generic restrictions. Might apply to the entities API though. But the reason for the question I guess is that we aren't constructing the restriction with the user ID but rather passing that in to I'm not sure how all of that meshes with the entity API though, because right now we are constructing with the user there. Which makes some sense because we're not generally going to be checking against multiple users at one, so we might as well go ahead and pass the user ID in to the constructor. But it makes it too easy to base application checks on the user, perhaps. Well, fortunately we aren't using the user ID in any of the entity restrictions' application checking code. However, we do have an issue (and this is why I brought this up), in that in the logs restrictions I am not passing the user ID in to the constructor, and yet a lot of checking is happening in the constructor. For the hooks restriction code this leads to an issue, as we're trying to construct the entity restrictions in our constructor but we don't have the user ID to pass in. So let's omit the user ID from the constructor in the entity restrictions API as well. That way it will be impossible to accidentally have the user be taken into account in |
Describing multi-part restrictionsNormally we want each restriction rule to have a simple description, like:
But sometimes we have multiple rules based on a single restriction. For example, our generic restriction for the events API, there might be several different args that relate to a single log and as a result there could be multiple restrictions, relating to a single log from that restriction, one for each arg. The question then, is how best to handle this. Rather than creating a complex description internally, I think that we should allow the restriction description method to return an array, not just a single string. Generally speaking though, I think that feature should only be used for generic restrictions, restrictions that can be split should be. Well, I guess in theory we could actually split this restriction if we wanted to, and register a separate restriction for each arg for each event. The downside to that though is that it would take more memory to store all of those restriction rules in the registry, and that it would take more work to check multiple restrictions for a single log type for logs that have many args. Though that latter probably wouldn't really make much difference, perhaps. So all told, I think it is nice to give ourselves this option, though it does need to be used with care. |
Positive overridesOne thing to note: the old filters would allow a positive override, that is converting a user not being able to view into a user being able to view. Generally though, that is a bad idea, because there is currently now way of knowing why a user isn't able to view, that is, which restrictions specifically applied. We could introduce such a feature in the future, however. |
Check inside query?Maybe the restriction checks should actually take place at the query level. Although that would preclude the use of placeholders rather than hiding the whole log entry and not just the text. Although even then we could still provide an option to the have the query return it either way, with placeholders or with logs removed. This is the way the The problem with doing this here though would be that we'd have to check the application of the restrictions all over again in order to display the message that not all users can see a given log. So I guess we can't do that. We could save the restriction objects and allow them to be retrieved from the query, of course, but that uses up additional memory. Although we could just save the descriptions, and discard the objects, I suppose. I'm still not sure it is the best fit for this though. Perhaps the best thing to do is a hybrid approach, where we basically convert our logs displaying function into a query wrapper that takes care of all of this stuff, but still allows the templating methods to be overridden. (Edit: thus, #544.) |
Description formatOne thing that we have to figure out is the format that the descriptions should take. This is more difficult because it is possible for multiple restrictions to apply to a single log, in which case we'll want to display all of the descriptions. I had been planning to some how have each of the individual descriptions be almost like partial sentences, and they would assume that they would be in a list preceded by an opener, like this:
However, this makes for difficult l10n. It would be better if we could just go with full sentences from the start, and do something more like this:
This is more redundant, but it would also allow us to just use the description all on its own when we didn't have more than one:
And since most of the time we'll only have one restriction per log, I think that making that work well is a priority. So I think this is what we should pursue, having each description be full. |
|
We could still standardize on the exact format though. Right now I have basically been following this format:
It could possibly be stated even more generally:
Since possibly in some cases we'd want to say "have the ability to {do this}" or something, instead of "can {do this}". Anyway, I think that in general this is good, though I do wonder about the ending part. We could possibly leave of the "log" entirely:
But maybe that isn't descriptive enough. However, the term "log" is not ideal because it can refer to the entire log or to that particular log entry (which is the case here). Of course, this should be fairly obvious based on the context in which it is displayed, but maybe it would make sense to go with:
I guess really there's no reason not to do that, it isn't so much more verbose, just one work (in English at least, and translators can decide whether it is necessary to differentiate in their own language by adding extra words or whether there are separate words that are unambiguous). So let's go with that. |
Restricted log textAbove we said:
After more thought about this, I don't think that it is really feasible for us to only restrict the log text. We have been thinking that possibly (probably?) we'd switch to showing the log but hiding the text in the future. However, this would lead to clever users being able to reconstruct the log text of hidden logs using the search feature. The way that WordPress handles this with password protected posts (a similar case) is to omit them from searches. We could do that, but not as cleanly as |
Extracts the code for tracking the switched state on multisite out of the un/installer class and into its own class that can be used elsewhere in the code. We will use it with the points logs momentarily. This allows us to switch without ever having to restore until we are done, saving us from switching back all of the time. Currently the class doesn’t support saving multiple snapshots, just one. If we need that feature in the future, and proper snapshotting hasn’t been introduced in core, we can add it here. In the process of running the unit tests for the new class I realized that creating the sites adds a bit of extra time to each test, since the tables for each site have to be created. WordPress core has been moving toward sharing some fixtures like this between all of the tests in a testcase. I decided to do the same, although I created our own method of doing so that will also automatically clean up after ourselves in the parent testcase, all that the child testcase has to do is ask for the fixtures to be created, and the parent testcase will take care of everything else. Currently no special attributes to give the fixtures can be specified, but we’ll expand on this in the future. In order to gain in cleaning up afterward, I’ve introduced a new interface that our factories can implement so that they can provide a delete method in addition to the create an update ones. We don’t implement it anywhere yet, but we’ll do that in the future when we need to actually use it. For the core object types, we’ll have to add methods to the parent testcase instead. For some of these that we already have in child classes, I’ve pulled them up. Note that shared fixtures have to be used with care, and generally only in tests where the fixtures are only read, not modified in any way. See #536, #424
SlugsDo we need to pass in the slugs? The log type isn't really necessary, because we can always determine that from the log itself. But what about the slug of the particular restriction for that log type? Is that necessary? Currently we don't even expose the restrictions in the API, we just return a wrapper (in our current mock-up), much like the entities API. With the entities API it was decided to just use a registry that didn't pass the slugs to the classes on construction. And I guess it makes sense to do the same here. |
|
I'm thinking that maybe this would be a better format for the descriptions:
This is a bit simpler, I think, and it places the specific conditions of the restriction at the end which I think will make them easier to pick out than if they were stuck in the middle. |
SwitchingShould we move the switching into the restrictions API? I suppose that since we provide the view pattern now, it isn't really necessary for us to check here instead of there. We are able to optimize the checks within the view, so in the interest of performance let's just leave it at that for now. Or maybe the better thing to do would be to support a flag that disables the internal context handling, and set that in our view. That way we don't run the risk of developers using the API outside of the views and forgetting that they need to do their own context checking. |
Legacy handlingWe have decided to deprecate the old filters, but we still need to apply them for legacy reasons. What we did in the entity restriction API is to move the filter being deprecated there into a legacy restriction handler. But we can't do that here, because we can't check when any functions hooked to the filter have logic that actually applies to this particular log. We didn't have to worry about that with the entity restrictions filter because it was new and likely unused anyway. But if we just check if there are any functions at all hooked to the filter in But if we were to only check whether any functions were hooked to the type-specific filter, this would mean that the generic filter would only run when there were functions hooked to the specific filter. The reason being that the filters would have to be run in So it really doesn't work to have these filters be moved into a restriction object. However, they also can't just sit within the function that they currently reside in, because we no longer use it in the points logs display code. We will likely be keeping it around for the future, but it only allows us to check if the user can, whereas we have optimized to also be able to easily check if the restriction applies in our new points log views so that we can show the descriptions for the applicable filters. Which means that we still need to call these filters within that function, but we also need to be able to call them within the points logs view as well. We can't just integrate them into the So it would seem that what we'd need to do is introduce a new function to house these filters, that we can call in the old function and also in the points logs view. I think that it would make sense to just make this a method on the points logs viewing restrictions app, since we'll already be using that in both places anyway. The main question remaining then is whether the result of the restriction API's |
|
One more thing in regard to legacy handling, and that is context switching. Because the legacy handling will have to be entirely separate from the rest of the restrictions API, it will have to have separate switching, too, I guess. Although I think for the sake of optimization withing our points logs views, we should allow this to be disabled just as we have done for the rest of the API. That would still mean that there would be excessive switching in the old can view function though, because it would happen once in |
Disabling switchingAfter further thought, I've decided not to allow switching to be disabled, as it really doesn't accomplish anything. There is almost no overhead in checking if we need to switch or not, all we do is call |
|
We do have to do one other thing though, and that is to check if we are on multisite, because if we aren't the logs will be saved with the |
Points HooksThe old points hooks objects had methods that were hooked up to the deprecated filters. We'll need to deprecate them, but we have to be conscious that they are still needed for any extending classes outside of core. I was planning to update these methods to use the new API when appropriate, but looking back at that I find that I wasn't using the So maybe the best thing is just to go ahead and get all of the restrictions that apply to that log type and throw them into a wrapper. But then we'd still need to worry about switching sites and stuff. Though I guess we really ought to add site switching there either way. However, there is another issue, and that is that even if we get all of the restrictions for that log type, this will be leaving out generic restrictions that might also apply to that log type. I guess this was the previous behavior anyway though. All things considered, I think that we should just leave these functions as they are, but deprecate them. It is too much work to update them to use the new API correctly, and they really shouldn't be called directly anyway, after 2.1.0. Or, I guess there's no reason for us not to just go ahead and hard-code the use of the restriction classes into this directly, so that we are using the improved logic for the checks, but aren't duplicating it here. So let's do that. |
This is needed for the points logs viewing restrictions API. See #536
Introduces an API for restricting points logs from being viewed by certain users. The app is a children class registry, where the parent slugs are the log types. Generic restrictions that apply to all types of logs can also be registered with `'all'` as the parent slug. This registry constructs the restrictions without the slugs, since the log type can be determined from the log itself, which is passed to the restrictions' constructors, and the slug of the particular class for that log type is not needed. The restrictions are not mean to be retrieved directly, but via the `get_restrictions()` method, which will automatically include the generic restrictions along with any restrictions that apply specifically to that log type. The value of the new API is that it allows one to not only determine whether a particular user can view a log, but whether there are any restrictions that apply to that particular log at all, and if so, to get a description of each of them. The legacy function and the filters are retained, but the filters are deprecated. The old points hooks will be updated to use the new API shortly. The points logs view is updated to use the new API directly, so that it will also be able to get the description of the applicable restrictions, which it will use shortly. The fixture handling in the testcase has been updated extensively, with the additional features documented inline. This has also resulted in some updates within the points logs factory. See #536, #424
We've just introduced the points logs viewing restrictions API, and now we update the old points hooks to use that API. See #536
The introduction of the points logs restrictions API involved several modifications of the PHPUnit tests, and there were a few remaining issues that we didn't spot. First, there were some errors being caused by us assuming that `$extra_fixture_ids` would always being an array, even though we set it to `null` after the fixtures are destroyed. I've decided to just reset these values to empty arrays, instead of `null`, so we won't have these kind of issues. We also realized that after one testcase had run our fixtures were no longer being saved when we created them. This is because the parent `setUp()` method sets `autocommit` to `0` when it starts the transactions, meaning anything that isn't explicitly committed will be discarded when a new transaction is created. This didn't reveal itself as a problem for our fixtures when we were running a single testcase, because we call `parent::setUp()` *after* we create the fixtures, so that when it first runs no transactions have been started and autocommitting hasn't yet been disabled. But after one testcase has run, autocomitting has then been disabled, and so it is necessary for us to explicitly commit our fixtures or else they will be discarded when we call `parent::setUp()` and a new transaction is started. We also discovered that there were some errors in tests that were expecting a points type to exist, as created by `$this->create_points_type()`. It turned out that that method wasn't creating a points type after we'd created some by our fixture code (as we do for points logs). This was because when we delete the points type created with the fixtures, the option in the database will remain as an empty array. And because we create the points type using `add_option()`, this would cause it not to be created since the option would already exist. I've just updated the code to use `update_option()` instead, which should cause no issues, since there is little fear of overwriting anything since anywhere this is being called it is expected to be creating something where nothing exists anyway, and wouldn't be working if the option did. Finally, there were a few race-condition issues in the points logs view tests, and update them to expect the correct descending ordering. See #536, #544
The points logs viewing restrictions for the points hooks was only passing in the ID of the entities, but greater precision can be obtained by passing in the GUID of the entity instead. To this end, we update the Points reactor to save the GUID for each entity, as well as the ID. Then in the points logs vewing restriction for the hooks API we utilize the GUIDs when they are available, but fall back to just the ID for back-compat. The tests for the hooks restriction is also updated to use more fixtures. This prompted us to fix a bug in the base testcase's fixture handling, that was causing an error about an array being provided where a string was expected, because the points logs meta arg is an array of values. See #424, #536, #535, #479
|
Closing notes:
|
|
I've added documentation here: http://wordpoints.org/developer-guide/points-logs-viewing-restrictions-api/ |
In #535 it became possible to retrieve entities from a different context than the entity actually exists in, by passing in the full GUID of the entity. Switching to the proper context to retrieve the entity was now handled internally. However, when retrieving entity children, like a related entity, the current context was still always assumed to be correct. This commit fixes that in regard to relationships, by: - Ensuring that the correct context is switched to when retrieving the related entity IDs, and - Passing in the full GUIDs to the related entity object when its value is set, rather than just the IDs, so that it can also switch to the correct context to retrieve the entity object (as #535 made possible). The base implementation here only covers the most common form of relationship, where the related entity comes from a context the same as or above the context of the primary entity. For relationships where the related entity comes from a lower or different context, special handling would be needed to fully support out-of-context retrieval of the related entity. See #536
In #535 it became possible to retrieve entities from a different context than the entity actually exists in, by passing in the full GUID of the entity. Switching to the proper context to retrieve the entity was now handled internally. However, when retrieving entity children, like an entity attribute, the current context was still always assumed to be correct. This commit fixes that in regard to attributes, by ensuring that the correct context is switched to when retrieving the attribute value. The base implementation here only covers the most common form of attribute, where the attribute value comes from the same context as the entity. For attributes where the value comes from a different context, special handling would be needed to fully support out-of-context retrieval of the attribute value. Though this situation is unlikely to exist in practice. See #536
Conceived in #424 (comment):
As also noted there, it is possible for multiple rules to apply to a single log. There was also a question as to whether the nature of the WordPress caps API would permit us to build such an API, but it was decided that it would indeed.
The text was updated successfully, but these errors were encountered: