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

Rollbar::init() shouldn't require access token when using agent as handler #405

Closed
joshrai opened this issue Aug 30, 2018 · 4 comments
Closed
Assignees
Labels
Type: Bug Fix a component so that its behavior aligns with its documentation.
Milestone

Comments

@joshrai
Copy link

joshrai commented Aug 30, 2018

In v0.18.2, it's legal and workable to call Rollbar::init() without passing an access_token entry in the $config param, if you do pass an entry of 'handler' => 'agent':

rollbar-php/src/rollbar.php

Lines 190 to 192 in fafe13f

if (!$this->access_token && $this->handler != 'agent') {
$this->log_error('Missing access token');
}

But in v1.6.2, and possibly other earlier versions, Rollbar::init() always requires an access_token entry, and furthermore requires it to be a string of length 32, because it always reaches this line:

$this->utilities->validateString($config['access_token'], "config['access_token']", 32, false);

I think this should not be required if we're using the Rollbar agent as the handler, because this library shouldn't need to communicate with Rollbar's server in that case; it should just be writing events to a local log file that a separate agent process will pick up. I was advised by support that this might have been an oversight, and that using a dummy value should serve as a workaround. That seems a little messy and misleading; I think it would be better if the library detected when it does and doesn't need an access token.

I tried this out using HHVM 3.24.7, but it seems like the code path in question would be relevant for any PHP or HHVM version.

@rivkahstandig3636 rivkahstandig3636 added the Type: Bug Fix a component so that its behavior aligns with its documentation. label Aug 31, 2018
@rivkahstandig3636 rivkahstandig3636 added this to the Spikes milestone Aug 31, 2018
@jessewgibbs jessewgibbs added Type: Enhancement Changes that add to, improve upon, enhance, or extend the existing component. and removed Type: Enhancement Changes that add to, improve upon, enhance, or extend the existing component. labels Sep 10, 2018
@jessewgibbs jessewgibbs removed this from the Spikes milestone Oct 4, 2018
@joshrai
Copy link
Author

joshrai commented Oct 10, 2018

By the way, it looks like the workaround of using an arbitrary 32-char value for access_token doesn't work after all. Providing a dummy value does bypass the validation that the library does, but it seems that you need to pass a real access token to the client library in order for the agent to succeed. If I use a dummy value in the Rollbar::init() configuration, I see this error in the Rollbar agent log when I try to log something:

2018-10-08 23:34:57,327 WARNI Unexpected response from Rollbar API. Code: 401 Body: {
"err": 1,
"message": "invalid access token"
}

and log entries do not get through to the Rollbar service. If I use the correct access token in Rollbar::init(), then my log entries do get through to Rollbar, and I no longer see that error in the agent log.

My understanding is that you must configure the agent itself to know the access token, which made me think that requiring it additionally in the client seemed redundant. Maybe there is a security rationale for requiring it in both places. But if so, it would be great for the rollbar-php docs to explain why. Thanks!

@JohnKendhammer
Copy link
Contributor

I think it would be beneficial to leave the access_token in both places. We don't want the Agent to be building the payload. That should not be it's responsibility. The payload delivered to the agent should be self-sufficient and that is why the SDK requires the access_token. The Agent currently has a check that populates the access_token if it is missing from the payload. Removing the access_token from either side would mean that we could possibly cause failures in user apps that are relying on that check and we are also removing the additional sanity check. At the end of the day I think the best thing we could do is to leave the access_token in both places.

@joshrai are you experiencing any issues involved with having the access_token in both places?

@JohnKendhammer
Copy link
Contributor

@joshrai You are very much right that this is not updated in the documentation and is something that I am updating.

@ArturMoczulski ArturMoczulski added this to the Spikes milestone Oct 17, 2018
@joshrai
Copy link
Author

joshrai commented Oct 26, 2018

@JohnKendhammer - sorry for not responding sooner. It's definitely doable for me to continue providing the access token to both the SDK and the agent. I was mostly just curious what the rationale was, and your explanation makes sense, especially given that existing users might be relying on the current behavior.

That said, if you believe this really shouldn't be the agent's responsibility, maybe it would make sense to deprecate the agent-side check/population behavior in some future version, and give people ample warning that they need to populate it in the payload (i.e., be even more explicit about requiring it when using the SDK, even in agent mode). But I don't feel strongly about that.

Thanks for working on the documentation! I think that would be helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Fix a component so that its behavior aligns with its documentation.
Projects
None yet
Development

No branches or pull requests

5 participants