Skip to content

Conversation

@BarcoMasile
Copy link

Description

This PR builds on top of #659.

This adds the broker resource implementation for the e2e tests present.
It implements the login behavior in the vanilla/common/BrowserWindow.py file, with the device flow.

An important thing to note is that the python script for the vanilla broker does two additional things compared to the original one used as a template from the msentraid folder:

  • it adds the implementation (leveraging only the standard library) of the TOTP generation, so a new parameter is needed, which is the secret key for the TOTP
  • it adds an assertion at the end of the sign-in process, since there was no assertion initially, so the script didn't change exit value based on the actual success of the login flow: now it does and it communicates success/failure through 0/1 exit codes

denisonbarbosa and others added 7 commits September 4, 2025 08:53
These rely on YARF to test the authentication process of authd and
its brokers, along with checking if the user was properly setup in the
system.
This is a simple documentation that defines the requirements needed to
run the tests locally. After the tests are automated and are stabilized,
we should definitely focus on improving it.
Since YARF currently does not manage the VMs on its own, we need to
manually provision and execute the VM in order to run the tests. There
are also some other limitations (i.e. not being able to run tests from a
specific robot file) that are covered by this script.
@BarcoMasile BarcoMasile requested a review from a team as a code owner September 20, 2025 10:10
@BarcoMasile
Copy link
Author

As per offline agreement, I'm moving this to Draft
cc @denisonbarbosa @shipperizer

@BarcoMasile BarcoMasile marked this pull request as draft September 22, 2025 09:54
from gi.repository import Gdk, Gtk, GLib, WebKit2 as WebKit # type: ignore


class BrowserWindow(Gtk.Window):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class should be actually moved to a generic place and re-used in both msentraid and vanilla.

The only part that should change is in the test itself, so I'd try to just have a common class and override what we need in each implementation

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I'll coordinate with Denison for this


return token

def __get_hotp_token(self, secret, intervals_no):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can also depend and rely on python3-pyotp here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given some struggles to make the python part working (dependency-wise) I prefered to stick to a basic implementation that relies on the standard library only, so it would not add on the requirements

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we already have dependency on non-standard library already since we rely on python3-gi and some gir files, so I feel it's not a big deal one more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note there's also python3-onetimepass, not sure which one is maintained better, pick the one you like better

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've evaluated both those libraries when I went with this implementation
It is very simple as you can see, so IMO it would be good either way.
But I'll sync with Denis on this so we're aligned, thanks



def wait_for_successful_text(webview, selector="h1.p-heading--4", retries=10, delay=1.0):
js = f'''
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this can be part of the "generic" class and this can be another parameter

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it could be, even though from what I'm told this implementation is ok to keep implementation speicific (relatively to the OIDC broker target of this e2e test, which is the Canonical Identity Platform)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep implementation speicific (relatively to the OIDC broker target of this e2e test, which is the Canonical Identity Platform)

Yes, but the much code we can share the better, so I'd make sure that what lands in #659 is something generic enough to be overridden here, or you rebase and split it here.

Comment on lines +195 to +196
return result_holder["text"]

Copy link
Collaborator

@3v1n0 3v1n0 Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not something we should do, as it may break many things (github diff is messed, i meant):

while Gtk.events_pending():
       Gtk.main_iteration()

Instead the code should be looking something like:

    js_result = None
    loop = GLib.MainLoop()

    def on_js_finished(webview, result):
        nonlocal js_result

        try:
            js_result = webview.evaluate_javascript_finish(result)
        except Exception as e: # TODO: filter exception...
            print(f"{e}")
            # We want to raise it too?
            # raise e
        loop.quit()

    webview.evaluate_javascript(js, -1, None, None, None, on_js_finished, None)
    loop.run()

    if js_result and js_result.to_string():
        return js_result.to_string()

    wait_for_timeout(delay)

Using: #659 (comment)


return None


Copy link
Collaborator

@3v1n0 3v1n0 Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, no time.sleep in UI components :)

See also #659 (comment)

EDIT: github diff is messed up, but well the reference should be clear

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, I'm aware that this sleep stops the main thread, but apparently it is useful to have a super short delay between one try and the other, I couldn't actually explain myself why.
It's for "looking for" the sucess message.

I'll try to remove it, but being the peculiarity of the Gtk library behavior, I'm not sure it will not affect the code :-/

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you use the code above should be good enough, if not we've other problems... :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants