Skip to content
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

[RFC] Refactor policy matching, introduce high-level matching API #1757

wants to merge 13 commits into
base: master


Copy link

commented Jul 24, 2019

This implements a high-level policy matching API on top of match_policies/get_action_values that aims to simplify the most common policy matching operations.

An example:

login_mode_dict = Match.simple(g, scope=SCOPE.WEBUI, action=ACTION.LOGINMODE,
                               realm=None, user=user_object).action_values(unique=True)

The line reads as follows:

  • Retrieve all active policies with scope WEBUI and action loginmode that match the user object user_object (i.e. its username, realm, resolver, userinfo) and the current g.client_ip.
  • For these policies, extract the action values of loginmode, check for them for uniqueness, write matching policies to the audit log and return the action values dict.

Why do this? :-) Some reasons:

  • When using match_policies/get_action_values directly, we almost always pass the same set of arguments: e.g. client_ip=g.client_ip, audit_data=g.audit_object.audit_data, or active=True (for match_policies). With the new API, these arguments are passed implicitly. If we do not want the matching policies to be written to the audit log (which doesn't happen often), we explicitly pass write_to_audit_log=False.
  • match_policies and get_action_values take very similar arguments. The only difference is that get_action_values performs some postprocessing. With the new API, we first call Match.func(g, ...) to specify our parameters for matching, and then call .policies() or .action_values(), depending on our needs.
  • There are more complicated cases of policy matching, e.g. if we want to match scope=ADMIN or scope=USER policies depending on the current logged-in user's role. Up until now, we had to manually put together the respective values of scope, user, realm and adminrealm. The new API provides a method for that:
      audit_age = Match.admin_or_user(g, action=ACTION.AUDIT_AGE, realm=user_object.realm).action_values(unique=True)
  • The new API makes it easier to extend policy matching in the future: e.g. if we want to allow matching of arbitrary request parameters (#1425), we only need to modify the Match class.
  • For cases of policy matching that do not fit in the provided methods (there are a few), match_policies/get_action_values still works :)

Do you think this is a reasonable refactoring? What do you think of the API?

I realize this is quite a big change, so I would be ok with postponing this until after privacyIDEA 3.1.

ref #1691


This comment has been minimized.

Copy link

commented Jul 24, 2019

Codecov Report

Merging #1757 into master will decrease coverage by 0.04%.
The diff coverage is 97.31%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1757      +/-   ##
- Coverage   97.05%     97%   -0.05%     
  Files         149     150       +1     
  Lines       18115   18007     -108     
- Hits        17581   17468     -113     
- Misses        534     539       +5
Impacted Files Coverage Δ
privacyidea/lib/tokens/ 100% <100%> (ø) ⬆️
privacyidea/lib/tokens/ 100% <100%> (ø) ⬆️
privacyidea/lib/ 99.29% <100%> (ø) ⬆️
privacyidea/api/ 90.9% <100%> (+0.13%) ⬆️
privacyidea/api/lib/ 97.88% <100%> (+0.49%) ⬆️
privacyidea/lib/ 99.28% <100%> (-0.01%) ⬇️
privacyidea/lib/tokens/ 98.12% <100%> (+0.01%) ⬆️
privacyidea/lib/ 98.79% <100%> (-0.13%) ⬇️
privacyidea/api/lib/ 98.07% <100%> (-0.23%) ⬇️
privacyidea/lib/ 91.52% <91.52%> (ø)
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4d352f...a97d825. Read the comment docs.

fredreichbier added some commits Jul 17, 2019

Start refactoring policy matching
This adds a function match_policies_strict as a short-hand
for the most usual calls to PolicyClass.match_policies.

Working on #1691
Refactor most usages of get_action_values
Instead, the more high-level match_policy_action_values_strict
is used.
Fix policy matching in allowed_audit_realm
Previously, allowed_audit_realm matched scope=ADMIN policies
even if the currently logged-in user was not an admin.
Preserve behavior of get_action_values_from_options
The function did not write matching policies to the audit log.

@fredreichbier fredreichbier force-pushed the 1691/match-policies branch from 8ee6ef1 to a97d825 Aug 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
1 participant
You can’t perform that action at this time.