[Pki-devel] [PATCH] 98-2 Fixes for comments on patch 98
Ade Lee
alee at redhat.com
Wed Jun 25 16:47:50 UTC 2014
In profile.py:
1. Usually, we set @author and so on after the copyright notice.
2. In PolicyDefault, you have methods to add/remove/get attributes and
parameters, as in:
def add_attribute(self, profile_attribute):
self.policy_attributes.append(profile_attribute)
Change profile_attribute to policy_attribute etc. You should indicate in
a docstring that the expected type for the policy_attribute etc. is a
ProfileAttribute.
3. I'm not a big fan of your _send_profile_request() and
_send_request_in_file() methods in that they mix create and update
operations. That might make sense if the operations had a lot of common
code and were doing the same HTTP operation, but otherwise is just
confusing.
A better way to split this up might be to define:
def _send_profile_create(self, profile_data)
def _send_profile_modify(self, profile_id, profile_data)
@staticmethod
def _get_profile_data_from_file(path_to_file, file_format)
and then call these as needed.
So for example, create_profile_using_file_input(self, path_to_file,
file_format) -> which I would rename to create_profile_from_file(..) ->
would call:
profile_data = _get_profile_data_from_file(path_to_file, file_format)
_send_profile_create(profile_data)
4. In the test where you add a duplicate profile, you should check for
the specific exception thrown -- is there a specific exception?
5. Change the profileID in the test to MySampleProfile instead of
MySampleCert.
6. In the test, we are testing file input using original.xml and
modified.xml. Where do these files come from? It would be better if
these files were generated as part of the previous tests - which means
you need a function to save files to xml/json. You should then test xml
and json.
7. The policy stuff gets confusing fast, so the example really helps.
We need more function docstrings though that specify when inputs/outputs
the functions have - and in particular the class of each input.
8. We also need more api documentation on the wiki as we have done in
the past. This is not part of this patch of course.
9. There are Pycharm errors about unused variables. Most of these will
be addressed by the refactoring I mention in point 3 above.
10. profile.py - which is all new code generates probably close to 200
new messages in pylint. We really should fix these. To keep things
clear, you should do this in a separate patch.
On Wed, 2014-06-25 at 16:49 +1000, Fraser Tweedale wrote:
> On Tue, Jun 24, 2014 at 05:32:25PM -0400, Abhishek Koneru wrote:
> > Please review the patch with fixes for comments on patch 98.
> > Issues addressed:
> > 1. Added a try catch block for issue 1(as noted below) caught by
> > ftweedal(nice catch!).
> > 2. Made the data_format mandatory for file_input methods.
> > 3. Added code to create a profile with PolicyOutput and PolicySets.
> >
> > -- Abhishek
> > Please apply patch 97 before applying this patch.
> >
>
> ACK
>
> > On Fri, 2014-06-20 at 12:02 +1000, Fraser Tweedale wrote:
> > > On Thu, Jun 19, 2014 at 09:58:43AM -0400, Abhishek Koneru wrote:
> > > > Please review the attached patch which adds methods that allow users to
> > > > pass the profile data in a file.
> > > >
> > > > Also attached two sample xml files, original.xml and modified.xml.
> > > > Place them in /tmp before running the profile.py module.
> > > >
> > > > Thanks,
> > > > Abhishek
> > > >
> > >
> > > Hi Abhishek,
> > >
> > > Patch applied and __main__ example run. Seems to do what it says on
> > > the tin; the usual "haven't used it in anger" caveat ^_^. Review
> > > comments inline below.
> > >
> > > Fraser
> > >
> > > > + @pki.handle_exceptions()
> > > > def modify_profile(self, profile_data):
> > > > """
> > > > Modify an existing profile.
> > > > """
> > > > - if profile_data is None:
> > > > - raise ValueError("No ProfileData specified")
> > > > + return self._send_profile_request(profile_data, 'modify')
> > > > +
> > > > + def _send_request_in_file(self, path_to_file, data_format, operation):
> > > > +
> > > > + if path_to_file is None:
> > > > + raise ValueError("File path must be specified.")
> > > > +
> > > > + if data_format not in ['xml', 'json']:
> > > > + raise ValueError("Unsupported data type: " + str(data_format))
> > > > +
> > > > + if operation not in ['create', 'modify']:
> > > > + raise ValueError("Invalid operation specified: " + str(operation))
> > > > +
> > > > + data = None
> > > > + with open(path_to_file) as input_file:
> > > > + data = input_file.read()
> > > > +
> > > > + if data_format == 'xml':
> > > > + self.headers['Content-type'] = 'application/xml'
> > >
> > > 1)
> > >
> > > Overwriting self.headers['Content-type'] is problematic. For
> > > example, if the data cannot be parsed or lacks an 'id' key, an
> > > exception will be raised and the ProfileClient will be stuck with
> > > the wrong Content-Type header.
> > >
> > > Possible solutions:
> > >
> > > - use try/finally or a context manager to ensure the header gets
> > > reset to 'application/json' even if an exception is raised.
> > > - Modify the _put and _post methods to include an optional argument
> > > to override the content-type header. To avoid special-cases too
> > > many things, this arg could even be a dict that can be merged with
> > > the default headers, e.g.:
> > >
> > > def _post(self, url, payload=None, headers=None):
> > > self.account_client.login()
> > > headers = dict(self.headers, **(headers or {}))
> > > r = self.connection.post(url, payload, headers, query_params)
> > > ...
> > >
> > > Then callers can supply custom headers for a single request
> > > without potentially affecting other requests.
> > >
> > > > +
> > > > + r = None
> > > > + # Sending the data to the server.
> > > > + if operation == 'create':
> > > > + r = self._post(self.profiles_url, data)
> > > > + else:
> > > > + profile_id = None
> > > > + if data_format == 'xml':
> > > > + profile_id = etree.fromstring(data).get('id')
> > > > + else:
> > > > + profile_id = json.loads(data)['id']
> > > > +
> > > > + if profile_id is None:
> > > > + raise ValueError('Profile Id is not specified.')
> > > > + url = self.profiles_url + '/' + profile_id
> > > > + r = self._put(url, data)
> > > > +
> > > > + # Reset the Content-type header to json(As used by other methods).
> > > > + if data_format == 'xml':
> > > > + self.headers['Content-type'] = 'application/json'
> > > >
> > > > - url = self.profiles_url + '/' + str(profile_data.profile_id)
> > > > - profile_object = json.dumps(profile_data, cls=encoder.CustomTypeEncoder,
> > > > - sort_keys=True)
> > > > - r = self._put(url, profile_object)
> > > > return Profile.from_json(r.json())
> > > >
> > > > + @pki.handle_exceptions()
> > > > + def create_profile_using_file_input(self, path_to_file, data_format="xml"):
> > > > + """
> > > > + Create a new profile from a profile object stored in a file.
> > > > + Acceptable data formats - json, xml.
> > > > + """
> > > > + return self._send_request_in_file(path_to_file, data_format, 'create')
> > > > +
> > >
> > > 2)
> > >
> > > Default ``data_format="xml"`` makes it too easy for people to misuse
> > > the API, e.g. a path to a JSON file, but no ``data_format`` kwarg
> > > given, resulting in default "xml" being used. I suggest either
> > > making it a compulsory argument, or making its default ``None`` and,
> > > if no explicit ``data_format`` arg given, using the file extension.
> > >
> > >
> > > > + @pki.handle_exceptions()
> > > > + def modify_profile_using_file_input(self, path_to_file, data_format="xml"):
> > > > + """
> > > > + Modify a profile from a profile object stored in a file.
> > > > + Acceptable data formats - json, xml.
> > > > + """
> > > > + return self._send_request_in_file(path_to_file, data_format, 'modify')
> > > > +
> > > > + @pki.handle_exceptions()
> > > > def delete_profile(self, profile_id):
> > > > """
> > > > Delete a profile.
> > > > @@ -1185,6 +1253,23 @@ def main():
> > > > # pylint: disable-msg=W0703
> > > > except Exception as e:
> > > > print str(e)
> > > > + print
> > > > +
> > > > + # Creating a profile from file
> > > > + print('Creating a profile using file input.')
> > > > + print('------------------------------------')
> > > > + original = profile_client.create_profile_using_file_input(
> > > > + '/tmp/original.xml')
> > > > + print(original)
> > > > + print
> > > > +
> > > > + # Modifying a profile from file
> > > > + print('Modifying a profile using file input.')
> > > > + print('------------------------------------')
> > > > + modified = profile_client.modify_profile_using_file_input(
> > > > + '/tmp/modified.xml')
> > > > + print(modified)
> > > > + print
> > >
> > > 3)
> > >
> > > Nit-pick: could you delete the created profile here so that the
> > > example can be run multiple times?
> >
>
> > From 3a031e463e6fb0b5658ad9e1bc37a018c99f32de Mon Sep 17 00:00:00 2001
> > From: Abhishek Koneru <akoneru at redhat.com>
> > Date: Thu, 19 Jun 2014 00:10:13 -0400
> > Subject: [PATCH] Added methods for providing file input for profile request.
> >
> > Added new methods to allow user to provide file input to perform
> > operations like create profile/modify profile.
> > The supported file formats a re xml and json.
> > ---
> > base/common/python/pki/__init__.py | 4 +-
> > base/common/python/pki/profile.py | 265 ++++++++++++++++++++++++++++++++++---
> > 2 files changed, 252 insertions(+), 17 deletions(-)
> >
> > diff --git a/base/common/python/pki/__init__.py b/base/common/python/pki/__init__.py
> > index e9b726cf763785b4a700ef314ff27774b13aba40..891d6ea6364b037f132ff3754b73b372c638b0f7 100644
> > --- a/base/common/python/pki/__init__.py
> > +++ b/base/common/python/pki/__init__.py
> > @@ -168,7 +168,7 @@ class PKIException(Exception, ResourceMessage):
> > ret = cls(json_value['Message'], json_value['Code'],
> > json_value['ClassName'])
> > for attr in json_value['Attributes']['Attribute']:
> > - print(str(attr))
> > + print str(attr)
> > ret.add_attribute(attr["name"], attr["value"])
> > return ret
> >
> > @@ -299,7 +299,7 @@ class PropertyFile(object):
> > def show(self):
> > """ Show contents of property file."""
> > for line in self.lines:
> > - print(line)
> > + print line
> >
> > def insert_line(self, index, line):
> > """ Insert line in property file """
> > diff --git a/base/common/python/pki/profile.py b/base/common/python/pki/profile.py
> > index 4f08ee5ba9527855f63b2785c32f9aaddc8d1289..2d4ce5ace11bc6417032fb89ecf6b18707b6fca0 100644
> > --- a/base/common/python/pki/profile.py
> > +++ b/base/common/python/pki/profile.py
> > @@ -1,12 +1,27 @@
> > #!/usr/bin/python
> > """
> > -Created on May 13,, 2014
> > + at author: Abhishek Koneru <akoneru at redhat.com>
> >
> > - at author: akoneru
> > + This program is free software; you can redistribute it and/or modify
> > + it under the terms of the GNU General Public License as published by
> > + the Free Software Foundation; version 2 of the License.
> > +
> > + This program is distributed in the hope that it will be useful,
> > + but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + GNU General Public License for more details.
> > +
> > + You should have received a copy of the GNU General Public License along
> > + with this program; if not, write to the Free Software Foundation, Inc.,
> > + 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> > +
> > + Copyright (C) 2014 Red Hat, Inc.
> > + All rights reserved.
> > """
> >
> > import json
> > import types
> > +from lxml import etree
> >
> > import pki
> > import pki.client as client
> > @@ -433,6 +448,38 @@ class PolicyDefault(object):
> > def policy_params(self, value):
> > setattr(self, 'params', value)
> >
> > + def add_attribute(self, profile_attribute):
> > + self.policy_attributes.append(profile_attribute)
> > +
> > + def remove_attribute(self, profile_attribute_name):
> > + for attr in self.policy_attributes:
> > + if attr.name == profile_attribute_name:
> > + self.policy_attributes.remove(attr)
> > + break
> > +
> > + def get_attribute(self, profile_attribute_name):
> > + for attr in self.policy_attributes:
> > + if attr.name == profile_attribute_name:
> > + return attr
> > +
> > + return None
> > +
> > + def add_parameter(self, profile_parameter):
> > + self.policy_params.append(profile_parameter)
> > +
> > + def remove_parameter(self, profile_parameter_name):
> > + for param in self.policy_params:
> > + if param.name == profile_parameter_name:
> > + self.policy_params.remove(param)
> > + break
> > +
> > + def get_parameter(self, profile_parameter_name):
> > + for param in self.policy_params:
> > + if param.name == profile_parameter_name:
> > + return param
> > +
> > + return None
> > +
> > @classmethod
> > def from_json(cls, json_value):
> > policy_def = cls()
> > @@ -531,6 +578,22 @@ class PolicyConstraint(object):
> > def policy_constraint_values(self, value):
> > setattr(self, 'constraint', value)
> >
> > + def add_constraint_value(self, policy_constraint_value):
> > + self.policy_constraint_values.append(policy_constraint_value)
> > +
> > + def remove_attribute(self, policy_constraint_value_name):
> > + for attr in self.policy_constraint_values:
> > + if attr.name == policy_constraint_value_name:
> > + self.policy_constraint_values.remove(attr)
> > + break
> > +
> > + def get_attribute(self, policy_constraint_value_name):
> > + for constraint in self.policy_constraint_values:
> > + if constraint.name == policy_constraint_value_name:
> > + return constraint
> > +
> > + return None
> > +
> > @classmethod
> > def from_json(cls, json_value):
> > policy_constraint = cls()
> > @@ -995,31 +1058,99 @@ class ProfileClient(object):
> > """
> > return self._modify_profile_state(profile_id, 'disable')
> >
> > + def _send_profile_request(self, profile_data, operation):
> > +
> > + if profile_data is None:
> > + raise ValueError("No ProfileData specified")
> > +
> > + if operation not in ['create', 'modify']:
> > + raise ValueError("Invalid operation specified: " + str(operation))
> > +
> > + profile_object = json.dumps(profile_data, cls=encoder.CustomTypeEncoder,
> > + sort_keys=True)
> > + r = None
> > + if operation == 'create':
> > + r = self._post(self.profiles_url, profile_object)
> > + else:
> > + if profile_data.profile_id is None:
> > + raise ValueError("Profile Id is not specified.")
> > + url = self.profiles_url + '/' + str(profile_data.profile_id)
> > + r = self._put(url, profile_object)
> > +
> > + return Profile.from_json(r.json())
> > +
> > + @pki.handle_exceptions()
> > def create_profile(self, profile_data):
> > """
> > Create a new profile for the given ProfileData object.
> > """
> > - if profile_data is None:
> > - raise ValueError("No ProfileData specified")
> > -
> > - profile_object = json.dumps(profile_data, cls=encoder.CustomTypeEncoder,
> > - sort_keys=True)
> > - r = self._post(self.profiles_url, profile_object)
> > - return Profile.from_json(r.json())
> > + return self._send_profile_request(profile_data, 'create')
> >
> > + @pki.handle_exceptions()
> > def modify_profile(self, profile_data):
> > """
> > Modify an existing profile.
> > """
> > - if profile_data is None:
> > - raise ValueError("No ProfileData specified")
> > + return self._send_profile_request(profile_data, 'modify')
> > +
> > + def _send_request_in_file(self, path_to_file, data_format, operation):
> > +
> > + if path_to_file is None:
> > + raise ValueError("File path must be specified.")
> > +
> > + if data_format not in ['xml', 'json']:
> > + raise ValueError("Unsupported data type: " + str(data_format))
> > +
> > + if operation not in ['create', 'modify']:
> > + raise ValueError("Invalid operation specified: " + str(operation))
> > +
> > + data = None
> > + with open(path_to_file) as input_file:
> > + data = input_file.read()
> > +
> > + if data_format == 'xml':
> > + self.headers['Content-type'] = 'application/xml'
> > +
> > + r = None
> > + try:
> > + # Sending the data to the server.
> > + if operation == 'create':
> > + r = self._post(self.profiles_url, data)
> > + else:
> > + profile_id = None
> > + if data_format == 'xml':
> > + profile_id = etree.fromstring(data).get('id')
> > + else:
> > + profile_id = json.loads(data)['id']
> > +
> > + if profile_id is None:
> > + raise ValueError('Profile Id is not specified.')
> > + url = self.profiles_url + '/' + profile_id
> > + r = self._put(url, data)
> > + finally:
> > + # Reset the Content-type header to json(As used by other methods).
> > + if data_format == 'xml':
> > + self.headers['Content-type'] = 'application/json'
> >
> > - url = self.profiles_url + '/' + str(profile_data.profile_id)
> > - profile_object = json.dumps(profile_data, cls=encoder.CustomTypeEncoder,
> > - sort_keys=True)
> > - r = self._put(url, profile_object)
> > return Profile.from_json(r.json())
> >
> > + @pki.handle_exceptions()
> > + def create_profile_using_file_input(self, path_to_file, data_format):
> > + """
> > + Create a new profile from a profile object stored in a file.
> > + Acceptable data formats - json, xml.
> > + """
> > + return self._send_request_in_file(path_to_file, data_format, 'create')
> > +
> > + @pki.handle_exceptions()
> > + def modify_profile_using_file_input(self, path_to_file, data_format):
> > + """
> > + Modify a profile from a profile object stored in a file.
> > + Acceptable data formats - json, xml.
> > + """
> > + return self._send_request_in_file(path_to_file, data_format, 'modify')
> > +
> > + @pki.handle_exceptions()
> > def delete_profile(self, profile_id):
> > """
> > Delete a profile.
> > @@ -1108,6 +1239,7 @@ def main():
> > renewal=False, xml_output=False,
> > authorization_acl="")
> >
> > + # Adding a profile input
> > profile_input = ProfileInput("i1", "subjectNameInputImpl")
> > profile_input.add_attribute(ProfileAttribute("sn_uid"))
> > profile_input.add_attribute(ProfileAttribute("sn_e"))
> > @@ -1121,6 +1253,92 @@ def main():
> >
> > profile_data.add_input(profile_input)
> >
> > + # Adding a profile output
> > + profile_output = ProfileOutput("o1", name="Certificate Output",
> > + class_id="certOutputImpl")
> > + profile_output.add_attribute(ProfileAttribute("pretty_cert"))
> > + profile_output.add_attribute(ProfileAttribute("b64_cert"))
> > +
> > + profile_data.add_output(profile_output)
> > +
> > + # Create a Policy set with a list of profile policies
> > + policy_list = []
> > +
> > + # Creating profile policy
> > + policy_default = PolicyDefault("Subject Name Default",
> > + "userSubjectNameDefaultImpl",
> > + "This default populates a User-Supplied "
> > + "Certificate Subject Name to the request.")
> > +
> > + attr_descriptor = Descriptor(syntax="string", description="Subject Name")
> > + policy_attribute = ProfileAttribute("name", descriptor=attr_descriptor)
> > + policy_default.add_attribute(policy_attribute)
> > +
> > + policy_constraint = PolicyConstraint("Subject Name Constraint",
> > + "This constraint accepts the subject "
> > + "name that matches UID=.*",
> > + "subjectNameConstraintImpl")
> > + constraint_descriptor = Descriptor(syntax="string",
> > + description="Subject Name Pattern")
> > + policy_constraint_value = PolicyConstraintValue("pattern",
> > + "UID=.*",
> > + constraint_descriptor)
> > + policy_constraint.add_constraint_value(policy_constraint_value)
> > +
> > + policy_list.append(ProfilePolicy("1", policy_default, policy_constraint))
> > +
> > + # Creating another profile policy
> > + # Defining the policy default
> > + policy_default = PolicyDefault("Validity Default", "validityDefaultImpl",
> > + "This default populates a Certificate "
> > + "Validity to the request. The default "
> > + "values are Range=180 in days")
> > + attr_descriptor = Descriptor(syntax="string", description="Not Before")
> > + policy_attribute = ProfileAttribute("notBefore", descriptor=attr_descriptor)
> > + policy_default.add_attribute(policy_attribute)
> > +
> > + attr_descriptor = Descriptor(syntax="string", description="Not After")
> > + policy_attribute = ProfileAttribute("notAfter", descriptor=attr_descriptor)
> > + policy_default.add_attribute(policy_attribute)
> > +
> > + profile_param = ProfileParameter("range", 180)
> > + profile_param2 = ProfileParameter("startTime", 0)
> > + policy_default.add_parameter(profile_param)
> > + policy_default.add_parameter(profile_param2)
> > +
> > + #Defining the policy constraint
> > + policy_constraint = PolicyConstraint("Validity Constraint",
> > + "This constraint rejects the validity "
> > + "that is not between 365 days.",
> > + "validityConstraintImpl")
> > + constraint_descriptor = Descriptor(syntax="integer",
> > + description="Validity Range (in days)",
> > + default_value=365)
> > + policy_constraint_value = PolicyConstraintValue("range", 365,
> > + constraint_descriptor)
> > + policy_constraint.add_constraint_value(policy_constraint_value)
> > +
> > + constraint_descriptor = Descriptor(syntax="boolean", default_value=False,
> > + description="Check Not Before against"
> > + " current time")
> > + policy_constraint_value = PolicyConstraintValue("notBeforeCheck", False,
> > + constraint_descriptor)
> > + policy_constraint.add_constraint_value(policy_constraint_value)
> > +
> > + constraint_descriptor = Descriptor(syntax="boolean", default_value=False,
> > + description="Check Not After against"
> > + " Not Before")
> > + policy_constraint_value = PolicyConstraintValue("notAfterCheck", False,
> > + constraint_descriptor)
> > + policy_constraint.add_constraint_value(policy_constraint_value)
> > +
> > + policy_list.append(ProfilePolicy("2", policy_default, policy_constraint))
> > +
> > + policy_set = PolicySet("userCertSet", policy_list)
> > +
> > + profile_data.add_policy_set(policy_set)
> > +
> > + # Create a new profile
> > created_profile = profile_client.create_profile(profile_data)
> > print(created_profile)
> > print
> > @@ -1185,6 +1403,23 @@ def main():
> > # pylint: disable-msg=W0703
> > except Exception as e:
> > print str(e)
> > + print
> > +
> > + # Creating a profile from file
> > + print('Creating a profile using file input.')
> > + print('------------------------------------')
> > + original = profile_client.create_profile_using_file_input(
> > + '/tmp/original.xml', 'xml')
> > + print(original)
> > + print
> > +
> > + # Modifying a profile from file
> > + print('Modifying a profile using file input.')
> > + print('------------------------------------')
> > + modified = profile_client.modify_profile_using_file_input(
> > + '/tmp/modified.xml', 'xml')
> > + print(modified)
> > + print
> >
> >
> > if __name__ == "__main__":
> > --
> > 1.8.5.3
> >
>
> _______________________________________________
> Pki-devel mailing list
> Pki-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel
More information about the Pki-devel
mailing list