Enforce requiring API key to initialise notifier #280

fractalwrench · 2018-05-18T10:01:23Z

Goal

Avoid instantiating the Notifier in an invalid state (without an API key).

Changeset

Changed

Nullability annotations on API
Gate all required methods in Bugsnag.m with a check to see if the notifier has been initialised, and NOP if not.
Avoid initialising notifier if the supplied API key is nil.
Attempting to set a non-nil API key as nil is ignored

Tests

Added a unit test for API key setter override.

Discussion

Much of the discussion for this changeset took place here: bugsnag/bugsnag-react-native#237

Review

For the submitter, initial self-review:

Commented on code changes inline explain the reasoning behind the approach
Reviewed the test cases added for completeness and possible points for discussion
A changelog entry was added for the goal of this pull request
Check the scope of the changeset - is everything in the diff required for the pull request?
This pull request is ready for:
- Initial review of the intended approach, not yet feature complete
- Structural review of the classes, functions, and properties modified
- Final review

For the pull request reviewer(s), this changeset has been reviewed for:

Consistency across platforms for structures or concepts added or modified
Consistency between the changeset and the goal stated above
Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
Usage friction - is the proposed change in usage cumbersome or complicated?
Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
Concurrency concerns - if components are accessed asynchronously, what issues will arise
Thoroughness of added tests and any missing edge cases
Idiomatic use of the language

Alter the API key annotation to Nonnull rather than Nullable. If the API key is set as nil, the value is ignored and a warning logged.

Gates all methods in Bugsnag.m so that they no-op and log a warning if the notifier has not been initialised. Also avoids initialising the notifier if the API key is not supplied.

snmaynard

Would be good to have some tests for the behaviour.

snmaynard · 2018-05-19T18:16:48Z

Source/Bugsnag.m

-        bsg_g_bugsnag_notifier =
-        [[BugsnagNotifier alloc] initWithConfiguration:configuration];
-        [bsg_g_bugsnag_notifier start];
+        if (configuration != nil && configuration.apiKey != nil) {


Might be nicer to have a configuration based validation method. [configuration isValid] for example, rather than the Bugsnag class having to know what that means.

snmaynard · 2018-05-19T18:18:22Z

By tests I mean, that configuration isValid works as expected and that you can't start bugsnag if that function returns false.

fractalwrench · 2018-05-21T08:49:58Z

Added a test to BugsnagConfiguration to encapsulate the API key check.

martin308

Left a couple of comments, a larger question I have is: it looks like the bugsnagStarted method only does a nil check on the notifier and so it is possible that the notifier won't be started as it is assigned to the variable before being started, does this matter?

martin308 · 2018-05-21T16:42:41Z

Source/BugsnagConfiguration.m

@@ -231,4 +246,9 @@ - (NSDictionary *)sessionApiHeaders {
             kHeaderApiSentAt: [BSG_RFC3339DateTool stringFromDate:[NSDate new]]
             };
 }
+
+- (BOOL)hasValidApiKey {
+    return _apiKey != nil && ![@"" isEqualToString:_apiKey];


could we just use return [_apiKey length] == 0;?

Flipped the boolean but yeah, [_apiKey length] > 0 is the ticket here.

martin308 · 2018-05-21T16:47:34Z

Source/BugsnagConfiguration.m

+
+- (NSString *)apiKey {
+    return _apiKey;
+}


could we remove this as it is just the default generated by @synthesize?

I think it needs to be specified since setApiKey is overridden.

martin308 · 2018-05-21T16:48:59Z

Source/BugsnagConfiguration.h

@@ -69,7 +69,7 @@ typedef NSDictionary *_Nullable (^BugsnagBeforeNotifyHook)(
 /**
 *  The API key of a Bugsnag project
 */
-@property(readwrite, retain, nullable) NSString *apiKey;
+@property(readwrite, retain, nonnull) NSString *apiKey;


is this a breaking change? Do we need to increment the major version number?

This is a breaking change in Swift at least, as anyone fetching the value would need to update their code with the appropriate number of ? and !. Its also not enforced, as BugsnagConfiguration().apiKey could be nil.

We should not make this change just yet and instead design/think more about the interface, such as automatic API key detection from Info.plist or having a recommended constructor which takes an non-null API key as an argument. This change is also not required for this fix.

kattrali · 2018-05-22T04:43:48Z

Source/BugsnagConfiguration.m

+}
+
+- (void)setApiKey:(NSString *)apiKey {
+    if (apiKey != nil) {


if (apiKey.length > 0) would be more comprehensive here and catch both the nil and the empty string case.

kattrali · 2018-05-22T04:45:45Z

Source/BugsnagConfiguration.m

@@ -231,4 +246,9 @@ - (NSDictionary *)sessionApiHeaders {
             kHeaderApiSentAt: [BSG_RFC3339DateTool stringFromDate:[NSDate new]]
             };
 }
+
+- (BOOL)hasValidApiKey {
+    return _apiKey != nil && ![@"" isEqualToString:_apiKey];


Flipped the boolean but yeah, [_apiKey length] > 0 is the ticket here.

kattrali · 2018-05-22T04:46:22Z

Tests/BugsnagConfigurationTests.m

+    XCTAssertFalse([config hasValidApiKey]);
+
+    config.apiKey = @"5adf89e0aaa";
+    XCTAssertTrue([config hasValidApiKey]);


config.apiKey = "" should also be false.

That's covered by L122, as config defaults to @""

… than checking null

fractalwrench · 2018-05-22T08:50:55Z

@martin308 good catch with bugsnagStarted - I've altered this so that it checks a boolean property which is set to true when the notifier has been started, rather than checking for nil.

I have addressed all other inline feedback.

martin308 · 2018-05-22T17:44:52Z

Source/Bugsnag.m

@@ -161,7 +177,7 @@ + (void)clearTabWithName:(NSString *)tabName {
 }

 + (BOOL)bugsnagStarted {
-    if (self.notifier == nil) {
+    if (!self.notifier.started) {


Does this handle the notifier being nil?

Yes, as nil will be coerced to false.

Since we already had it through synthesized properties, keeps it in case its used.

bengourley

The merge invalidated the last review due to conflicts. I'm approving the merge conflict resolution here, not the code changes 👍

$fractalwrench$

fractalwrench added 2 commits May 18, 2018 10:34

$@fractalwrench$

test: Enforce non-nil api key

084e88c

Alter the API key annotation to Nonnull rather than Nullable. If the API key is set as nil, the value is ignored and a warning logged.

$@fractalwrench$

feat: Gate Bugsnag calls with check to see if notifier initialised

1200634

Gates all methods in Bugsnag.m so that they no-op and log a warning if the notifier has not been initialised. Also avoids initialising the notifier if the API key is not supplied.

$@fractalwrench$ fractalwrench requested a review from a team May 18, 2018 10:01

$@fractalwrench$

docs: Add changelog entry for requiring api key

71032ed

$@fractalwrench$ fractalwrench mentioned this pull request May 18, 2018

fix: ignore empty api key from react native bugsnag/bugsnag-react-native#237

Merged

15 tasks

snmaynard reviewed May 19, 2018

View reviewed changes

$@fractalwrench$

test: Verify that configuration has a valid api key

ce966a6

$@fractalwrench$ fractalwrench requested a review from kattrali May 21, 2018 09:52

martin308 reviewed May 21, 2018

View reviewed changes

kattrali reviewed May 22, 2018

View reviewed changes

fractalwrench added 3 commits May 22, 2018 09:29

$@fractalwrench$

refactor: Tweak apiKey config in response to review feedback

47a3426

$@fractalwrench$

fix: Add a boolean property to check when notifier initialised rather…

2638a69

… than checking null

$@fractalwrench$

fix(make started property atomic):

1f6c4f3

martin308 reviewed May 22, 2018

View reviewed changes

$@fractalwrench$ fractalwrench requested a review from a team May 23, 2018 08:57

Keep key-value observability support

8a91bf8

Since we already had it through synthesized properties, keeps it in case its used.

kattrali previously approved these changes May 24, 2018

View reviewed changes

$@fractalwrench$

Merge branch 'master' into api-key-init-fixes

3102f15

$@fractalwrench$ fractalwrench dismissed kattrali’s stale review via 3102f15 May 24, 2018 07:45

bengourley approved these changes May 24, 2018

View reviewed changes

$@fractalwrench$ fractalwrench merged commit 5ed93f5 into master May 24, 2018

$@fractalwrench$ fractalwrench deleted the api-key-init-fixes branch May 24, 2018 08:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce requiring API key to initialise notifier #280

Enforce requiring API key to initialise notifier #280

$@fractalwrench$ fractalwrench commented May 18, 2018 •

edited

Loading

snmaynard left a comment

snmaynard May 19, 2018

snmaynard commented May 19, 2018

fractalwrench commented May 21, 2018

martin308 left a comment

martin308 May 21, 2018

kattrali May 22, 2018

martin308 May 21, 2018

kattrali May 22, 2018

martin308 May 21, 2018

kattrali May 22, 2018

kattrali May 22, 2018

kattrali May 22, 2018

kattrali May 22, 2018

$@fractalwrench$ fractalwrench May 22, 2018

fractalwrench commented May 22, 2018

martin308 May 22, 2018

$@fractalwrench$ fractalwrench May 23, 2018

bengourley left a comment

Enforce requiring API key to initialise notifier #280

Enforce requiring API key to initialise notifier #280

Conversation

fractalwrench commented May 18, 2018 • edited Loading

Goal

Changeset

Changed

Tests

Discussion

Review

snmaynard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snmaynard commented May 19, 2018

fractalwrench commented May 21, 2018

martin308 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fractalwrench commented May 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bengourley left a comment

Choose a reason for hiding this comment

$@fractalwrench$ fractalwrench commented May 18, 2018 •

edited

Loading