Add openid provider eid interface

swp-fu-eid / eid-fu-swp

Docker-based REST API implemented with Django and restframework.

MIT License

2 stars 1 forks source link

Add openid provider eid interface #67

Closed BenjaminKeller closed 6 years ago

BenjaminKeller commented 7 years ago

This pr is not ready yet and is just added for discussion and organization purpose.

BenjaminKeller commented 7 years ago

TODO:

[x] fix old tests @nielsgroth
[x] add new tests:
- [x] retrieve jwks and parse as JSON, filter kid @nielsgroth
- [x] add fake authentication to database for testing @BenjaminKeller
- [x] add failing test if the authentication is skipped and the refreshaddress is called manually
- [x] add positive testcases
[x] travis registerclient improvement @m273d15
[x] fix eid service with emtpy user (aka b") in another branch @BenjaminKeller
[x] create pr with local test execution @BenjaminKeller

BenjaminKeller commented 7 years ago

@nils-wisiol The tests are working for me on my computer. I think it's a problem with the environment variables but I don't get it. @m273d15 suggested that the problem is OIDC_RESPONSE_TYPE="id_token token". But I cannot reproduce it.

nils-wisiol commented 7 years ago

@BenjaminKeller could you paste your .env file here?

BenjaminKeller commented 7 years ago

@nils-wisiol Finally it works! Now, I can integrate my tests...

nils-wisiol commented 7 years ago

for nested e2e tests, see also

nils-wisiol commented 7 years ago

I finished my e2e tests for desec-stack. Any news on this PR?

BenjaminKeller commented 7 years ago

@nils-wisiol Finally, I managed to add the positive test cases. I could deactivate the consent in the openid provider plugin so we don't have to mess with the csrf stuff. The cookies are working fine now. I tried to follow your e2e tests but it's not completely suitable. Additionally, the authentication tests are in one file and should be split up maybe. As I don't have time anymore I will pass this branch to @m273d15 after rebasing.

@m273d15 I finished rebasing, please take over.

TODO:

[x] rebase branch (d2672f7 includes tests for example, squash d2672f7 and 2c67597, ...)
[x] use djangos built-in method to redirect to other views in all cases
[x] add negative tests for every stage
[x] refactor and reorganize test/e2e/spec/eid_oidc_provider/auth_spec if possible

nils-wisiol commented 6 years ago

I am not convinced that just deactivating the CSRF token does not pose a security thread. Wasn't it there for a reason in the first place? Please look into that. (Also, I am wondering why disable it just after we finished debugging the test case that does cover CSRF?)

Armagetron commented 6 years ago

I remember that there where some problems with Django and the CSRF token. Disabling helped there.

nils-wisiol commented 6 years ago

Disabling CSRF means disabling a security feature. In most cases, that's a bad idea. But it may be okay under circumstances. So, why is it okay in this context?

BenjaminKeller commented 6 years ago

Just a quick note before I fly away: I did not deactivate csrf. I just deactivated that the open id plugin asks the user for permission. Then, csrf is not required. As this is a client specific setting, future clients can activate this feature nevertheless. See registerclient.py for the option.

m273d15 commented 6 years ago

Please re-review this PR

BenjaminKeller commented 6 years ago

I take back the responsibility for this branch. It's a pity that it couldn't be finished in these two weeks.

auvin commented 6 years ago

Here is what we discussed on the monday meeting:

Create a new pull request with a meaningful description in the first post
Squash the commits so that every commit is one feature (just as always)

After that @MrM0nkey or @zervnet will review the new branch.

BenjaminKeller commented 6 years ago

See #78.