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

remote_server: Add management command for deactivation. #20775

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

eeshangarg
Copy link
Member

@eeshangarg eeshangarg commented Jan 13, 2022

Testing plan:

  • The mobile_push_service management command was tested manually and then the outcome was verified by manually checking the database via python3 manage.py shell.
  • The rest of the changes have accompanying unit tests.

@eeshangarg
Copy link
Member Author

CodeQL was complaining about mobile_push_service.py outputting the SECRETS_FILENAME and was labeling it as "logging potentially secret data". I marked it as a false positive since we are only just logging the path to the file and not any actual secrets.

@eeshangarg eeshangarg marked this pull request as ready for review January 14, 2022 03:51
@eeshangarg
Copy link
Member Author

@timabbott This PR is now ready for review! Thanks! :)

@timabbott
Copy link
Sponsor Member

I merged the first commit as 7a1ed9a.

zilencer/urls.py Outdated
path("remotes/server/register", register_remote_server),
path("remotes/server/deactivate", deactivate_remote_server),
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should use the rest_path authentication, just like sending a push notification -- the zulip_org_id / zulig_org_key pair is authentication.

content_dict = response.json()
raise CommandError("Error: " + content_dict["msg"])

return response
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you do this error-handling refactoring in preparatory commits? Renaming this tool is a sensitive operation that will need to be merged carefully, since we don't want to make our documentation confusing, so we want only changes necessary to the renaming operation in that commit.

)

if response.json()["result"] == "success":
print("Mobile Push Notification Service registration successfully deactivated!")
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should display some sort of error if this doesn't succeed, right?

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Ideally, just the error response returned by the server).

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or if _request_push_notification_bouncer_url ensures it can't have failed, we should have an assert rather than an if statement.


@staticmethod
def msg_format() -> str:
return _("This remote server has been deactivated")
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"The mobile push notification service registration for your server has been deactivated".

@eeshangarg eeshangarg force-pushed the remote-server branch 2 times, most recently from 8aba811 to 091e822 Compare January 21, 2022 01:10
@eeshangarg
Copy link
Member Author

@timabbott I just addressed your feedback. Thanks!

event_type=RealmAuditLog.REMOTE_SERVER_DEACTIVATED
).last()
assert remote_realm_audit_log is not None
self.assertTrue(server.deactivated)
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this change, after reordering commits so that the "Fail API requests if deactivated" commit is before this one.

diff --git a/zerver/tests/test_push_notifications.py b/zerver/tests/test_push_notifications.py
index f4dd9c34cd..07dfc02b6c 100644
--- a/zerver/tests/test_push_notifications.py
+++ b/zerver/tests/test_push_notifications.py
@@ -2514,6 +2514,16 @@ class PushBouncerSignupTest(ZulipTestCase):
         assert remote_realm_audit_log is not None
         self.assertTrue(server.deactivated)
 
+        # Now test that trying to deactivate again reports the right error.
+        result = self.uuid_post(
+            zulip_org_id, "/api/v1/remotes/server/deactivate", request, subdomain=""
+        )
+        self.assert_json_error(
+            result,
+            "The mobile push notification service registration for your server has been deactivated",
+            status_code=401,
+        )
+
     def test_push_signup_invalid_host(self) -> None:
         zulip_org_id = str(uuid.uuid4())
         zulip_org_key = get_random_string(64)

This commit replaces the management command `register_server` with
a `mobile_push_service` command with `--register` and `--rotate-key`
as subcommands. This allow us to add more subcommands in the future.
@timabbott
Copy link
Sponsor Member

I merged the series ending with 93329c2 after the small changes noted above and adding a small commit of my own. Thanks for making this reviewable @eeshangarg!

The last couple commits remaining on this PR look reasonable, but we need to think carefully about the name for the script as well as how we want to document the change in name. I'll start a chat.zulip.org conversation.

@zulipbot
Copy link
Member

Heads up @eeshangarg, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

@timabbott timabbott added the completion candidate PRs with reviews that may unblock merging label Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
completion candidate PRs with reviews that may unblock merging has conflicts size: XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants