WebexCommunity / WebexPythonSDK

Work with the Webex APIs in native Python!
https://webexcommunity.github.io/WebexPythonSDK/
MIT License
236 stars 152 forks source link

Spark token: read from env #2

Closed ObjectIsAdvantag closed 8 years ago

ObjectIsAdvantag commented 8 years ago

It is a good practice not to embedd secretes in code. Cloud practices (see 12factor.net) are to read from an env variable. I suggest we read by default from SPARK_TOKEN env variable.

Moreover, any sample we provide will leverage the env variable approach.

ObjectIsAdvantag commented 8 years ago

In term of implementation, I suggest we provide 2 constructors :

cmlccie commented 8 years ago

Yes, this is a best practice (which I personally use); however, I don't particularly favor specifying an environment variable that must be used by anyone wanting to use the package. My thoughts were that you would initialize your 'api object' with the access token that you want to use, where you source that access token from is up to you. This also facilitates creating multiple 'api objects' representing 'connections' to the Cisco Spark Cloud as multiple users, which can be useful in a few circumstances.

It would be well for the examples to illustrate this practice. Is there a benefit for coding a specific environment variable into package itself, or just let others retrieve their access token from an environment variable of their choosing?

ObjectIsAdvantag commented 8 years ago

an advantage is that it makes your library easier to use, and immediately integrates best practices instead of relying on developers to know about. Enforcing good practices to avoid token to be disclosed sound much more attractive.

Note that it is also a good practice to support both ways to initialize.

For an example in nodejs check https://github.com/ObjectIsAdvantag/sparkbot-webhook-samples/blob/master/sparkbot/webhook.js

function Webhook(config) {
    // Inject defaults if no configuration specified
    if (!config) {
        config = {
            port            : process.env.PORT || 8080,
            token           : process.env.SPARK_TOKEN
        };
    } 

Which makes the developer code as light as this : https://github.com/ObjectIsAdvantag/sparkbot-webhook-samples/blob/master/tests/onEvent-messages-created.js

var SparkBot = require("../sparkbot/webhook");

// Starts your Webhook with default configuration where the SPARK API access token is read from the SPARK_TOKEN env variable 
var bot = new SparkBot();
cmlccie commented 8 years ago

I can certainly add a check for the presence of an environment variable and default the initialization new CiscoSparkAPI objects to use the environment variable.

cmlccie commented 8 years ago

I have updated the Example on the README to reflect how I would normally pull and use my access token from my SPARK_ACCESS_TOKEN environment variable, and I will make sure that our example scripts use the same environment variable and represent this best practice.

I'm not completely sold on hard coding a check for a 'static' environment variable into the package. What if a user has named their environment variable differently than we do (i.e. we use "SPARK_ACCESS_TOKEN" and they use "SPARK_TOKEN" or something else)? What if people use different packages that each used differently environment variable names? Importing and supply their access token takes just two lines of code. I think it best for users to manage and use their own environment variables, rather than us specifying one.

We can, however, encourage the use with our examples.