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

HostsMachineResolver uses wrong parameter name #1912

Closed
jmozd opened this issue Nov 3, 2019 · 8 comments
Assignees
Labels
Milestone

Comments

@jmozd
Copy link

@jmozd jmozd commented Nov 3, 2019

Fresh install of privacyIDEA 3.1.1 via Python Package Index.

When adding a machine resolver via "Config" - "Machines" - "New HOSTS resolver", I can fill in a "File name" field, to specify the location of the "hosts" file to parse.

Although saving that value produces no error, immediately re-editing the resolver settings shows an empty field and listing all "Machines" (top-level) produces an empty list.

Looking at the logs, immediately the following catches the eye:

[2019-11-03 16:54:12,461][10838][140138801751808][WARNING][privacyidea.lib.utils:442] the passed key 'fileName' is not a parameter for the machine resolver 'hosts'
[2019-11-03 16:54:17,631][10838][140138801751808][ERROR][flask.app:1761] Exception on /machine/ [GET]
Traceback (most recent call last):
  File "/opt/privacyidea/lib/python3.6/site-packages/flask/app.py", line 2292, in wsgi_app
    response = self.full_dispatch_request()
  File "/opt/privacyidea/lib/python3.6/site-packages/flask/app.py", line 1815, in full_dispatch_request
[...]
  File "/opt/privacyidea/lib/python3.6/site-packages/privacyidea/lib/machines/hosts.py", line 142, in load_config
    raise MachineResolverError("filename is missing!")
privacyidea.lib.machines.base.MachineResolverError: filename is missing!

The reason seems to be that the UI uses the parameter "fileName", while the code in HostsMachineResolver looks for "filename". Changing the resolver code solved the issue for me:

server:/opt/privacyidea/lib/python3.6/site-packages/privacyidea # diff -C3  /opt/privacyidea/lib/python3.6/site-packages/privacyidea/lib/machines/hosts.py.dist /opt/privacyidea/lib/python3.6/site-packages/privacyidea/lib/machines/hosts.py
*** /opt/privacyidea/lib/python3.6/site-packages/privacyidea/lib/machines/hosts.py.dist 2019-11-01 16:41:46.000000000 +0100
--- /opt/privacyidea/lib/python3.6/site-packages/privacyidea/lib/machines/hosts.py      2019-11-03 16:59:13.000000000 +0100
***************
*** 137,149 ****
          :type config: dict
          :return: None
          """
!         self.filename = config.get("filename")
          if self.filename is None:
              raise MachineResolverError("filename is missing!")
  
      @classmethod
      def get_config_description(cls):
!         description = {cls.type: {"config": {"filename": "string"}}}
          return description
  
      @staticmethod
--- 137,149 ----
          :type config: dict
          :return: None
          """
!         self.filename = config.get("fileName")
          if self.filename is None:
              raise MachineResolverError("filename is missing!")
  
      @classmethod
      def get_config_description(cls):
!         description = {cls.type: {"config": {"fileName": "string"}}}
          return description
  
      @staticmethod
server:/opt/privacyidea/lib/python3.6/site-packages/privacyidea #

@cornelinux

This comment has been minimized.

Copy link
Member

@cornelinux cornelinux commented Nov 4, 2019

I see that you are running python 3.6.
For some reason this problem was not there there previously. I can reproduce the problem.
(Could it be for some reason due to pyhton 2.7 and 3.6.)

However, If you want to, you are welcome to provide a corresponding pull request.

filename is stored lower case in the database.

@NuvandaPV

This comment has been minimized.

Copy link
Contributor

@NuvandaPV NuvandaPV commented Nov 6, 2019

I can reproduce the same behavior with Python 3.7.3.

@cornelinux

This comment has been minimized.

Copy link
Member

@cornelinux cornelinux commented Nov 7, 2019

Hm, maybe I am a bit confused now. I think we should do this in lower case, so that existing database values can be read.
But then it looks like, we should change this in the webui, @NuvandaPV ?

I am again a bit confused, because the only place, where "fileName" exists is in the config controller:
https://github.com/privacyidea/privacyidea/blob/master/privacyidea/static/components/config/controllers/configControllers.js#L752

But in the view, we use params.filename.
https://github.com/privacyidea/privacyidea/blob/master/privacyidea/static/components/config/views/config.mresolvers.hosts.html#L35

Could it be, that javascript/angularJS, changes this to "fileName", since it was already set in the controller? So we would have to fix the controller?

Please take a look to fix this (so that "filename" is stored in the db)

@cornelinux cornelinux added the Type: Bug label Nov 7, 2019
@cornelinux cornelinux added this to the 3.2 milestone Nov 7, 2019
@NuvandaPV

This comment has been minimized.

Copy link
Contributor

@NuvandaPV NuvandaPV commented Nov 11, 2019

So I have had a closer look and this seems to be an easy fix, we just have to pick one capitalization and stick with it. Are you sure filename is the way to go though? The PasswdResolver (whose code is rather similar) uses fileName as well, it feels a bit inconsistent to me to change just the HostsMachineResolver on its own.

Changing the capitalization would also mean having to write a database migration. We could avoid that by sticking with the current version, however we might want to write a migration either way, since triggering this bug will leave an orphaned machineresolverconfig (with an empty resolver_id) in the database. Although I guess cleaning that up is not crucial.

From my understanding this has worked in the past, which means we also need to make sure that the changes we make to fix this do not break preexisting resolvers. I am assuming right now these use fileName? I cannot check, as this has always been broken for me, and I cannot seem to produce the behavior we had previously to this bug. @cornelinux do you have a database you could check?

@cornelinux

This comment has been minimized.

Copy link
Member

@cornelinux cornelinux commented Nov 11, 2019

Old installations stored "filename" in the DB. So if we simply change the host config to "filename" we should be fine.

@NuvandaPV

This comment has been minimized.

Copy link
Contributor

@NuvandaPV NuvandaPV commented Nov 12, 2019

Ok, will do.

@cornelinux

This comment has been minimized.

Copy link
Member

@cornelinux cornelinux commented Nov 18, 2019

fixed and merged

@cornelinux cornelinux closed this Nov 18, 2019
@jmozd

This comment has been minimized.

Copy link
Author

@jmozd jmozd commented Nov 18, 2019

Thank you all for your support!

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