Refs #6760: Validator to ensure associations are authorized.

[ehelms]

My goal was to create the validator and an example model as a starting place prior to attempting full coverage.

Please see for a test scenario - http://projects.theforeman.org/issues/6760
Also, a corresponding beginning usage in Katello - Katello/katello#4483

The idea is to define what resources a model is associated to that should be checked upon association and the permission that corresponds. There is the possibility that this may need to be evolved for derived permissions (e.g. in Katello repositories derive their permissions from Products).

[ehelms]

@ares Please review.

[domcleal]

This is pretty much the same as http://projects.theforeman.org/issues/4477, but the edit case would need to continue working.

[ehelms]

@domcleal Not following what you mean exactly by the edit case? Do you have an opinion on this proposed change in approach? Katello/katello#4483 (comment)

[isratrade]

@ehelms, nitpik, I would use Location.scoped instead of Location.all since .all returns an Array where .scoped returns ActiveRecord::Relation that can be chained where an Array cannot be chained.

[ehelms]

I could also use some design help with respect to this implementation as right now this permits by default. The options would be:

1. Permit by default: explicit rules and expressions of the associations would need to be declared
2. Deny by default (implied): Automatically detect associations and assume 'view_' permission is required but allow overrides to be specified on a per model basis via some function that for example returns a hash of association - rule mappings
3. Deny by default (explicit): Raise warning or error if association rule is not defined and require that a rule must be defined for an association

[isratrade]

@ehelms, I think the idea of this PR is similar to detecting "mismatches" between a host's attributes and the associations defined in the taxable_taxonomies table. Also, when we mass update organization/location for hosts, there's the option to "fix on mismatch" or "fail on mismatch".

In which cases are you referring to regarding "permit by default" or "deny by default"? Is it only on host creation? or other objects? Host is the only object that is explicitly assign an organization or location. For all other objects, there are currently no restrictions in the UI (filters in dropdown fields) that are based on which locations/organizations are selected.

[isratrade]

correction for those reading my comment above which is based on an a "similar but different" branch that @ehelms is working on that is not yet a PR.

[domcleal]

@ehelms I mean that if user A creates a host that's associated to a domain I can't view, I'd expect to still be able to edit the host without a validation failure on that attribute, with the attribute remaining at the current value or set to another value that I can view.

We would need to update forms to ensure they show only visible resources. Today, as per http://projects.theforeman.org/issues/4477, they show everything (except for taxonomies, which always apply). @witlessbird has been come up against this problem.

I think this is the right direction, I like it.

[domcleal]

@ehelms I think (2), using an implicit rule and defining exceptions will be best.

[ehelms]

@domcleal @ares This is ready for review. I have switched it to a deny by default strategy. For checking only updated values, I think we will require #1479 in order to do so. Is there anything we or I can do to help push that PR along to then incorporate into my own?

[ehelms]

[test]

[dmitri-d]

this breaks for me on polymorphic associations, looking into it...

[dmitri-d]

This swallows exceptions and as a result grants access to associations that shouldn't be granted.

[ehelms]

The ones I saw it swallowing didn't appear to be true associations. For example, all of the audited associations that get interjected but don't appear to be true AR models. What were you seeing get swallowed?

[dmitri-d]

Anything with polymorphic associations: owners and audits, are among other things.

[dmitri-d]

I'm also running into issues with the "audited" gem. I think that at the very least this needs more tests with actual associations (not test-doubles), including stuff like has_many through and has_one associations, associations with non-default classes, etc.

[dmitri-d]

Here's what I used to (partially) resolve polymorphic associations issues:

  def check_authorization?(record, association, exemptions)
    klass = association.options.include?(:polymorphic) ? record.send(association.foreign_type) : association.klass
    !exemptions[association.name] &&
      klass &&
      klass.respond_to?(:authorized) &&
      association.name != 'taxonomies'
  end

[ehelms]

I contemplated attempting to add tests using "real" models and their associations. Call me tainted by old Katello tests, but I didn't want to create a suite of tests that were tied to a particular model such that if that model changes somehow has to deal with figuring out the tangle of these tests that attempted to abuse the model. Granted, there is no good answer either way, but I also debated trying to derive a test that would, similar the orgs/locs test, check every association for all models with this validator include. However, I did not see an easy way to do this.

[dmitri-d]

I'm concerned that the current tests aren't sufficient, as they don't cover a lot of ground/corner cases. You can use test-doubles to verify the behaviour, but I find that association internal behavour sometimes changes quite a bit depending on the options used, for example has_many vs. has_many :through (and I haven't looked at AR 4.0 yet). I think it would be safer if tests used real models. If you don't want to rely on Foreman models, you could extract this into a gem and have a dedicated to tests schema.

[dLobatog]

@witlessbird @ehelms What's the current status of this PR? It's quite important in my opinion and people in the community are asking for this validation, are there other (better) approaches to the problem we've been looking for?

[tbrisker]

@ehelms,
Thank you very much for your efforts!
This PR has been open for a very long time with no activity.
Please indicate if you plan to continue working on it or not; if not, feel free to close it.
If you do not respond either way within a couple of weeks, I will assume this PR is abandoned and will go ahead and close it for you.