-
Notifications
You must be signed in to change notification settings - Fork 41
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
Pairing: allow re-registering devices even if limit is reached #940
base: master
Are you sure you want to change the base?
Pairing: allow re-registering devices even if limit is reached #940
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #940 +/- ##
==========================================
+ Coverage 68.13% 68.17% +0.04%
==========================================
Files 281 281
Lines 7421 7435 +14
==========================================
+ Hits 5056 5069 +13
- Misses 2365 2366 +1 ☔ View full report in Codecov by Sentry. |
@@ -223,7 +223,28 @@ defmodule Astarte.Pairing.Engine do | |||
end | |||
end | |||
|
|||
defp verify_can_register_device(realm_name) do | |||
defp verify_can_register_device(realm_name, device_id) do | |||
# An already existsing device should always be able to retrieve a new credentials secret |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
@@ -117,6 +117,34 @@ defmodule Astarte.Pairing.Queries do | |||
end | |||
end | |||
|
|||
def verify_already_registered_device(realm_name, device_id) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion it would be more correct, since it returns {:ok, boolean}
, to change the function to verify_already_registered_device?(...)
.
Also, the function is a bit too specific... might it be worth changing the pattern/logic to simply use select_device_by_id(realm_name, device_id)
and manage externally whether it is present/registered in the db or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On function name: we usually follow the check_*
convention for {:ok, bool}
results (see e.g. check_device_exists/2
in RealmManagement), good catch!
On function logic: code that is already in place uses the pattern of selecting only relevant information (see e.g. select_device_for_credentials_request/2
and select_device_for_info/2
), I have followed the same pattern. Your suggestion would be interesting when refactoring Pairing!
Also, note that a function that performs the same query does already exist (check_already_registered_device/2
), however it uses CQEx and therefore it needs a CQEx client rather than a simple realm name to run. I decided to keep engine.ex
code simple and duplicate the functionality in queries.ex
- not only for that, but also because we will soon™ remove CQEx entirely. Also, I had to change that check_yadayada
function to verify_yadayada
, as it does not return an {:ok, bool}
but :ok/{:error/reason}
The device registration limit set for an Astarte realm is meant to prevent registration of new devices when the limit is reached. However, it blocked re-registration of already existing devices. Allow to re-register existing devices without checking if registration limit has been reached. See [astarte-platform#933](astarte-platform#933). Signed-off-by: Arnaldo Cesco <arnaldo.cesco@secomind.com>
b369db6
to
394adc5
Compare
The device registration limit set for an Astarte realm is meant to prevent registration of new devices when the limit is reached. However, it blocked re-registration of already existing devices.
Allow to re-register existing devices without checking if registration limit has been reached.
Closes #933.