Skip to content

Conversation

@DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Nov 23, 2023

…d also used authenticated

Description

Some controllers define methods which are publicly accessible and accessible for authenticated users the same time.
e.g. https://github.com/owncloud/files_texteditor/blob/3b00b6ea0b4d89bc91c5fdd68d459337f079399f/controller/filehandlingcontroller.php#L105

In such situations the 2fa handling was bypassed because of the @PublicPage annotation.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@DeepDiver1975 DeepDiver1975 marked this pull request as draft November 23, 2023 16:41
@DeepDiver1975 DeepDiver1975 force-pushed the fix/2fa-hybrid-controller-methods branch from f3f78a1 to 3437599 Compare November 24, 2023 10:47
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@DeepDiver1975 DeepDiver1975 marked this pull request as ready for review November 24, 2023 11:39
@DeepDiver1975 DeepDiver1975 force-pushed the fix/2fa-hybrid-controller-methods branch from 3437599 to f0c1de7 Compare November 24, 2023 13:10
@DeepDiver1975
Copy link
Member Author

@phil-davis no idea if we want to implement an acceptance test for this ....

@DeepDiver1975
Copy link
Member Author

failing drone is unrelated - docker images not found ..... as usual ....

@jvillafanez
Copy link
Member

We might need to retest all the 2fa flows to ensure everything works. I've quickly checked with 2fa enforcement (without encryption) and it seems to work well.

@DeepDiver1975
Copy link
Member Author

The pure 2fa flow is untouched as this is "only" about the controller annotations. I wonder if this needs to be added to webdav as well which does not got through the controller layer .....

@phil-davis
Copy link
Contributor

LGTM - files_texteditor and twofactor_totp work together correctly.

@DeepDiver1975 DeepDiver1975 merged commit 30c4bb4 into master Dec 1, 2023
@delete-merged-branch delete-merged-branch bot deleted the fix/2fa-hybrid-controller-methods branch December 1, 2023 10:01
pako81 pushed a commit to owncloud/docs that referenced this pull request Dec 13, 2023
DeepDiver1975 added a commit that referenced this pull request Dec 13, 2023
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.

4 participants