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

more eval for config items #307

Closed
pierremarc opened this issue Feb 17, 2016 · 9 comments
Closed

more eval for config items #307

pierremarc opened this issue Feb 17, 2016 · 9 comments
Labels

Comments

@pierremarc
Copy link

When trying to set up a quite circumvoluted configuration involving OAUTH2, I ran into the problem of not being able to call functions from the pythonfile everywhere I needed to.
When I realized it was by design, I slightly modified CustomConfigParser.getdefault to test if the option might be evaluated. It's dirty, but it worked.

diff --git a/offlineimap/CustomConfig.py b/offlineimap/CustomConfig.py
index 44cfcab..7cb2820 100644
--- a/offlineimap/CustomConfig.py
+++ b/offlineimap/CustomConfig.py
@@ -32,9 +32,14 @@ class CustomConfigParser(SafeConfigParser):
     def getdefault(self, section, option, default, *args, **kwargs):
         """Same as config.get, but returns the value of `default`
         if there is no such option specified."""
-
         if self.has_option(section, option):
-            return self.get(*(section, option) + args, **kwargs)
+            value = self.get(*(section, option) + args, **kwargs)
+            if self.localeval:
+                try:
+                    value = self.localeval.eval(value)
+                except Exception:
+                    pass
+            return value
         else:
             return default

What I can think of for a proper implementation would be a dedicated subsection for evaluable options but it's lacking from ConfigParser, would need to use another parser.
Another way would be an [eval] section where option would be of the form <section>.<option> , evaled and inserted in their sections.

@nicolas33
Copy link
Member

I understand this patch may work for you. However, we don't want each option to be evaluable.

I don't know your use case but offlineimap is not designed to support all sort of complex configurations. Changing of parser sounds rather improbable.

Perhaps you'd like a new evaluable configuration option for OAUTH2?

@pierremarc
Copy link
Author

However, we don't want each option to be evaluable.

I understand, I was even surprised it worked out. The patch was here as an illustration.
My point, if any, is that some options would greatly benefit from a computed value (e.g. #288 ), and it's difficult to predict this in advance ---we people have imagination :)---. So whatever mechanism that would allow a user to instruct the program to eval a specific option would be welcome.

@nicolas33
Copy link
Member

Look at how current computed options are evaluated. AFAIR, they are all dedicated options. Hence my advice to introduce new dedicated options regardless they serve the same purpose of already existing options.

It's good to provide a use case while at it. I already declined features because of the lack of clear use case. The reason is that each code change is an agreement that the benefits overtake the inherent complexity introduced by the change.

@pierremarc
Copy link
Author

It's about OAuth2 for GMail with client ID, client secret and refresh token saved in a keyring.
I totally understand that you don't want to bother bringing much complexity. But here the access token has to be regenerated often and the refresh token is also variable && somewhat persistent. I found it easier to use the pythonfile helper rather than messing with the config file each time offlineimap would run.

@nicolas33
Copy link
Member

I don't get why storing data in a keyring helps for values exposed to changes. However, it makes sense from a privacy/security POV. Feel free to go for it. ,-)

@pierremarc
Copy link
Author

To be honest, if I'd to write it myself, I'd go for an over simplification and just check for the start of the value to match a pattern, e.g (in vim style with an extra space):

an_option = ! evaluate_this()

Maybe too simple?

@nicolas33
Copy link
Member

Well, if this could work that's not correct. There's no reason to introduce edge-cases in the code that will break one day or a another. Especially for something that trivial with all the required code already there and factorized.

@nicolas33
Copy link
Member

This would make sense for security reasons for:

  • oauth2_client_id
  • oauth2_client_secret
  • oauth2_access_token
  • oauth2_refresh_token

nicolas33 added a commit to nicolas33/offlineimap that referenced this issue Jul 23, 2016
Introduce:
- oauth2_client_id_eval
- oauth2_client_secret_eval
- oauth2_access_token_eval
- oauth2_refresh_token_eval

Github-ref: OfflineIMAP#307
Signed-off-by: Nicolas Sebrecht <nicolas.s-dev@laposte.net>
@nicolas33
Copy link
Member

Please, test above version (on top of v7.0.0).

nicolas33 added a commit to nicolas33/offlineimap that referenced this issue Jul 23, 2016
Introduce:
- oauth2_client_id_eval
- oauth2_client_secret_eval
- oauth2_access_token_eval
- oauth2_refresh_token_eval

Github-ref: OfflineIMAP#307
Signed-off-by: Nicolas Sebrecht <nicolas.s-dev@laposte.net>
nicolas33 added a commit to nicolas33/offlineimap that referenced this issue Jul 23, 2016
Introduce:
- oauth2_client_id_eval
- oauth2_client_secret_eval
- oauth2_access_token_eval
- oauth2_refresh_token_eval

Github-ref: OfflineIMAP#307
Signed-off-by: Nicolas Sebrecht <nicolas.s-dev@laposte.net>
nicolas33 added a commit that referenced this issue Jul 27, 2016
Introduce:
- oauth2_client_id_eval
- oauth2_client_secret_eval
- oauth2_access_token_eval
- oauth2_refresh_token_eval

Github-ref: #307
Signed-off-by: Nicolas Sebrecht <nicolas.s-dev@laposte.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants