Kqueen ldap #52

naumvd95 · 2018-04-04T09:16:18Z

refactor env-vars arch to provide configuration using only docker env-vars
remove apply_env changes due issues with bad overriding in case of multiple value-entries in config file
logging increased

atengler · 2018-04-04T12:38:27Z

kqueen_ui/config/auth.py

+from kqueen_ui.config import current_config
+
+
+class AuthModules():


Is there a reason to create this class?

atengler · 2018-04-04T12:40:01Z

kqueen_ui/config/utils.py

    """

    for name, value in os.environ.items():
        if name.startswith(prefix):
            config_key_name = name[len(prefix):]
+            if re.search('(?i)true|(?i)false', value):


If we are converting strings to bool, can't we convert json string to list/dict in python and avoid creating unnecessary class?

katyafervent · 2018-04-05T11:52:14Z

.travis.yml

@@ -27,7 +27,7 @@ jobs:
    - pip3 install --editable .
    - kqueen-ui &
    - sleep 2
-    - wget -O - http://localhost:8000/
+    - wget -O - http://localhost:5080/


Is this change connected with ldap changes?

its needed to pass tests

katyafervent · 2018-04-05T12:02:08Z

kqueen_ui/blueprints/manager/views.py

@@ -169,7 +170,10 @@ class MemberCreate(KQueenView):

    def handle(self, organization_id):
        form_cls = MemberCreateForm
-        auth_options = app.config.get('AUTH_OPTIONS', {})
+
+        auth_options = AUTH_MODULES


this is misleading, you can use AUTH_MODULES directly

katyafervent · 2018-04-05T12:02:56Z

kqueen_ui/config/base.py

@@ -56,6 +57,8 @@ class BaseConfig:
    PROVISIONER_UNKNOWN_STATE = 'Not Reachable'

    # Authentication choices
+    LDAP_AUTH_NOTIFY = False
+    LOCAL_AUTH_NOTIFY = True
    AUTH_OPTIONS = {}


we don't have AUTH_OPTIONS anymore as I understand correctly

katyafervent · 2018-04-05T12:04:53Z

kqueen_ui/config/test.py

@@ -2,27 +2,23 @@


 class Config(BaseConfig):
-    DEBUG = True
+    DEBUG = False


Are you sure you want to set it to False?

atengler · 2018-04-06T09:49:07Z

This change is not equivalent to the current configuration. When you move the auth dict sample from config to code, you can no longer use local auth only for example. Can you alter the code so it is still configurable which auth backends we use?

katyafervent · 2018-04-06T09:58:30Z

kqueen_ui/blueprints/ui/views.py

-        auth_options = app.config.get('AUTH_OPTIONS', {})
-        if auth_options:
+
+        if AUTH_MODULES:


you are imported it directly, this check is redundant

katyafervent · 2018-04-06T10:01:26Z

kqueen_ui/config/base.py

@@ -56,7 +57,8 @@ class BaseConfig:
    PROVISIONER_UNKNOWN_STATE = 'Not Reachable'

    # Authentication choices
-    AUTH_OPTIONS = {}
+    LDAP_AUTH_NOTIFY = False


In the rest of the file boolean variables are strings, let's keep consistent and dot he same for those paramaters

in my opinion it redundant, those variables should be bool ideally
in docker env bool vars is not allowed in compose file docker/compose#2000
so imo docker-env and config files should not be equal

IMO one way we should avoid quotes - is defining all docker vars into '.env' file. There is allowed to use normal bool vars, but it is another PR, does not related to this case

ok, so need to fix ENABLE_PUBLIC_REGISTRATION = 'False' in that file in a separate change

ok, so pls approve changes @atengler @katyafervent

katyafervent · 2018-04-06T10:01:36Z

kqueen_ui/config/demo.py

+    # Auth configuration
+
+    # Enable email notifications to user
+    LDAP_AUTH_NOTIFY = False


same as above

in my opinion it redundant, those variables should be bool ideally
in docker env bool vars is not allowed in compose file docker/compose#2000
so imo docker-env and config files should not be equal

IMO one way we should avoid quotes - is defining all docker vars into '.env' file. There is allowed to use normal bool vars, but it is another PR, does not related to this case

- refactor arch for Auth modules management

katyafervent

LGTM, but I suggest to use one style for commit messages - with the capital letter

- configuration file parsing refactores to increase visibility and decrease cyclomatic complexity - bool-checker added to avoid issues with defining bool variables as string in docker-compose

naumvd95 · 2018-04-09T15:59:10Z

@atengler pls clarify info about auth_config, i don't understand what do u mean

naumvd95 requested review from atengler and katyafervent April 4, 2018 09:58

atengler reviewed Apr 4, 2018

View reviewed changes

naumvd95 force-pushed the kqueen-ldap branch 2 times, most recently from 90971bb to 06cf7a5 Compare April 5, 2018 10:34

naumvd95 requested a review from atengler April 5, 2018 10:39

katyafervent reviewed Apr 5, 2018

View reviewed changes

naumvd95 force-pushed the kqueen-ldap branch 3 times, most recently from bcfd4ea to fe41a4c Compare April 5, 2018 13:53

naumvd95 force-pushed the kqueen-ldap branch from fe41a4c to f628f0c Compare April 6, 2018 09:55

katyafervent suggested changes Apr 6, 2018

View reviewed changes

naumvd95 force-pushed the kqueen-ldap branch from f628f0c to cae1aba Compare April 6, 2018 10:28

katyafervent previously approved these changes Apr 6, 2018

View reviewed changes

naumvd95 dismissed katyafervent’s stale review via 2673353 April 6, 2018 14:35

naumvd95 force-pushed the kqueen-ldap branch from cae1aba to 2673353 Compare April 6, 2018 14:35

OPS LDAP support

b7467d4

- refactor arch for Auth modules management

naumvd95 force-pushed the kqueen-ldap branch from 2673353 to e1dfbc5 Compare April 9, 2018 07:43

katyafervent reviewed Apr 9, 2018

View reviewed changes

atengler previously approved these changes Apr 9, 2018

View reviewed changes

naumvd95 dismissed atengler’s stale review via 7e20f81 April 9, 2018 13:13

naumvd95 force-pushed the kqueen-ldap branch from e1dfbc5 to 7e20f81 Compare April 9, 2018 13:13

naumvd95 added 6 commits April 9, 2018 17:15

Increase logging from Auth modules

fb6fcc7

Refactor Kqueen Configuration managing

0fd405d

- configuration file parsing refactores to increase visibility and decrease cyclomatic complexity - bool-checker added to avoid issues with defining bool variables as string in docker-compose

Refactor default variables in Config files

84a1c41

Refactor arch for Auth modules management

3db6aeb

Change checker for UI access

84c0783

Move AUTH modules into common auth package

8955e0f

naumvd95 force-pushed the kqueen-ldap branch from 7e20f81 to 8955e0f Compare April 9, 2018 13:16

naumvd95 requested review from katyafervent and atengler April 9, 2018 13:17

katyafervent approved these changes Apr 9, 2018

View reviewed changes

katyafervent merged commit 8d3f440 into master Apr 9, 2018

katyafervent deleted the kqueen-ldap branch June 15, 2018 12:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kqueen ldap #52

Kqueen ldap #52

naumvd95 commented Apr 4, 2018

atengler Apr 4, 2018

atengler Apr 4, 2018

katyafervent Apr 5, 2018

naumvd95 Apr 5, 2018 •

edited

Loading

katyafervent Apr 5, 2018

katyafervent Apr 5, 2018

katyafervent Apr 5, 2018

atengler commented Apr 6, 2018

katyafervent Apr 6, 2018

katyafervent Apr 6, 2018

naumvd95 Apr 6, 2018

naumvd95 Apr 6, 2018

katyafervent Apr 6, 2018

naumvd95 Apr 9, 2018

katyafervent Apr 6, 2018

naumvd95 Apr 6, 2018

naumvd95 Apr 6, 2018

katyafervent left a comment

naumvd95 commented Apr 9, 2018

		from kqueen_ui.config import current_config


		class AuthModules():

Kqueen ldap #52

Kqueen ldap #52

Conversation

naumvd95 commented Apr 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naumvd95 Apr 5, 2018 • 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

atengler commented Apr 6, 2018

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

katyafervent left a comment

Choose a reason for hiding this comment

naumvd95 commented Apr 9, 2018

naumvd95 Apr 5, 2018 •

edited

Loading