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

Add return value to WiFi.setCredentials() #1247

Merged
merged 2 commits into from Mar 21, 2017

Conversation

@sergeuz
Copy link
Member

commented Feb 5, 2017

Currently, WiFi.setCredentials() may fail silently depending on specified security settings, it also doesn't provide any mechanism for error reporting, which may be confusing for users. This PR makes WiFi.setCredentials() return true if credentials has been stored successfully, or false otherwise.

I also noticed that the API allows to use NULL as a password argument, however, the implementation lacks necessary null pointer checks.

TODO: Update firmware reference.


Doneness:

  • Contributor has signed CLA
  • Problem and Solution clearly stated
  • Code peer reviewed
  • API tests compiled
  • Run unit/integration/application tests on device
  • Add documentation
  • Add to CHANGELOG.md after merging (add links to docs and issues)

Enhancements

  • [PR #1247] Adds error checking to WiFi.setCredentials(), will return true if credentials has been stored successfully, or false otherwise.

Bug fixes

  • [PR #1247] Previously no null pointer checks on password argument of WiFi.setCredentials().
@technobly

This comment has been minimized.

Copy link
Member

commented Feb 9, 2017

Since we will have a return type now, should we enforce 8-63 character passwords for WPA? I know from testing in the past that any password < 8 for WPA will not be saved by WICED. I'm not sure how this differs for WEP though.

@technobly technobly added this to the 0.7.0 milestone Feb 9, 2017

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Feb 9, 2017

I'll make some tests to see if WICED returns an error in this case, and if it does, then the error should be propagated to WiFi.setCredentials() already.

@sergeuz sergeuz added the bug label Feb 19, 2017

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2017

Added "bug" label since the PR also fixes potential null pointer access.

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Feb 23, 2017

@technobly You're right, since we're only storing WiFi credentials to DCT rather than trying to connect to a specified AP directly, we're bypassing WICED's preliminary checks for password validity and thus WiFi.setCredentials() returns true for passwords that don't meet requirements for a specified security type.

I've added password length checks for WPA/WPA2 and WEP (Photon-only).

@technobly technobly merged commit abc63d8 into develop Mar 21, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@technobly technobly deleted the fix/wifi_set_cred branch Mar 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.