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.

Sign up for GitHub

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

Jump to bottom

Explicitly allow access on some (public) routes also without 2FA #29752

Merged
merged 1 commit into from
Nov 18, 2021
Merged

Explicitly allow access on some (public) routes also without 2FA #29752

merged 1 commit into from
Nov 18, 2021

Conversation

ChristophWurst
Copy link
Member

Fixes nextcloud/twofactor_totp#1147 and many similar bugs in 2FA apps.

This is kind of a revert of #28725, or another revision. The problem is that we have some public routes (@PublicPage) that should be accessible during the 2FA setup. That is after the login (user context exists) but before completing any 2FA challenge or setting up a 2FA provider (other routes have to remain blocked).

This new annotation will allow us to mark (public) routes to be accessible again.

This follows @nickvergessen's suggestion from #29056 (comment)

We will have to add this annotation to the 2FA provider routes.

@ChristophWurst
Explicitly allow some routes without 2FA
c8caba2 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst ChristophWurst added the 2. developing Work in progress label Nov 17, 2021
@ChristophWurst ChristophWurst self-assigned this Nov 17, 2021
@ChristophWurst ChristophWurst changed the title Explicitly allow some routes without 2FA Explicitly allow access on some (public) routes also without 2FA Nov 17, 2021
nickvergessen
nickvergessen approved these changes Nov 17, 2021
View reviewed changes
Copy link
Member

@nickvergessen nickvergessen 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's the best approach, but we should document properly that this is only supposed to be used by twofactor apps?

@ChristophWurst ChristophWurst added the pending documentation This pull request needs an associated documentation update label Nov 17, 2021
@ChristophWurst
Copy link
Member Author

I think it's the best approach

In an ideal world we would be able to load @PublicPage without user context until 2FA is disabled. Then enabling all public pages again wouldn't be dangerous. But I have no idea how we could achieve that. At least not easily.

@nickvergessen
Copy link
Member

In an ideal world we would be able to load @publicpage without user context until 2FA is disabled. Then enabling all public pages again wouldn't be dangerous. But I have no idea how we could achieve that. At least not easily.

Well not sure I agree here. Without the context how should the 2fa apps know it?
But anyway let's go with the approach above for now

@ChristophWurst ChristophWurst marked this pull request as ready for review November 18, 2021 09:06
@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 18, 2021
@ChristophWurst ChristophWurst requested review from a team, skjnldsv, CarlSchwan and rullzer and removed request for a team November 18, 2021 09:07
@ChristophWurst ChristophWurst added this to the Nextcloud 23 milestone Nov 18, 2021
@nickvergessen nickvergessen merged commit 2df7ea7 into master Nov 18, 2021
@nickvergessen nickvergessen deleted the fix/allow-some-pages-without-two-factor branch November 18, 2021 09:43
@ChristophWurst
Copy link
Member Author

/backport to stable23

@ChristophWurst
Copy link
Member Author

/backport to stable22

@ChristophWurst
Copy link
Member Author

/backport to stable21

@ChristophWurst
Copy link
Member Author

/backport to stable20

This was referenced Nov 18, 2021
Closed
Closed
@ChristophWurst ChristophWurst mentioned this pull request Nov 18, 2021
Closed
@backportbot-nextcloud
Copy link

The backport to stable23 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable21 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable20 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable22 failed. Please do this backport manually.

This was referenced Nov 18, 2021
Merged
Merged
Merged
Merged
@Ivansss Ivansss mentioned this pull request Nov 18, 2021
Closed
@ghost ghost mentioned this pull request Nov 22, 2021
Open
@mojansch mojansch mentioned this pull request Nov 23, 2021
Closed
@ChristophWurst ChristophWurst mentioned this pull request Nov 29, 2021
Closed
@dodedodo dodedodo mentioned this pull request Jan 17, 2022
Closed
@ChristophWurst ChristophWurst mentioned this pull request Nov 14, 2022
Merged
@ChristophWurst ChristophWurst removed the pending documentation This pull request needs an associated documentation update label Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CarlSchwan CarlSchwan CarlSchwan approved these changes

@nickvergessen nickvergessen nickvergessen approved these changes

@LukasReschke LukasReschke Awaiting requested review from LukasReschke

@blizzz blizzz Awaiting requested review from blizzz

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

@rullzer rullzer Awaiting requested review from rullzer

Assignees

@ChristophWurst ChristophWurst

Labels
3. to review Waiting for reviews
Projects
None yet
Milestone
Nextcloud 23
Development

Successfully merging this pull request may close these issues.

Invalid QR code on Edge during first login of a user
4 participants
@ChristophWurst @nickvergessen @CarlSchwan @skjnldsv