eID-Service Integration #57

BenjaminKeller · 2017-08-22T10:13:49Z

This pull request implements the required functionality of the interfaces of the eID-Service.

It contains the interface to the eID-Server so the SAML-procedure has only to modify the value of restrictedId in the ServiceRequest table. @Armagetron Please give some feedback about that.

Moreover it provides the view getUserId for the openId Provider. The refreshAddress provides a json containing the access token:

{"accessToken": "9465901b-6693-462b-9044-db3a80fe0cd0"}

getUserId provides the restricted id as base64:

{"userId": "b''"}

@nielsgroth @m273d15 @larissazech Please give some feedback.

See also #44 and #46.

nils-wisiol

As a general point, please increase code documentation throughout your PR.

nils-wisiol · 2017-08-22T10:50:52Z

test/e2e/spec/eid_service/main_spec.js

+        });
+    });
+
+    it("is working fine", function(done) {


The description here should be a little more descriptive. I advise to split this test case into several expectation handlers to make it more readable:

frisby.get(....getTcTokenUrl....) .expect('status', 302) .expect('tcToken retrievable')

where the new tcToken retrievable expectation handlers implements most of the actual testing.

With #55 in mind, this should be compatible with chakram by extending chai.

nils-wisiol · 2017-08-22T10:52:01Z

test/e2e/spec/eid_service/main_spec.js

@@ -0,0 +1,39 @@
+describe("eID-Service: The general flow", function() {


This suit just contains one positive test case. Please also add some negative checks to increase the test coverage to the cases where we need to reply faulty requests in the eid service / eid server.

nils-wisiol · 2017-08-22T10:56:00Z

It may be worthwhile to add a README to the eid server Django app to summarize the interface. Alternatively, this could be entirely put in code docs.

BenjaminKeller · 2017-08-22T14:11:20Z

You're right. Doc is now required as the complexity is high. I put it in code.

Update: I added documentation.

TODO:

Rewrite tests according to new framework
Add negative tests
Adapt interface to OpenId-Provider (see Implement interface between OpenID Provider and eID-Service #62)

auvin · 2017-08-22T15:29:58Z

Currently the openid provider expects a POST with username, client_id, redirect_uri, response_type, scope and state. The username is used to present the restricted userID, so i think we can than use the provided getUserId for that.
The other attributes are present as soon as the user clicks 'login with eid' and should be forwarded from that point on.

@larissazech, @m273d15 Please correct me, if i am wrong.

Edit: See #62 for a description more precisely.

BenjaminKeller · 2017-08-28T15:39:30Z

Chakram seems to be broken, too?
@nils-wisiol Done.

nils-wisiol · 2017-08-29T07:22:22Z

@BenjaminKeller

Chakram seems to be broken, too?

looks good to me, why do you think it is broken?

BenjaminKeller · 2017-08-29T09:54:03Z

@nils-wisiol The link is not working. Have a look at build #266 in travis. the tc token generator returns a tc token is the only test that fails but it affects 6 other tests, too.

Rebasing done.

nils-wisiol · 2017-08-29T11:03:09Z

In the travis build, tests fail as regex do not match. The Location that was sent,

https://eid.local/api/openid/login?eid_access_token=5cd623f4-9cd4-46a4-b49c-ea330fe8fb11&test=bla&foo=bar

does indeed not match the given expression

/\/api\/openid\/login?eid_access_token=.*[&]test=bla[&]foo=bar$/

Try escaping ? and _:

/\/api\/openid\/login\?eid\_access\_token=.*[&]test=bla[&]foo=bar$/

Also useful.

BenjaminKeller · 2017-08-29T11:31:49Z

Hi @nils-wisiol your described issue was problem of the test The refresh page redirects correctly with GET parameters. And obviously it is now working and I have fixed that a day ago. The other tests (eID-Service: The tcTokenUrl generator redirects by default and so on) are in a completely different file with a completely different definition. The regex in this file is also completely different from the displayed. And they fail because the the other test fails. That's the problem.

Nevertheless, this branch can be merged.

nils-wisiol · 2017-08-29T12:16:46Z

I'd blame that problem on the double-definition (here and here) of the correctRedirect property.

Chakram (using chai) runs all tests parallel, so you get a race condition and your expectation was overwritten.

I didn't reproduce this locally, but unless there is indication that this theory could be wrong, I'd rather spare the work.

nils-wisiol · 2017-08-29T07:26:10Z

api/eid_service/migrations/0001_initial.py

@@ -1,5 +1,5 @@
 # -*- coding: utf-8 -*-


This extension of the database model should be in a second file.

nils-wisiol · 2017-08-29T07:27:57Z

api/eid_server/serviceInterface.py

@@ -0,0 +1,24 @@
+from eid_server.models import ServiceRequest


according to PEP8, python modules (i.e. source files) should have all lower-case names

BenjaminKeller · 2017-08-29T12:19:25Z

This shouldn't be a problem as every suite has its own before call.

Do you have any further suggestions or can we merge?

nils-wisiol · 2017-08-29T12:21:36Z

The double definition is put onto a singleton object, that's why the old one is overwritten. (That's how require.js works)

BenjaminKeller · 2017-08-29T16:05:41Z

@nils-wisiol Files renamed, database migrated and tests improved. Do you check line feeds, formatting and style conventions manually or do you have some cool tool?

nils-wisiol

This lgtm! Nice work.

I found one more typo, see code annotations. One more general thing, using 'correct' as test description is a tautology: your tests always check if the system behaves correctly. So, if you want to further improve this PR, rename the tests to something avoiding the word 'correct'. How about adding a method rather than a property that can be used like expect(...).to.redirect_to(CONST_REGEX)? Anyways, this is good enough for merging (but please fix the typo real quick -- the approval will stay).

nils-wisiol · 2017-08-29T16:28:34Z

api/eid_service/views.py


 def error(request):
-    return HttpResponse("An error occurred during the authentication.")
+    """
+    The use agent is redirected to this view if the authentication session between eID-Client


"user agent"

nils-wisiol · 2017-08-29T17:25:07Z

I do not use a tool to check style conventions on reviews (unless I have a suspicion), but I recommend pycharm's integrated style checker. For line endings, I find GNU/Linux' file command helpful. I used it for checking line endings in a bunch of files at once.

BenjaminKeller · 2017-08-29T20:49:50Z

@nils-wisiol Thank you very much for your answer. I fixed the typo. I will use a constant redirect test function for testing next time!

nils-wisiol · 2017-08-29T21:06:12Z

Alrighty. Looking forward to see this in master.

…

On August 29, 2017 10:49:52 PM GMT+02:00, BenjaminKeller ***@***.***> wrote: @nils-wisiol Thank you very much for your answer. I fixed the typo. I will use a constant redirect test function for testing next time! -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #57 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

BenjaminKeller self-assigned this Aug 22, 2017

BenjaminKeller requested review from nils-wisiol, m273d15, Armagetron, auvin and larissazech August 22, 2017 10:13

BenjaminKeller added this to the Setup working OpenID Connect with data from the mock eID framework milestone Aug 22, 2017

nils-wisiol requested changes Aug 22, 2017

View reviewed changes

nils-wisiol mentioned this pull request Aug 22, 2017

Changed testing framework to chakram #56

Merged

BenjaminKeller force-pushed the eidservice_integration branch from 6a6b30c to 03cceaa Compare August 22, 2017 14:49

BenjaminKeller force-pushed the eidservice_integration branch 5 times, most recently from 969518c to 2954bfb Compare August 28, 2017 15:31

BenjaminKeller force-pushed the eidservice_integration branch from 2954bfb to 7d98b4d Compare August 28, 2017 15:47

BenjaminKeller force-pushed the eidservice_integration branch from 7d98b4d to d40a8c2 Compare August 29, 2017 09:58

nils-wisiol requested changes Aug 29, 2017

View reviewed changes

added interface between eID-Service and eID-Server

fc592a8

BenjaminKeller force-pushed the eidservice_integration branch 3 times, most recently from 1f00fe5 to bd4f61b Compare August 29, 2017 14:10

nils-wisiol approved these changes Aug 29, 2017

View reviewed changes

BenjaminKeller added 2 commits August 29, 2017 22:47

added interface between eID-Service and OpenId-Provider

bdac39e

adapted client checker to pass GET parameter through

95e9117

BenjaminKeller force-pushed the eidservice_integration branch from bd4f61b to 95e9117 Compare August 29, 2017 20:48

larissazech merged commit 4278974 into master Aug 30, 2017

BenjaminKeller deleted the eidservice_integration branch August 31, 2017 19:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eID-Service Integration #57

eID-Service Integration #57

BenjaminKeller commented Aug 22, 2017 •

edited

Loading

nils-wisiol left a comment

nils-wisiol Aug 22, 2017

nils-wisiol Aug 22, 2017

nils-wisiol commented Aug 22, 2017 •

edited

Loading

BenjaminKeller commented Aug 22, 2017 •

edited

Loading

auvin commented Aug 22, 2017 •

edited

Loading

BenjaminKeller commented Aug 28, 2017 •

edited

Loading

nils-wisiol commented Aug 29, 2017 •

edited

Loading

BenjaminKeller commented Aug 29, 2017 •

edited

Loading

nils-wisiol commented Aug 29, 2017

BenjaminKeller commented Aug 29, 2017

nils-wisiol commented Aug 29, 2017

nils-wisiol Aug 29, 2017

nils-wisiol Aug 29, 2017

BenjaminKeller commented Aug 29, 2017 •

edited

Loading

nils-wisiol commented Aug 29, 2017 •

edited

Loading

BenjaminKeller commented Aug 29, 2017

nils-wisiol left a comment •

edited

Loading

nils-wisiol Aug 29, 2017

nils-wisiol commented Aug 29, 2017

BenjaminKeller commented Aug 29, 2017

nils-wisiol commented Aug 29, 2017 via email

		@@ -0,0 +1,39 @@
		describe("eID-Service: The general flow", function() {

		@@ -0,0 +1,24 @@
		from eid_server.models import ServiceRequest

eID-Service Integration #57

eID-Service Integration #57

Conversation

BenjaminKeller commented Aug 22, 2017 • edited Loading

nils-wisiol left a comment

Choose a reason for hiding this comment

nils-wisiol Aug 22, 2017

Choose a reason for hiding this comment

nils-wisiol Aug 22, 2017

Choose a reason for hiding this comment

nils-wisiol commented Aug 22, 2017 • edited Loading

BenjaminKeller commented Aug 22, 2017 • edited Loading

auvin commented Aug 22, 2017 • edited Loading

BenjaminKeller commented Aug 28, 2017 • edited Loading

nils-wisiol commented Aug 29, 2017 • edited Loading

BenjaminKeller commented Aug 29, 2017 • edited Loading

nils-wisiol commented Aug 29, 2017

BenjaminKeller commented Aug 29, 2017

nils-wisiol commented Aug 29, 2017

nils-wisiol Aug 29, 2017

Choose a reason for hiding this comment

nils-wisiol Aug 29, 2017

Choose a reason for hiding this comment

BenjaminKeller commented Aug 29, 2017 • edited Loading

nils-wisiol commented Aug 29, 2017 • edited Loading

BenjaminKeller commented Aug 29, 2017

nils-wisiol left a comment • edited Loading

Choose a reason for hiding this comment

nils-wisiol Aug 29, 2017

Choose a reason for hiding this comment

nils-wisiol commented Aug 29, 2017

BenjaminKeller commented Aug 29, 2017

nils-wisiol commented Aug 29, 2017 via email

BenjaminKeller commented Aug 22, 2017 •

edited

Loading

nils-wisiol commented Aug 22, 2017 •

edited

Loading

BenjaminKeller commented Aug 22, 2017 •

edited

Loading

auvin commented Aug 22, 2017 •

edited

Loading

BenjaminKeller commented Aug 28, 2017 •

edited

Loading

nils-wisiol commented Aug 29, 2017 •

edited

Loading

BenjaminKeller commented Aug 29, 2017 •

edited

Loading

BenjaminKeller commented Aug 29, 2017 •

edited

Loading

nils-wisiol commented Aug 29, 2017 •

edited

Loading

nils-wisiol left a comment •

edited

Loading