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

Fix password enforcement #8745

Merged
merged 3 commits into from
Mar 29, 2023
Merged

Fix password enforcement #8745

merged 3 commits into from
Mar 29, 2023

Conversation

kulmann
Copy link
Contributor

@kulmann kulmann commented Mar 29, 2023

Description

Fix some quirks of the public link password enforcement.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

kulmann added 3 commits March 29, 2023 21:54
Hard realization: the passwordEnforced capability member names are
misleading. They don't map to permissions, but to the existing roles.
That is most obvious with the "read_write" password enforcement,
which needs "read" and "create", but doesn't accept "update" and
"delete". Not accepting "update" is not reasonable on a "read_write".
onError(e)
console.error(e)
this.showMessage({
title: this.$gettext('Failed to create link'),
status: 'danger'
})
})
return
Copy link
Contributor Author

Choose a reason for hiding this comment

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

prevents a fall through to the success message when the request resulted in an error status code. previously an error would cause both the error and the success message. :trollface:

})
return
Copy link
Contributor Author

Choose a reason for hiding this comment

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

prevents a fall through to the success message when the request resulted in an error status code. previously an error would cause both the error and the success message. :trollface:


/**
* `passwordEnforced` members are oddly designed. they look like they map to permissions,
* but in reality they map to role names. hence the comparison with specific link roles.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wanted to add that I feel betrayed by the capability. It looks so promising regarding permissions... but it's a big nope in reality.

@kulmann kulmann requested review from JammingBen and dschmidt and removed request for JammingBen March 29, 2023 20:29
@kulmann kulmann self-assigned this Mar 29, 2023
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

42.1% 42.1% Coverage
0.0% 0.0% Duplication

Copy link
Member

@dschmidt dschmidt left a comment

Choose a reason for hiding this comment

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

Don't know much about any of this, but looks good code wise

@kulmann kulmann merged commit 2fe56a1 into master Mar 29, 2023
@delete-merged-branch delete-merged-branch bot deleted the fix-password-enforcement branch March 29, 2023 22:07
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