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

Add OTP support #131

Merged
merged 7 commits into from
Jan 18, 2024
Merged

Add OTP support #131

merged 7 commits into from
Jan 18, 2024

Conversation

C0ntroller
Copy link
Member

Pull Request

Description

This PR will add OTP support to TUfast where the secret is stored, and the values automatically filled on login.

Although this makes 2FA kinda useless, the fact that 2FA will be forced, and that this feature was requested multiple times means it should be added.
Also, currently the really critical logins (HISQIS/jExam/Selma) are not even secured by 2FA.

References

Closes #127

Type of change

  • Bug fix (non-breaking change which fixes a bug)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that might cause existing functionality to not work as expected)

Further info

  • This change requires a documentation update
  • I updated the documentation accordingly, if required
  • I commented my code where its useful

Testing

We have 1500+ Users. Please test your changes thoroughly.

  • Tested my changes on Firefox
  • Tested my changes on Chromium-Based-Browsers
  • Successfully run npm run test locally

Additional Information

When testing this, please be careful when creating new tokens!! Always save them in another location!

@C0ntroller C0ntroller added enhancement New feature or request testing wanted Part of this pull request requires further testing labels Jan 14, 2024
@C0ntroller C0ntroller requested a review from a team as a code owner January 14, 2024 14:18
@C0ntroller C0ntroller requested a review from Noxdor January 14, 2024 14:18
@C0ntroller C0ntroller changed the base branch from main to develop January 14, 2024 14:54
@OliEfr
Copy link
Member

OliEfr commented Jan 14, 2024

Thank you very much - I will get it to test it on Chrome.

@Nix-da
Copy link

Nix-da commented Jan 18, 2024

Hi, I testet it in Chrome (Version 120.0.6099.224) and the new 2FA auto login worked.
I testet it on my Windows 11 Laptop and my Windows 10 PC and succsessfully logged 3 times on all of those plattforms: Opal, Self Service Portal, OpalExam, OpalExam2 and OpalExam3.

Furtheremore I reviewed the code changes a bit and everything looks fine to me. It reuses many functions of the username-password auto login, which already proofed to work.

I just have some notes regarding its usability:

  • On OpalExam 1-3 the automatic selection of "TU Dresden" is not working. I did not test if this is a problem of this branch or if this does not work on the current release aswell.
  • There really needs to be a hint somewhere on how to save the token in TUFast. I expected to find another input field below username and password in the settings. I only found out that you have to generate a new token while reviewing the code. Just adding a small text panel below the selma-password input field for this would help a lot.

@Nix-da
Copy link

Nix-da commented Jan 18, 2024

Also - when adding the Add On to chrome - this throws an error in contentScripts/login/opalHome.js:32 (clickLogin):

Refused to run the JavaScript URL because it violates the following Content Security Policy directive: "script-src 'self' 'wasm-unsafe-eval' 'inline-speculation-rules' http://localhost:* http://127.0.0.1:*". Either the 'unsafe-inline' keyword, a hash ('sha256-...'), or a nonce ('nonce-...') is required to enable inline execution. Note that hashes do not apply to event handlers, style attributes and javascript: navigations unless the 'unsafe-hashes' keyword is present.

I am not sure what that even means, but i does not seem to have impact on functionality.

@C0ntroller
Copy link
Member Author

On OpalExam 1-3 the automatic selection of "TU Dresden" is not working. I did not test if this is a problem of this branch or if this does not work on the current release aswell.

We do not inject any scripts into OpalExam for obvious reasons. So works as intended.

Also - when adding the Add On to chrome - this throws an error in contentScripts/login/opalHome.js:32 (clickLogin):

Refused to run the JavaScript URL because it violates the following Content Security Policy directive: "script-src 'self' 'wasm-unsafe-eval' 'inline-speculation-rules' http://localhost:* http://127.0.0.1:*". Either the 'unsafe-inline' keyword, a hash ('sha256-...'), or a nonce ('nonce-...') is required to enable inline execution. Note that hashes do not apply to event handlers, style attributes and javascript: navigations unless the 'unsafe-hashes' keyword is present.

I am not sure what that even means, but i does not seem to have impact on functionality.

Nice catch, would've never noticed that. As you said, it has no impact on functionality in this case.
I do know what that means (the CSP is a security mechanism to specify sources for any resources) but As I honestly don't even know why the error occurs in this line, I would say this is a problem for later.

There really needs to be a hint somewhere on how to save the token in TUFast. I expected to find another input field below username and password in the settings. I only found out that you have to generate a new token while reviewing the code. Just adding a small text panel below the selma-password input field for this would help a lot.

Yes this will need to be done, thanks for pointing that out. Probably it will just be another "tab" in the login window, as layout shifts currently mean more styling issues ^^

@OliEfr
Copy link
Member

OliEfr commented Jan 18, 2024

Could and should the UI-Change (i.e. additional input field) be done in another PR, or should I leave this one open?

@C0ntroller
Copy link
Member Author

Could and should the UI-Change (i.e. additional input field) be done in another PR, or should I leave this one open?

Will try to add it today.

@C0ntroller
Copy link
Member Author

Nope, seems to be a bit harder as the currently established Username-Password input fields are really hard to extend.
This is more a work for @Noxdor or future me when I ever try to kill snowpack and make the settings mobile friendly ^^'

But I don't think thats too bad. Most users use TOTPs (I think? Personal experience...) and don't know where to find their secret. So saying to just generate a new token currently is the easiest way for anyone...

So @OliEfr if you want to merge do it.

@OliEfr OliEfr merged commit 755db8b into develop Jan 18, 2024
1 check passed
OliEfr added a commit that referenced this pull request Jan 23, 2024
* fix _execute_action for ff

* Fix npm commands (#132)

Thanks @A-K-O-R-A for noticing this bug and reporting it! Have a good day.

* Feature/add hover texts (#133)

* Add hover texts for the popup icons

* Extend Informatik discord hover text

* Correct spelling

* Add OTP support (#131)

* Add first classes for otp saving and generation

* Typo

* OTP saving complete

* Better OTP saving

* OTP filling

* Add recovery codes to prompt

* Eslint

---------

Co-authored-by: Daniel Kluge <daniel-git@c0ntroller.de>
Co-authored-by: Maf <65976562+A-K-O-R-A@users.noreply.github.com>
Co-authored-by: Daniel <43993811+C0ntroller@users.noreply.github.com>
@OliEfr OliEfr deleted the feature/otp branch October 2, 2024 15:09
@OliEfr OliEfr restored the feature/otp branch October 2, 2024 15:09
@OliEfr OliEfr deleted the feature/otp branch October 2, 2024 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request testing wanted Part of this pull request requires further testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG]: 2-Factor-Authentification not working
3 participants