2:52 p.m.
Yes, this must be done inside the identity provider when Google sends the
token because attackers might intercept the request and change the hd
parameter.
Unfortunately the integration tests failed randomly on Travis without
showing any output. In my first run it were TESTS=server-group4 and
TESTS=old and in the second TESTS=server-group1, TESTS=server-group2
and TESTS=server-group4.
Could this be related to my changes?
Best,
Steffen
Stian Thorgersen <sthorger(a)redhat.com> schrieb am Do., 7. Juni 2018, 21:20:
On 7 June 2018 at 10:11, Steffen Kreutz <s.kreutz(a)yieldlab.de> wrote:
> I will remove the unit tests and mocking stuff from the PR and create
> integration tests.
>
Great, thanks
>
> Would it be sufficient to test if logging in succeeds for an account with
> a valid domain and accordingly fails for an account with an invalid domain?
> Or should I somehow test if the token contains the hd param.
>
Your code already checks if the hd param in the token matches the
requested hd though doesn't it? So testing valid/invalid domain seems
sufficient to me.
>
>
> > Am 06.06.2018 um 16:17 schrieb Marek Posolda <mposolda(a)redhat.com>:
> >
> > +1 for investigate. I hope the contributor can help us with clarifying
> > it and write the test?
> >
> > Maybe the test for this will need to be in separate class, so it's
> > possible to test it separately from the other Google IDP stuff, which
> > can be tested with free account. And the other contributors to Google
> > IDP, which don't have corporate account, won't be blocked.
> >
> > But I suppose that for our CI, we can test with "redhat.com" domain
> account?
> >
> > Marek
> >
> >
> > On 06/06/18 15:22, Stian Thorgersen wrote:
> >> I think we're in agreement here that the ideal is to do proper
> >> integration tests. Rather than open the doors for mocks (java based or
> >> http based) we should attempt to do it proper. For Google I'm not sure
> >> how realistic that is as the hd param probably requires an corporate
> >> account rather than a freebie account that we are using today for
> >> testing. We should investigate that first though.
> >>
> >> On 6 June 2018 at 10:03, Hynek Mlnarik <hmlnarik(a)redhat.com
> >> <mailto:hmlnarik@redhat.com>> wrote:
> >>
> >> To answer the original question whether to mock or not to mock, I
> >> wouldn't
> >> object allowing those in unit tests (which we have not that many
> >> though
> >> until now there's no need to them and I hope we can preserve this
> >> state).
> >> I'd rather not introduce those in integration tests because of
> >> maintenance
> >> costs.
> >>
> >> The biggest issue I see with mocks is that one needs to be extra
> >> careful to
> >> return exactly what's expected from the specification/counterparty.
> >> Since social
> >> providers can change without prior notice, our mocked IdPs might
> >> give us
> >> false impression that we're testing them. Furthermore, such mocks
> >> would be
> >> rather complex, and would require non-trivial maintenance. I'm
> >> hence biased
> >> against mock IdPs and prefer the tests with real IdPs as mentioned
> >> by Marek.
> >>
> >>
> >> On Mon, Jun 4, 2018 at 11:22 AM, Marek Posolda
> >> <mposolda(a)redhat.com <mailto:mposolda@redhat.com>> wrote:
> >>
> >>> Dne 1.6.2018 v 14:48 Bill Burke napsal(a):
> >>>> On Fri, Jun 1, 2018 at 5:18 AM, Marek Posolda
> >> <mposolda(a)redhat.com <mailto:mposolda@redhat.com>>
> >>> wrote:
> >>>>> For IDP, I am not sure how would Mocked IDP help? We already
> >> have lots
> >>> of
> >>>>> test coverage for Identity brokering for OIDC and SAML. The
> >> social
> >>> providers
> >>>>> usually implements OAuth/OIDC and there are just small
> >> differences
> >>> between
> >>>>> them. But those small differences and the fact that social
> >> providers
> >>> are out
> >>>>> of our control, is exactly the reason why we need to test
> >> with real
> >>>>> providers instead of mocks IMO.
> >>>>>
> >>>> IDP mocks are not a replacement for real testing. Just
> >> something that
> >>>> would run in public CI, that way at least there's something to
> >> catch
> >>>> issues like when/if brokering gets refactored again.
> >>> AFAIK the longer-term plan is to run social tests on every PR,
> >> to catch
> >>> regressions by testing with real social providers. So maybe it's
> >> better
> >>> to rather wait until we have that rather then introduce mocks now?
> >>>
> >>> Also anyone from community should be able to run social tests
> >> based on
> >>> our README. The important is, that someone could run it just for
> >> single
> >>> provider, which he is interested in (EG. Google for this PR) and
> >> is not
> >>> required to create the account+application in all social providers,
> >>> which we support. But I hope the social tests already support
> >> this now.
> >>>
> >>> Marek
> >>>>
> >>>> Bill
> >>>
> >>>
> >>> _______________________________________________
> >>> keycloak-dev mailing list
> >>> keycloak-dev(a)lists.jboss.org
<mailto:keycloak-dev@lists.jboss.org>
> >>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
> >> <https://lists.jboss.org/mailman/listinfo/keycloak-dev>
> >>>
> >>
> >>
> >>
> >> --
> >>
> >> --Hynek
> >> _______________________________________________
> >> keycloak-dev mailing list
> >> keycloak-dev(a)lists.jboss.org <mailto:keycloak-dev@lists.jboss.org>
> >> https://lists.jboss.org/mailman/listinfo/keycloak-dev
> >> <https://lists.jboss.org/mailman/listinfo/keycloak-dev>
> >>
> >>
> >
> > _______________________________________________
> > keycloak-dev mailing list
> > keycloak-dev(a)lists.jboss.org
> > https://lists.jboss.org/mailman/listinfo/keycloak-dev
>
>
> _______________________________________________
> keycloak-dev mailing list
> keycloak-dev(a)lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>