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 a reg-lock verification action to the new console #2467

Merged
merged 1 commit into from
Jul 8, 2024

Conversation

gbrodman
Copy link
Collaborator

@gbrodman gbrodman commented Jun 3, 2024

The front end will have a (hidden) page that passes the verification
code to this API endpoint and displays the result.


This change is Reviewable

@gbrodman
Copy link
Collaborator Author

friendly ping

Copy link
Collaborator

@ptkach ptkach left a comment

Choose a reason for hiding this comment

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

Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gbrodman)


core/src/main/java/google/registry/ui/server/console/ConsoleRegistryLockVerifyAction.java line 55 at r1 (raw file):

    domainLockUtils.verifyVerificationCode(lockVerificationCode, user.getUserRoles().isAdmin());
    // TODO: send to a custom console landing page, but for now just redirect back to the console
    String url = String.format("%s/%s", consoleApiParams.request().getServerName(), "console");

First of all servername is not needed as console ui is hosted on the same server, so relative url is more preferable.
Apart from that I suggest not to use an url to direct console, but better keep simple JSON contract and allow front-end to do the thing and navigate where it needs upon receiving the response, this way API doesn't need to know about paths on front-end

Copy link
Collaborator

@ptkach ptkach left a comment

Choose a reason for hiding this comment

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

sorry I totally forgot to click publish on my comment

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gbrodman)

Copy link
Collaborator Author

@gbrodman gbrodman left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ptkach)


core/src/main/java/google/registry/ui/server/console/ConsoleRegistryLockVerifyAction.java line 55 at r1 (raw file):

Previously, ptkach (Pavlo Tkach) wrote…

First of all servername is not needed as console ui is hosted on the same server, so relative url is more preferable.
Apart from that I suggest not to use an url to direct console, but better keep simple JSON contract and allow front-end to do the thing and navigate where it needs upon receiving the response, this way API doesn't need to know about paths on front-end

This action is not called via a JSON request; is not an API. This action is triggered by the user clicking a link in an email, meaning that they're just opening up a fresh link in a new tab.

@gbrodman gbrodman force-pushed the regLockVerify branch 3 times, most recently from 62587e4 to 349a885 Compare June 26, 2024 20:36
Copy link
Collaborator Author

@gbrodman gbrodman left a comment

Choose a reason for hiding this comment

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

Reviewable status: 5 of 9 files reviewed, 1 unresolved discussion (waiting on @ptkach)


core/src/main/java/google/registry/ui/server/console/ConsoleRegistryLockVerifyAction.java line 55 at r1 (raw file):

Previously, gbrodman wrote…

This action is not called via a JSON request; is not an API. This action is triggered by the user clicking a link in an email, meaning that they're just opening up a fresh link in a new tab.

Ehhhh, I think I get what you were saying and I think this new way (what I'm pretty sure you were suggesting) is better. The front end will have a page that the email will link to, and that page will call this JSON API.

Copy link
Collaborator Author

@gbrodman gbrodman left a comment

Choose a reason for hiding this comment

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

Reviewable status: 5 of 9 files reviewed, 1 unresolved discussion (waiting on @ptkach)


core/src/main/java/google/registry/ui/server/console/ConsoleRegistryLockVerifyAction.java line 55 at r1 (raw file):

Previously, gbrodman wrote…

Ehhhh, I think I get what you were saying and I think this new way (what I'm pretty sure you were suggesting) is better. The front end will have a page that the email will link to, and that page will call this JSON API.

to be more clear, I think what I have now implemented is what you were suggesting.

Copy link
Collaborator

@ptkach ptkach left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gbrodman and @jianglai)


core/src/main/java/google/registry/ui/server/console/ConsoleRegistryLockAction.java line 159 at r2 (raw file):

              .setScheme("https")
              .setHost(consoleApiParams.request().getServerName())
              .setPath("/console/registry-lock-verify")

We use hash routing system in console ui, so most routes look like /console/#/{route_name}


core/src/main/java/google/registry/ui/server/console/ConsoleRegistryLockVerifyAction.java line 55 at r1 (raw file):

Previously, gbrodman wrote…

to be more clear, I think what I have now implemented is what you were suggesting.

That's exactly what I meant, sorry if I didn't make it clear. I believe this is better than sending a redirect

Copy link
Collaborator Author

@gbrodman gbrodman left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jianglai and @ptkach)


core/src/main/java/google/registry/ui/server/console/ConsoleRegistryLockAction.java line 159 at r2 (raw file):

Previously, ptkach (Pavlo Tkach) wrote…

We use hash routing system in console ui, so most routes look like /console/#/{route_name}

Done.

Copy link
Collaborator

@ptkach ptkach left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jianglai)

@gbrodman gbrodman enabled auto-merge July 8, 2024 19:28
The front end will have a (hidden) page that passes the verification
code to this API endpoint and displays the result.
@gbrodman gbrodman added this pull request to the merge queue Jul 8, 2024
Merged via the queue into google:master with commit b8a6ac7 Jul 8, 2024
8 of 9 checks passed
@gbrodman gbrodman deleted the regLockVerify branch July 8, 2024 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants