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

"Current Device" for Trusted Devices does not work #1285

Closed
aschempp opened this issue Feb 4, 2020 · 6 comments · Fixed by #1299
Closed

"Current Device" for Trusted Devices does not work #1285

aschempp opened this issue Feb 4, 2020 · 6 comments · Fixed by #1299
Labels
bug up for discussion Issues and PRs which will be discussed in our monthly Mumble calls.
Milestone

Comments

@aschempp
Copy link
Member

aschempp commented Feb 4, 2020

Affected version(s)
4.9

Description
The current implementation to detect the current trusted device will not work reliably. We are storing the trusted device cookie data, but the data will change for ever user that has logged in (all users are stored in the same cookie).

There are only two options:

  1. remove "current device" for trusted devices
  2. duplicate probably 80% of the 2FA Trusted Devices code of scheb/two-factor-bundle

😞

I'm not sure whats best… option 1 means we can't tell the user the current device – not so nice. Option 2 means we're getting rid of even more of the bundle, and I start wondering if we (should) need it at all. But then we need to re-invent the wheel all over again …

@aschempp aschempp added this to the 4.9 milestone Feb 4, 2020
@aschempp aschempp added the bug label Feb 4, 2020
@bytehead
Copy link
Member

bytehead commented Feb 4, 2020

  1. duplicate probably 80% of the 2FA Trusted Devices code of scheb/two-factor-bundle

I do have some working code laying around. But exactly with this issue, because you need to write your own DeviceTokenStorage.

@bytehead bytehead added the up for discussion Issues and PRs which will be discussed in our monthly Mumble calls. label Feb 4, 2020
@bytehead
Copy link
Member

bytehead commented Feb 4, 2020

To keep it simple as it gets I personally vote for option 1.

@leofeyer
Copy link
Member

leofeyer commented Feb 4, 2020

Can you point me to the problematic code please?

@aschempp
Copy link
Member Author

aschempp commented Feb 5, 2020

Can you point me to the problematic code please?

Not sure what you mean. https://github.com/contao/contao/blob/master/core-bundle/src/Security/TwoFactor/TrustedDeviceManager.php#L72 stores the current cookie value, but the cookie value can contain multiple trusted device tokens.

@leofeyer
Copy link
Member

leofeyer commented Feb 7, 2020

@bytehead has explained the issue to me (thank you again). Now that I understand it, I also vote for option 1, not showing "this device" in the device list.

@leofeyer leofeyer linked a pull request Feb 11, 2020 that will close this issue
@leofeyer
Copy link
Member

Closing this in favor of #1299.

leofeyer pushed a commit that referenced this issue Feb 14, 2020
Description
-----------

Here's a proposal how to fix #1285.

Commits
-------

fd2f57a Removes current device and fixes other issues related to trusted devices
e54cb33 Fix test
d011973 Fix trans files
dcbb9bc Newline?
0ee808e Remove version
fea9783 Cleanup
4712935 Simplify
a5e2f77 Remove skeleton config
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug up for discussion Issues and PRs which will be discussed in our monthly Mumble calls.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants