Adding option to explicitly set token, token_name on initialization #11
Conversation
|
Not sure, but guessing we need to record new cassettes for testing here. |
lib/hackerone/client.rb
Outdated
| unless ENV["HACKERONE_TOKEN_NAME"] && ENV["HACKERONE_TOKEN"] | ||
| raise NotConfiguredError, "HACKERONE_TOKEN_NAME HACKERONE_TOKEN environment variables must be set" | ||
|
|
||
| unless client_token_name && client_token |
There was a problem hiding this comment.
there may also be a VCR issue but I think a lot of the these test failures are related to client_token_name not being defined, should this be token_name as defined on line 73?
There was a problem hiding this comment.
Ooooh ... did I miss one of those instances (I thought I had cleaned up)? Good catch and thanks for the 👀
There was a problem hiding this comment.
Updated that (which needed updating all the same) and still getting all those failures ... 🤔 @rzhade3 can you take a peek when you have a moment. You're closer to this (historically at least) than I am.
rzhade3
left a comment
rzhade3
left a comment
There was a problem hiding this comment.
Some thoughts; for context the reason I never picked up this issue to work on was the insufficient encapsulation in the HackerOne::Client object which makes it difficult to pass instance level variables to it 🙃
| end | ||
|
|
||
| def token_name | ||
| @token_name || ENV["HACKERONE_TOKEN_NAME"] |
There was a problem hiding this comment.
When would this be needed? As far as I can tell, this conditional would only ever evaluate if the value of the HACKERONE_TOKEN_NAME token wasn't set when the client was initialized?
I suppose it would help from a backwards compatibility perspective, but just want to make sure this line earns its place in the code.
| @program = program | ||
| @token = token || ENV["HACKERONE_TOKEN"] | ||
| @token_name = token_name || ENV["HACKERONE_TOKEN_NAME"] | ||
| # Set class-level token and token_name if provided |
There was a problem hiding this comment.
Ehhhhh, if we're setting this token at a class level then this is no better logically than if we just continue to use environment variables; part of the impetus to use tokens as variables is such that there is instance level encapsulation. This adds side effects across entire classes which defeats the purpose of this in the first place. This would clobber any calls to HackerOne::Client methods anytime after a new API object is created, which doesn't seem ideal. I think 🤔
It is also tough because that class is designed to operate as a class, which makes encapsulation really tough.
Some alternatives:
- Accept this as tech debt, and accept that you can only have one instance of
HackerOne::Clientrunning - Put an instance of the
HackerOne::Cientinside theHackerOne::Apiobject, and operate on that object. This would certainly not be backwards compatible. - Do not add this side effect, in which case
HackerOne::Clientand all subclasses would only be able to respond to the environment variable - Deprecate the HackerOne::Client class. I would prefer this, but this is also certainly not backwards compatible
- Rewrite the
HackerOne::Clientobject such that it becomes a class instead of an object
There was a problem hiding this comment.
Appreciate the feedback ...
Deprecate the HackerOne::Client class. I would prefer this, but this is also certainly not backwards compatible
I think I prefer this too, but timeline changes with that. Gonna have to think on it a little more 🙇🏼
Adding ability to explicitly (but optional, falling back to
ENV) set token and token_name when initializing the client. Some issues with testing. WIP for now.