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

feat(sessions): implementing permission revoking #699

Merged
merged 4 commits into from
Jul 12, 2024

Conversation

geekbrother
Copy link
Contributor

@geekbrother geekbrother commented Jul 8, 2024

Description

This PR implements the permission context revoking endpoint /v1/sessions/{address}/revoke according to the API SPEC draft.

For the request authentication, the signature (signed message) is used.
As a signing message, the permission controller unique identifier is used (PCI) and signed by the signing key (created during the session creation request).
Then the signature is verified at the server by the verification key which is stored during the session creation in the permission session object.

How Has This Been Tested?

Due Diligence

  • Breaking change
  • Requires a documentation update
  • Requires a e2e/integration test update

@geekbrother geekbrother self-assigned this Jul 8, 2024
@geekbrother geekbrother force-pushed the feat/revoking_session branch from aaf5cc8 to 066d1ce Compare July 8, 2024 17:36
@geekbrother geekbrother marked this pull request as ready for review July 8, 2024 17:47
.hget(address.clone(), request_payload.pci.clone())
.await?
.ok_or_else(|| RpcError::PermissionNotFound(request_payload.pci.clone()))?;
state.metrics.add_irn_latency(irn_call_start, "hget".into());
Copy link
Member

@chris13524 chris13524 Jul 10, 2024

Choose a reason for hiding this comment

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

Would be good to make this "hget" thing an enum (converting it to a string inside add_irn_latency())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, we should get rid of magic string. Fixed in 4cf2436 by implementing OperationType enum.

)?;

// Remove the session/permission item from the IRN
let irn_call_start = SystemTime::now();
Copy link
Member

Choose a reason for hiding this comment

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

We should be using Instant::now() and instant.elapsed() instead here instead of SystemTime as it is panic/Result-free and possibly a little more accurate/less overhead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! This change should be the followup since we are using this everywhere in blockchain-api.

Copy link
Member

@chris13524 chris13524 left a comment

Choose a reason for hiding this comment

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

I think this needs to be idempotent

src/handlers/sessions/revoke.rs Outdated Show resolved Hide resolved
Base automatically changed from feat/update_sessions_context to master July 11, 2024 17:03
@geekbrother geekbrother force-pushed the feat/revoking_session branch from 066d1ce to b06a332 Compare July 12, 2024 12:34
@geekbrother geekbrother requested a review from chris13524 July 12, 2024 13:14
state.metrics.add_irn_latency(irn_call_start, "hget".into());
state
.metrics
.add_irn_latency(irn_call_start, OperationType::Hget.into());
Copy link
Member

Choose a reason for hiding this comment

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

Good to change to enum but I would go a step further and convert the enum into a string inside add_irn_latency()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 3ddc187. Thanks!

Copy link
Member

@chris13524 chris13524 left a comment

Choose a reason for hiding this comment

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

I think the revoke endpoint should be idempotent

@geekbrother
Copy link
Contributor Author

I think the revoke endpoint should be idempotent

I missed that we need to check the request signature later, we need to get the verifying key for that. If the PCI not exists we can't verify/authorize the request.

@chris13524
Copy link
Member

I missed that we need to check the request signature later, we need to get the verifying key for that. If the PCI not exists we can't verify/authorize the request.

You only need to verify it if you are going to perform an action. If the action is already performed then you are good.

@geekbrother
Copy link
Contributor Author

I missed that we need to check the request signature later, we need to get the verifying key for that. If the PCI not exists we can't verify/authorize the request.

You only need to verify it if you are going to perform an action. If the action is already performed then you are good.

Changed it. We are returning OK and warn on it now.

@geekbrother geekbrother merged commit b502eb6 into master Jul 12, 2024
12 checks passed
@geekbrother geekbrother deleted the feat/revoking_session branch July 12, 2024 20:47
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