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

feature(locksmith): Update access control for checkoutConfigs #14833

Merged
merged 24 commits into from
Oct 30, 2024

Conversation

0xTxbi
Copy link
Member

@0xTxbi 0xTxbi commented Oct 10, 2024

Description

This PR introduces changes to transition the access control mechanism for checkout configurations from relying on the createdBy field to using lock managers.

Previously, a user could only list, or edit a checkout config if they were its creator. With this update, lock managers are now granted the ability to manage configurations for locks they control, even if they are not the original creators.

Key changes include:

  • Allowing Lock Managers to save and manage configurations tied to locks they manage, regardless of who originally created the configuration.
  • Listing configurations for users if they are a lock manager on at least one associated lock, even if they did not create the configuration.
  • Refactoring the backend by separating logic into operations and controllers, improving organization.

Issues

Fixes #14248
Refs #14248

Checklist:

  • 1 PR, 1 purpose: my Pull Request applies to a single purpose
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the docs to reflect my changes if applicable
  • I have added tests (and stories for frontend components) that prove my fix is effective or that my feature works
  • I have performed a self-review of my own code
  • If my code involves visual changes, I am adding applicable screenshots to this thread

@0xTxbi 0xTxbi requested a review from julien51 October 10, 2024 15:15
@cla-bot cla-bot bot added the cla-signed label Oct 10, 2024
Copy link
Member

@julien51 julien51 left a comment

Choose a reason for hiding this comment

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

Let's talk about the way to list... because I don't think we can do that.

@0xTxbi
Copy link
Member Author

0xTxbi commented Oct 10, 2024

Let's talk about the way to list... because I don't think we can do that.

sure thing!

@0xTxbi 0xTxbi requested a review from julien51 October 25, 2024 18:47
Copy link
Member

@julien51 julien51 left a comment

Choose a reason for hiding this comment

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

More cleanup and changes...

if (existingConfig) {
const authorized = await isUserAuthorized(user, existingConfig)
if (!authorized) {
throw new Error('User not authorized to update this configuration')
Copy link
Member

Choose a reason for hiding this comment

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

If this throws then we need to handle it specifically at the controller level to return a 403

Copy link
Member

@julien51 julien51 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 it would be nice to add tests here please

@0xTxbi 0xTxbi requested a review from julien51 October 29, 2024 12:32
@julien51 julien51 merged commit 6b95559 into unlock-protocol:master Oct 30, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Let lock managers edit checkout URL
2 participants