Updated compliance api requests to actually use refresh token correctly #1416
Conversation
| @@ -55,6 +55,9 @@ def self.resolve(target) # rubocop:disable PerceivedComplexity, Metrics/Cyclomat | |||
| end | |||
| profile_fetch_url = Compliance::API.target_url(config, profile) | |||
| end | |||
| # We need to pass the token to the fetcher | |||
| config['token'] = Compliance::API.get_token(config) | |||
There was a problem hiding this comment.
Looking at this it might make sense to just follow this convention everywhere. Just on each request we set the config['token'] so it lives the entire life of the command but then do not save this to disk.
There was a problem hiding this comment.
Good idea. In general we want to avoid having the configuration in our API. Right now, storing it on disk is the only way to share it with the fetcher, since they run independently.
There was a problem hiding this comment.
So currently this does not save to disk so if that is a requirement this approach might need to be changed.
| @@ -179,7 +179,7 @@ def upload(path) # rubocop:disable Metrics/MethodLength, Metrics/AbcSize, Percei | |||
| end | |||
|
|
|||
| # determine user information | |||
| if config['token'].nil? || config['user'].nil? | |||
| if (config['token'].nil? && config['refresh_token'].nil?) || config['user'].nil? | |||
There was a problem hiding this comment.
Having one or the other is now a valid use case, if a token is still in the config it will default to using that (which is the legacy behavior
| end | ||
| headers | ||
| end | ||
|
|
||
| def self.get_token(config) | ||
| return config['token'] unless config['refresh_token'] | ||
|
|
There was a problem hiding this comment.
we do not need the spaces here
| @@ -344,11 +343,10 @@ def store_refresh_token(url, refresh_token, verify, user, insecure) | |||
| success = true | |||
| msg = 'API refresh token stored' | |||
| else | |||
| success, msg, access_token = Compliance::API.get_token_via_refresh_token(url, refresh_token, insecure) | |||
| success, msg = Compliance::API.get_token_via_refresh_token(url, refresh_token, insecure) | |||
There was a problem hiding this comment.
I think we need the _access_token here as well
| @@ -55,6 +55,9 @@ def self.resolve(target) # rubocop:disable PerceivedComplexity, Metrics/Cyclomat | |||
| end | |||
| profile_fetch_url = Compliance::API.target_url(config, profile) | |||
| end | |||
| # We need to pass the token to the fetcher | |||
| config['token'] = Compliance::API.get_token(config) | |||
There was a problem hiding this comment.
Good idea. In general we want to avoid having the configuration in our API. Right now, storing it on disk is the only way to share it with the fetcher, since they run independently.
|
@chris-rock are the functional tests known to have these failures or is that something I broke? |
|
I am taking care of the functional tests right now. |
|
@brentm5 Can you rebase on latest master? functional tests are fixed there. I apologise for the inconvenience. |
We do not store a token in the config file but rather generate one on each commmand. This is just a first pass and needs some work. Signed-off-by: Montague, Brent <brent@bmontague.com>
Signed-off-by: Montague, Brent <brent@bmontague.com>
brentm5
force-pushed
the
bm-add-refresh-token
branch
from
e4c42cf
to
463f3ff
Compare
|
@chris-rock Done |
|
Very nice improvement @brentm5 |
We do not store a token in the config file but rather generate one on each command called. This also does not cache any tokens but generates them every time. Not sure if this is ideal or not but this a first pass and needs some work. Also there are no tests added yet. At this point just looking for feedback on the approach. This is to help solve #1414