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.

Sign up for GitHub

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

Jump to bottom

better "return to app" HTML page #182

Closed
ghost opened this issue Dec 13, 2019 · 13 comments · Fixed by #401
Closed

better "return to app" HTML page #182

ghost opened this issue Dec 13, 2019 · 13 comments · Fixed by #401
Assignees
@johankool
Labels
macOS

Comments

@ghost
Copy link

ghost commented Dec 13, 2019

This one got lost? eduvpn/macos#153
@johankool
Copy link
Collaborator

My PR is still open: openid/AppAuth-iOS#413 😞

@ghost ghost added the macOS label Dec 16, 2019
@johankool johankool self-assigned this May 22, 2020
@roop
Copy link
Collaborator

roop commented Mar 26, 2021 •
edited
Loading

I see that @johankool pointed out earlier that this is better done with a page hosted in eduvpn.org and/or letsconnect-vpn.org, and @fkooman preferred that it be handled locally.

The local web server is probably not the right place for it because once the flow ends, the web server is dismantled. The OAuth library doesn't provide an API to provide the HTML, or to redirect to the local web server. It does provide an API to redirect to a URL that we host.

Given that OAuth hasn't been interested in merging Johan's PR, I think our options are:

  • Fork the OAuth library, modify it and use that (and keep it up to date), or
  • Host a page and redirect to that URL

@fkooman @efef What do you think?

@ghost
Copy link
Author

ghost commented Mar 26, 2021

It does provide an API to redirect to a URL that we host.

This really makes no sense. It is a really bad design to add a dependency on a central web server just because the API is so shitty. So no, we won't be doing that. Maybe you can investigate why they are not interested in the PR, maybe they don't like the way you do it? Another option would be just to improve the included HTML to look a bit better, so no need to provide our own, just improve it for everyone.

If it is not possible without patching the OAuth client I'd just say just leave it as-is and close this issue.

@johankool
Copy link
Collaborator

My guess is that their check on unit tests complained that this isn't unit tested, or at least caused the percentage of covered code to drop, albeit minimally.

@roop
Copy link
Collaborator

roop commented Apr 23, 2021

Another option would be just to improve the included HTML to look a bit better, so no need to provide our own, just improve it for everyone.

@fkooman This is a great idea and I want to try it. Can I base the HTML template on the HTML/CSS from https://github.com/Amebis/eduOAuth/tree/master/eduOAuth/Resources/Html ? (Just making sure there isn't any issue giving away our design / html to OAuth or something like that.)

@roop
Copy link
Collaborator

roop commented Apr 26, 2021

@efef Do you have any thoughts on whether I can take some HTML/CSS code from https://github.com/Amebis/eduOAuth/tree/master/eduOAuth/Resources/Html and contribute that to the AppAuth library?

@ghost
Copy link
Author

ghost commented Apr 26, 2021

Or you can also create a new one that fits better in Apple systems, e.g. use the correct fonts, style for Apple. Also dark mode would be nice :) If you need help with that let me know!

@roop
Copy link
Collaborator

roop commented Apr 26, 2021

I'm not great with web design and stuff, so if you can help me with the page HTML/CSS, that would be great.

@ghost
Copy link
Author

ghost commented Apr 26, 2021

Okay, on it! :-)

@ghost
Copy link
Author

ghost commented Apr 26, 2021 

<!DOCTYPE html>

<html lang="en" dir="ltr" xmlns="http://www.w3.org/1999/xhtml">
<head>
    <meta charset="utf-8" />
    <title>The client succesfully authorized.</title>
    <style>
body {
    font-family: -apple-system, BlinkMacSystemFont, "Avenir Next", Avenir, "Nimbus Sans L", Roboto, Noto, "Segoe UI", Arial, Helvetica, "Helvetica Neue", sans-serif;
    margin: 0;
    height: 100vh;
    display: flex;
    align-items: center;
    justify-content: center;
    background: #ccc;
    color: #888;
}

main {
    padding: 1em 2em;
    text-align: center;
    border: 1pt solid #666;
    box-shadow: rgba(0, 0, 0, 0.2) 0px 1px 4px;
    border-color: #aaa;
    background: #ddd;
}
    </style>
</head>
<body class="finished">
    <main>
        <h2>The client succesfully authorized.</h2>
        <p>You can now close this tab.</p>
    </main>
</body>
</html>

@ghost
Copy link
Author

ghost commented Apr 26, 2021

This might be a first good step, very simple, no crap. If this is acceptable for upstream, perhaps we can also look into dark-mode and the error pages that should also be made.

@roop
Copy link
Collaborator

roop commented Apr 29, 2021

Thanks. I've filed a PR on AppAuth with this: openid/AppAuth-iOS#633 . It also uses the same HTML template for error cases.

@roop
Copy link
Collaborator

roop commented Apr 30, 2021

The AppAuth PR gave me some ideas to fix this in our codebase itself by overriding the AppAuth code.

@roop roop mentioned this issue Apr 30, 2021
Merged
@roop roop closed this as completed in #401 May 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@johankool johankool

Labels
macOS
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

macOS: Improve HTML page shown after OAuth authentication completes roop/eduvpn-apple
2 participants
@roop @johankool