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

Edge: Implement AuthenticationListener handling for BasicAuth #1571

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

sratz
Copy link
Member

@sratz sratz commented Nov 4, 2024

Differences to IE engine:

  • Location attribute:
    • IE does not provide the 'location' for the callback, so this has to be guessed from lastNavigateURL which does not work consistently, e.g. after an immediate Browser.setUrl();
    • Edge does provide the 'location' information consistently
  • When returning invalid credentials
    • IE does not call the authentication handler again for further attempts
    • Edge calls the authentication handler again and again. Keeping track of this is out-of-scope for SWT. There is no retryCount field or similar in the event. AuthenticationHandler implementations need to account for unbounded repetitive calls in case the server does not bail out.

Resolves #730.

Differences to IE engine:
- Location attribute:
  - IE does not provide the 'location' for the callback, so this has to
    be guessed from lastNavigateURL which does not work consistently,
    e.g. after an immediate Browser.setUrl();
  - Edge does provide the 'location' information consistently
- When returning invalid credentials
  - IE does not call the authentication handler again for further
    attempts
  - Edge calls the authentication handler again and again.
    Keeping track of this is out-of-scope for SWT.
    There is no retryCount field or similar in the event.
    AuthenticationHandler implementations need to account for unbounded
    repetitive calls in case the server does not bail out.

Resolves #730.
@sratz
Copy link
Member Author

sratz commented Nov 4, 2024

Can be manually tested, e.g. using https://httpbin.org:

import org.eclipse.swt.SWT;
import org.eclipse.swt.browser.Browser;
import org.eclipse.swt.layout.FillLayout;
import org.eclipse.swt.widgets.Display;
import org.eclipse.swt.widgets.Shell;

public class EdgeAuth {

	public static void main(String[] args) {
		Display display = new Display();
		Shell shell = new Shell(display);
		shell.setLayout(new FillLayout());
		Browser browser = new Browser(shell, SWT.EDGE);
		browser.addAuthenticationListener(event -> {
			System.out.println(event.location);

			// testable scenarios

			{
				// 1: fallback to browser dialog
			}

			{
				// 2: failure (causing repeated calls)
				event.user = "foo";
				event.password = "WRONG";
			}

			{
				// 3: correct credentials
				event.user = "foo";
				event.password = "bar";
			}

			{
				// 4: cancel authentication
				event.doit = false;
			}

		});
		browser.setUrl("https://httpbin.org/basic-auth/foo/bar");
		shell.open();
		while (!shell.isDisposed()) {
			if (!display.readAndDispatch())
				display.sleep();
		}
		display.dispose();
	}

}

@sratz sratz added the edge Edge Browser label Nov 4, 2024
@sratz sratz added this to the 4.34 M3 milestone Nov 4, 2024
@sratz sratz requested a review from HeikoKlare November 4, 2024 14:26
Copy link
Contributor

github-actions bot commented Nov 4, 2024

Test Results

   483 files  ±0     483 suites  ±0   9m 10s ⏱️ + 1m 19s
 4 095 tests ±0   4 085 ✅ ±0   7 💤 ±0  3 ❌ ±0 
16 173 runs  ±0  16 080 ✅ ±0  90 💤 ±0  3 ❌ ±0 

For more details on these failures, see this check.

Results for commit 4858107. ± Comparison against base commit ce2795a.

Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

The code looks good and works as expected. Differences to IE sound okay or even beneficial.

Implementing the commented out code in ICoreWebView2BasicAuthenticationRequestedEventArg sounds reasonable, but from my side would not be necessary if it is currently not used anymore. Feel free to decide whether you want to change/add that. My approval holds no matter whether that change is made.

@sratz
Copy link
Member Author

sratz commented Nov 5, 2024

The code looks good and works as expected. Differences to IE sound okay or even beneficial.

Thanks for reviewing. Agreed, the Edge behavior feels better / more intuitive.

Implementing the commented out code in ICoreWebView2BasicAuthenticationRequestedEventArg sounds reasonable, but from my side would not be necessary if it is currently not used anymore. Feel free to decide whether you want to change/add that. My approval holds no matter whether that change is made.

I added the method implementation so the interface definition is complete.

@sratz sratz merged commit d59358c into master Nov 5, 2024
8 of 15 checks passed
@sratz
Copy link
Member Author

sratz commented Nov 5, 2024

Test failures on Linux are unrelated.

@sratz sratz deleted the edge-authhandler branch November 5, 2024 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
edge Edge Browser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable AuthenticationListener for Browser-Widget under Windows/Edge
3 participants