Create hostfactory token vanilla flow 12446 #339

eranha · 2021-09-29T09:28:37Z

What does this PR do?

What's changed? Why were these changes made?
Create token - vanilla flow
Added menu parser controller logic and data classes
Basic skeleton was created to allow easy entry into task (Addition was made to help screen, appropriate classes were added)
A token can be created after adding a hostfactory policy

What ticket does this PR close?

12446

Checklists

Change log

The CHANGELOG has been updated, or
This PR does not include user-facing changes and doesn't require a CHANGELOG update

Test coverage

This PR includes new unit and integration tests to go with the code changes, or
The changes in this PR do not require tests

Documentation

Docs (e.g. READMEs) were updated in this PR, and/or there is a follow-on issue to update docs, or
This PR does not require updating any documentation

sigalsax · 2021-09-29T10:43:39Z

conjur/argument_parser/_hostfactory_parser.py

+                                                   '10.0.11.1/32,10.0.20.0/24")')
+        hostfactory_create_token.add_argument('-d', '--duration-days', metavar='VALUE', type=int,
+                                              help='(Optional) the number of days the token will be valid.')
+        hostfactory_create_token.add_argument('-H', '--duration-hours', metavar='VALUE', type=int,


please consult with Sapir on this

sigalsax · 2021-09-29T10:44:23Z

conjur/api/client.py

@@ -180,6 +181,12 @@ def get_many(self, *variable_ids) -> Optional[bytes]:
        """
        return self._api.get_variables(*variable_ids)

+    def create_token(self, create_token_data: CreateTokenData) -> str:
+        """
+        Create token/s for hosts with restrictions


we create only a single token, correct?

Suggested change

Create token/s for hosts with restrictions

Create token for hosts with restrictions

You pass an argument count and get the multiple tokens in response

sigalsax · 2021-09-29T10:45:29Z

conjur/api/api.py

@@ -13,7 +13,11 @@
 from datetime import datetime, timedelta

 # Internals
+from urllib import parse
+import requests


should be under # Third Parties

mbenita-Cyberark

very good work

mbenita-Cyberark · 2021-09-29T10:44:50Z

conjur/api/api.py

@@ -221,6 +225,28 @@ def get_variables(self, *variable_ids) -> dict:

        return remapped_keys_dict

+    def create_token(self, create_token_data: CreateTokenData) -> requests.Response:


you return response here but in the client.py calling this function you say you are returning a str. is it correct?

mbenita-Cyberark · 2021-09-29T10:47:23Z

conjur/api/api.py

+        This method is used to create token/s for hosts with restrictions.
+        """
+        if create_token_data is None:
+            raise MissingRequiredParameterException('create_token_data')


For consistency I suggest show an error message for MissingRequiredParameterException like in other places for example in hte api constructor "Account cannot be empty!"

mbenita-Cyberark · 2021-09-29T10:56:13Z

conjur/api/client.py

@@ -180,6 +181,12 @@ def get_many(self, *variable_ids) -> Optional[bytes]:
        """
        return self._api.get_variables(*variable_ids)

+    def create_token(self, create_token_data: CreateTokenData) -> str:


actually return response no?

mbenita-Cyberark · 2021-09-29T11:05:30Z

conjur/controller/hostfactory_controller.py

+        Method that facilitates create token call to the logic
+        """
+        if create_token_data is None:
+            raise MissingRequiredParameterException('create_token_data')


For consistency maybe something like Token data is missing? this is for the UI. Even though I have to say that your way is much easier to debug. @sigalsax maybe we should use this approach for all our MissingRequiredParameterException ?

@mbenita-Cyberark agreed. This error isn't enough to explain what happened. It will just print out 'create_token_data'. Please a more explanatory error of what happened at tag Shuli

@sigalsax his should not happen to user unless a developer did something wrong.

mbenita-Cyberark · 2021-09-29T11:06:51Z

conjur/data_object/create_token_data.py

+    Used for organizing the params the user passed in to execute the CreateToken command
+    """
+
+    def __init__(self, **arg_params):


why kwargs and not just simple parameters? is the input dynamically changes?

I think we should always be as explicit as possible and avoid kwargs

mbenita-Cyberark · 2021-09-29T11:09:12Z

conjur/data_object/create_token_data.py

+        if days == 0 and hours == 0 and minutes == 0:
+            return default_expiration
+
+        return datetime.now() + timedelta(


is the datetime.now allign to the server? for our testing env working with aws n.virgia it's not. should we work with utc?

@eladkug I think this is a very shaky point that could cause future bugs due to server changes. can you suggest a test for this?

both working with UTC so its o.k. (I verified with @eranha )

mbenita-Cyberark · 2021-09-29T11:10:34Z

conjur/api/api.py

+        params.update(self._default_params)
+
+        token_params = {}
+        for attr, value in create_token_data.__dict__.items():


if we use repr function only here maybe we could set it to return only fields that have values?

mbenita-Cyberark · 2021-09-29T11:12:56Z

conjur/logic/hostfactory_logic.py

+        if create_token_data is None:
+            raise MissingRequiredParameterException('create_token_data')


This is being validate in three different places.. with only two possible paths (sdk going to the api, and cli going controller-> logic -> api) maybe we can skip this validation?

This is the case now it can change I prefer not to make this assumption

mbenita-Cyberark · 2021-09-29T11:22:44Z

conjur/logic/hostfactory_logic.py

+
+        response = self.client.create_token(create_token_data)
+
+        if response is not None and response.json() is not None and len(response.json()) > 0:


we are deserializing the content three times with response.json()
maybe

Suggested change

if response is not None and response.json() is not None and len(response.json()) > 0:

if response is not None :

data = response.json()

if data is not None and len(data) > 0 :

return json.dumps(data, indent=4, sort_keys=True)

mbenita-Cyberark · 2021-09-29T11:26:30Z

conjur/wrapper/http_wrapper.py

 # pylint: disable=too-many-locals,consider-using-f-string
 # ssl_verify can accept Boolean or String as per requests docs
 # https://requests.readthedocs.io/en/master/api/#main-interface
 def invoke_endpoint(http_verb: HttpVerb, endpoint: ConjurEndpoint, params: dict, *args,
                    check_errors: bool = True, ssl_verify: bool = True,
                    auth: tuple = None, api_token: str = None,
-                    query: dict = None) -> requests.Response:
+                    query: dict = None, headers: dict = {}) -> requests.Response:


This is very dangerous and possibly the source of all evil headers: dict = {} pycharm should warn you about this.

great point @mbenita-Cyberark

orenbm · 2021-09-29T11:37:30Z

conjur/argument_parser/_hostfactory_parser.py

+        hostfactory_create_token.add_argument('--cidr', metavar='VALUE',
+                                              help='(Optional) the CIDR address that contains all IPs that can '
+                                                   'use this token to create hosts. Yoo can specify multiple cidr, '
+                                                   'separated by commas (for example --cidr "10.0.10.0/24,'
+                                                   '10.0.11.1/32,10.0.20.0/24")')
+        hostfactory_create_token.add_argument('-d', '--duration-days', metavar='VALUE', type=int,
+                                              help='(Optional) the number of days the token will be valid.')
+        hostfactory_create_token.add_argument('-H', '--duration-hours', metavar='VALUE', type=int,
+                                              help='(Optional) the number of hours the token will be valid.')
+        hostfactory_create_token.add_argument('-m', '--duration-minutes', metavar='VALUE', type=int,
+                                              help='(Optional) the number of minutes the token will be valid.')
+        hostfactory_create_token.add_argument('-c', '--count', metavar='VALUE', type=int,
+                                              help='(Optional) the number of times the token can be used.')


isn't this PR for vanilla flow only?

You mean no args?

orenbm · 2021-09-29T11:37:54Z

conjur/cli.py

+    @classmethod
+    def handle_hostfactory_logic(cls, args:list=None, client=None):
+        """
+            Method that wraps the hostfdactory call logic


Suggested change

Method that wraps the hostfdactory call logic

Method that wraps the hostfactory call logic

sigalsax · 2021-09-29T11:48:37Z

conjur/api/api.py

+                               parse.urlencode(token_params),
+                               api_token=self.api_token,
+                               ssl_verify=self._ssl_verify,
+                               headers={'Content-Type': 'application/x-www-form-urlencoded'})


can we return a JSON or a string instead of the response itself? Would be clearer

The documentation of the logic class
`HostFactoryLogic

This class holds the business logic for executing and manipulating returned data`

i dont follow

@sigalsax The logic responsible to manipulate the response

sigalsax · 2021-09-29T11:50:32Z

conjur/argument_parser/_hostfactory_parser.py

+
+        hostfactory_create_subcommand_parser = menu \
+            .add_parser(name="token",
+                        help=' Create token/s for hosts with restrictions',


Suggested change

help=' Create token/s for hosts with restrictions',

help='Create token/s for hosts with restrictions',

sigalsax · 2021-09-29T11:51:10Z

conjur/argument_parser/_hostfactory_parser.py

+        # Options
+        hostfactory_create_token = hostfactory_create_subcommand_parser.add_argument_group(
+            title=title_formatter("Options"))
+        hostfactory_create_token.add_argument('-action_type', default='create_token', help=argparse.SUPPRESS)


what is this?

It helps diff create from revoke they both end in cli.handle_hostfactory_logic

sigalsax · 2021-09-29T11:52:31Z

conjur/cli.py

+        if args.action_type == 'create_token':
+            hostfactory_logic = HostFactoryLogic(client)
+            create_token_data = CreateTokenData(hostfactoryid=args.hostfactoryid,
+                                                cidr=args.cidr,


i would make attributes in CreateTokenData more readible maybe id, cidr, days, hours, minutes, count

omit duration? is token days more clear than duration days?

I think the attribute names are just very long and we should make them dev-friendly :)

@sigalsax I agree

sigalsax · 2021-09-29T11:53:01Z

conjur/cli.py

@@ -290,6 +310,9 @@ def run_action(resource:str, args):
            result = client.whoami()
            print(json.dumps(result, indent=4))

+        elif resource == 'hostfactory':


looks better if we put it under 'host' logic. it becomes easier to spot

I don't follow

switch the order of the elifs. Move this block under the host elif block for readability

sigalsax · 2021-09-29T11:53:12Z

conjur/controller/hostfactory_controller.py

+from conjur.errors import MissingRequiredParameterException
+
+"""
+VariableController module


Suggested change

VariableController module

HostfactoryController module

sigalsax · 2021-09-29T11:56:01Z

conjur/data_object/create_token_data.py

+        self.count = arg_params['count'] if arg_params['count'] else 1
+
+        self.expiration = self.get_expiration(
+            arg_params['duration_days'] if arg_params['duration_days'] else 0,


please get Sapir's approval on this

on the default

sigalsax · 2021-09-29T11:56:17Z

conjur/logic/hostfactory_logic.py

+# -*- coding: utf-8 -*-
+
+"""
+VariableLogic module


Suggested change

VariableLogic module

HostfactoryLogic module

sigalsax · 2021-09-29T11:56:45Z

conjur/logic/hostfactory_logic.py

+"""
+
+# Internals
+import json


json isn't an Internals it is Builtins

sigalsax

Great work! @eranha
Left a few comments and please see the failing build. We are failing on the code linter suggestions so please address them :)

sigalsax · 2021-09-30T07:22:27Z

conjur/argument_parser/_hostfactory_parser.py

+        hostfactory_create_token = hostfactory_create_subcommand_parser.add_argument_group(
+            title=title_formatter("Options"))
+        hostfactory_create_token.add_argument('-action_type', default='create_token', help=argparse.SUPPRESS)
+        hostfactory_create_token.add_argument('-i', '--hostfactoryid', metavar='VALUE',


@eranha i think this sould be --hostfactory-id

@sigalsax see my comment below

sigalsax · 2021-09-30T07:23:54Z

conjur/argument_parser/_hostfactory_parser.py

+                        description=command_description(hostfactory_name,
+                                                        hostfactory_usage),
+                        epilog=command_epilog(
+                            'conjur hostfactory create token --hostfactoryid my_factory --cidr 10.10.1.2/31 '


think it should be --hostfactory-id

@sigalsax this is how it appears in the sign-off doc

consult Sapir with this because the rest of the flags have - (duration-days, etc)

sigalsax · 2021-09-30T07:26:04Z

conjur/api/api.py

+        This method is used to create token/s for hosts with restrictions.
+        """
+        if create_token_data is None:
+            raise MissingRequiredParameterException('create_token_data cannot be empty!')


@eranha is this a user facing log? If so, this doesn't really explain anything

@sigalsax I consider it as an argument exception (runtime so to speak) it is intended for the developer

sigalsax · 2021-09-30T12:15:22Z

conjur/argument_parser/_hostfactory_parser.py

+                        description=command_description(hostfactory_name,
+                                                        hostfactory_usage),
+                        epilog=command_epilog(
+                            'conjur hostfactory create token --hostfactoryid my_factory '


not a blocker but this should be verified by Sapir (if it needs a -)

sigalsax · 2021-09-30T12:15:46Z

conjur/argument_parser/_hostfactory_parser.py

+                                       '10.0.11.1/32,10.0.20.0/24")')
+        create_token.add_argument('-d', '--duration-days', metavar='VALUE', type=int,
+                                  help='(Optional) the number of days the token will be valid.')
+        create_token.add_argument('-H', '--duration-hours', metavar='VALUE', type=int,


was anything decided here regarding the -H?

@sigalsax not that I know of

sigalsax · 2021-09-30T12:17:02Z

conjur/data_object/create_token_data.py

+    @staticmethod
+    def get_expiration(duration: timedelta) -> datetime:
+        """
+        Returns the token expiration in UTC; One hour of not specified.


Suggested change

Returns the token expiration in UTC; One hour of not specified.

Returns the token expiration in UTC; One hour if not specified.

sigalsax · 2021-09-30T12:18:25Z

conjur/logic/hostfactory_logic.py

+                                                            doseq=True))
+        if response is not None:
+            data = response.json()
+            if data is not None and len(data) > 0:


why do we need a compound check?
if Data is not none then its length automatically will be greater > 0 no?

not necessarily

Basic skeleton was created to allow easy entry into task (Addition was made to help screen, appropriate classes were added)

sigalsax · 2021-09-30T13:50:55Z

conjur/cli.py

            days = args.duration_days if args.duration_days else 0
            hours = args.duration_hours if args.duration_hours else 0
            minutes = args.duration_minutes if args.duration_minutes else 0
+            duration = timedelta(days=days, hours=hours, minutes=minutes)
+            default_duration = timedelta(hours=1)


thought to put this in the controller actually @eranha

since this is the view

Basic skeleton was created to allow easy entry into task (Addition was made to help screen, appropriate classes were added)

sigalsax

Great work!

eranha requested a review from sigalsax September 29, 2021 09:28

eranha requested review from a team as code owners September 29, 2021 09:28

sigalsax reviewed Sep 29, 2021

View reviewed changes

mbenita-Cyberark reviewed Sep 29, 2021

View reviewed changes

orenbm reviewed Sep 29, 2021

View reviewed changes

sigalsax reviewed Sep 29, 2021

View reviewed changes

sigalsax self-requested a review September 30, 2021 05:39

sigalsax suggested changes Sep 30, 2021

View reviewed changes

eranha requested review from sigalsax, mbenita-Cyberark and orenbm September 30, 2021 07:08

sigalsax reviewed Sep 30, 2021

View reviewed changes

eranha requested a review from sigalsax September 30, 2021 12:10

sigalsax reviewed Sep 30, 2021

View reviewed changes

eranha added 10 commits September 30, 2021 15:40

Create token - vanilla flow

6a20535

Basic skeleton was created to allow easy entry into task (Addition was made to help screen, appropriate classes were added)

Create token - vanilla flow

c4d2865

Basic skeleton was created to allow easy entry into task (Addition was made to help screen, appropriate classes were added)

Create token - vanilla flow

50e24d7

Basic skeleton was created to allow easy entry into task (Addition was made to help screen, appropriate classes were added)

Create token - vanilla flow

d53ef7f

Basic skeleton was created to allow easy entry into task (Addition was made to help screen, appropriate classes were added)

Create token - vanilla flow

f0f4ed8

Basic skeleton was created to allow easy entry into task (Addition was made to help screen, appropriate classes were added)

Create token - vanilla flow

26918ba

Basic skeleton was created to allow easy entry into task (Addition was made to help screen, appropriate classes were added)

Create token - vanilla flow

d0f24df

Basic skeleton was created to allow easy entry into task (Addition was made to help screen, appropriate classes were added)

Create token - vanilla flow

e1e680d

Basic skeleton was created to allow easy entry into task (Addition was made to help screen, appropriate classes were added)

Create token - vanilla flow

0e79ace

Basic skeleton was created to allow easy entry into task (Addition was made to help screen, appropriate classes were added)

Create token - vanilla flow

12c4563

Basic skeleton was created to allow easy entry into task (Addition was made to help screen, appropriate classes were added)

eranha force-pushed the create_token_vanilla_flow_12446 branch from e8d48d2 to 12c4563 Compare September 30, 2021 12:40

eranha added 2 commits September 30, 2021 15:54

Create token - vanilla flow

a997248

Basic skeleton was created to allow easy entry into task (Addition was made to help screen, appropriate classes were added)

Create token - vanilla flow

75dfc2c

Basic skeleton was created to allow easy entry into task (Addition was made to help screen, appropriate classes were added)

eranha requested a review from sigalsax September 30, 2021 13:39

sigalsax reviewed Sep 30, 2021

View reviewed changes

Create token - vanilla flow

5fc4083

Basic skeleton was created to allow easy entry into task (Addition was made to help screen, appropriate classes were added)

eranha requested a review from sigalsax September 30, 2021 14:24

eranha added 2 commits September 30, 2021 17:31

Create token - vanilla flow

568cda2

Basic skeleton was created to allow easy entry into task (Addition was made to help screen, appropriate classes were added)

Create token - vanilla flow

e9f1eb0

Basic skeleton was created to allow easy entry into task (Addition was made to help screen, appropriate classes were added)

sigalsax approved these changes Sep 30, 2021

View reviewed changes

eranha merged commit 56d92ff into main Sep 30, 2021

eranha deleted the create_token_vanilla_flow_12446 branch September 30, 2021 15:21

sigalsax changed the title ~~Create token vanilla flow 12446~~ Create hostfactory token vanilla flow 12446 Oct 4, 2021

jtuttle mentioned this pull request Jan 19, 2022

Release v1.15.0+suite.1 cyberark/conjur-oss-suite-release#242

Merged

	Create token/s for hosts with restrictions
	Create token for hosts with restrictions

		@@ -221,6 +225,28 @@ def get_variables(self, *variable_ids) -> dict:

		return remapped_keys_dict

		def create_token(self, create_token_data: CreateTokenData) -> requests.Response:

		if create_token_data is None:
		raise MissingRequiredParameterException('create_token_data')


		response = self.client.create_token(create_token_data)

		if response is not None and response.json() is not None and len(response.json()) > 0:

-        if response is not None and response.json() is not None and len(response.json()) > 0:
+        if response is not None :
+           data = response.json()
+            if data is not None and len(data) > 0 :
+                return json.dumps(data, indent=4, sort_keys=True)

	Method that wraps the hostfdactory call logic
	Method that wraps the hostfactory call logic

	help=' Create token/s for hosts with restrictions',
	help='Create token/s for hosts with restrictions',

	Returns the token expiration in UTC; One hour of not specified.
	Returns the token expiration in UTC; One hour if not specified.

Create hostfactory token vanilla flow 12446 #339

Create hostfactory token vanilla flow 12446 #339

Conversation

eranha commented Sep 29, 2021 • edited by szh Loading

What does this PR do?

What ticket does this PR close?

Checklists

Change log

Test coverage

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eranha Sep 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbenita-Cyberark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbenita-Cyberark Sep 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sigalsax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sigalsax Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sigalsax left a comment

Choose a reason for hiding this comment

eranha commented Sep 29, 2021 •

edited by szh

Loading

eranha Sep 29, 2021 •

edited

Loading

mbenita-Cyberark Sep 29, 2021 •

edited

Loading

sigalsax Sep 30, 2021 •

edited

Loading