Allow user to store the current url in the session through options #25

AlexandreRoba · 2024-01-20T18:37:13Z

Some IDPs only support predefined sets of returned URL. We need a way to define a single returnUrl configured on the IDP and then from this page be able to find out what was the address of the local url that was used to initiate the login flow.

This PR allows the user to ask the agent through the options to store the current url in the session for later retrieval once the authentication flow has completed and the user returns to the site.

…e IDP

AlexandreRoba · 2024-01-20T19:17:02Z

I've modified the API to make more user friendly. I do not know what is the best way to expose a method to the agent.

Ideally I would like to configure the callback as such in the login options:

let login_options = LoginOptions::new()
        .with_redirect_url(Url::parse("http://localhost:3000/callback").unwrap())
        .with_store_current_url_in_session();
    html!(
        <>
            <OAuth2 {config}
                scopes={vec!["openid".into(),"email".into(),"offline_access".into(),"api:call".into()]}
                audience={"http://localhost:8081/api"}
                options={login_options}>
                <Content/>
            </OAuth2>
        </>
    )

Then use it on the call back from the agent as such:

#[function_component]
pub fn CallbackPage() -> Html {
    let agent = use_auth_agent();
    if let Some(agent) = agent {
        match agent.get_stored_current_url() { //!!! How to expose a method to the agent?
            Some(url) => html!(<h1>{format!("Should redirect the user here :{:?}",url)}</h1>),
            None => html!(<h1>{format!("No stored url in the session")}</h1>),
        }
    } else {
        html!(<h1>{"No agent configured in the context."}</h1>)
    }
}

And this would be the method to expose the stored current URL:

    fn get_stored_current_url(&self) -> Option<Url> {
        SessionStorage::get::<String>(STORAGE_KEY_CURRENT_URL)
            .and_then(|item| {
                Url::parse(item.as_str())
                    .map_err(|_| StorageError::KeyNotFound(STORAGE_KEY_CURRENT_URL.to_string()))
            })
            .ok()
    }

AlexandreRoba · 2024-01-21T07:58:01Z

Hi @ctron, I finally managed to have the full flow working with Auth0 and the pull request submitted here. This is the implementation of the callback page on my app that I have implemented in order to leverage the url stored in the session when the IDP redirect me after successful login:

#[function_component]
pub fn CallbackPage() -> Html {
    let _ = use_auth_agent().expect("This page must have an Oauth2 Context");

    let local_storage: UseSessionStorageHandle<String> =
        use_session_storage("ctron/oauth2/currentUrl".to_string());

    let router = use_router::<AppRoute>().unwrap();

    let current_url = (*local_storage).clone();
    let target = match current_url {
        Some(current_url) => {
            let url = reqwest::Url::parse(&current_url)
                .expect("The value store in the session 'currentUrl' is not a valid url");
            let segments = url
                .path_segments()
                .expect("The Url path cannot be segmented")
                .collect::<Vec<&str>>();
            AppRoute::parse_path(segments.as_slice())
                .expect("Could not build the target out of the path segments")
        }
        None => AppRoute::Index,
    };

    router.push(target);

    html!(<p>{"Something went wrong"}</p>)
}

The part that I do not like is the reading of the session storage. This should be hidden in the agent somehow (using the get_stored_current_url but I do not know how to expose this in the agent.

Let me know what you think.

kate-shine

I'm sorry for intruding with review despite not being a member of project myself, but I also started working on this feature, so just adding some ideas I collected during that :)

Ultimately, it would be nice if handling of the redirect was also abstracted by yew-oauth2 - already working on it, so when your part gets through, I can finish and PR the rest of the code.

kate-shine · 2024-01-21T18:36:48Z

src/agent/state.rs

@@ -1,6 +1,7 @@
 pub const STORAGE_KEY_CSRF_TOKEN: &str = "ctron/oauth2/csrfToken";
 pub const STORAGE_KEY_LOGIN_STATE: &str = "ctron/oauth2/loginState";
 pub const STORAGE_KEY_REDIRECT_URL: &str = "ctron/oauth2/redirectUrl";
+pub const STORAGE_KEY_CURRENT_URL: &str = "ctron/oauth2/currentUrl";


I'd consider this key name misleading, because at the moment when user actually reads it, it's already not the "Current url"

When I was playing with my own implementation, I considered something like STORAGE_KEY_POST_LOGIN_REDIRECT_URL or STORAGE_KEY_ORIGINAL_URL

Hi @kate-shine , I can rename it. It is not exposed (should not once we have a way to retrieve it via the client api or the handling of the local redirection by the component itself) to the outside world and I named it after the method I get it from self::get_current_url().

I guess we should probably name the all local redirection as a process and have a way to 'activate' it as an extra local action when being redirected from the Idp.

I would also prefer a name like "post login redirect", as that makes is pretty clear what the intention is. Also, feel free (if you like) to change the prefix from ctron … seeing that now, I think I should have done that some time ago 🤣

Depends on how we want to see it? Do we want to name it as how we want to use it, or do we want to name it as what it is. It is the url of the ressource that initiated the flow but indeed we want to automatically redirect to it once the login is complete. Today the component does not do it automatically. So from a developer point of view it is the url the user tried to access before the authentication start and it is up to the dev to perform the Post login redirection.

I will rename it to post login url but for me this sound strange... Cause it gives the impression this post url is settable which is not. It is the original url. It is what we do with it that makes it a PostRedirectUrl. If you both agree on that. Let's do it :)

Indeed if we manage to have everything handle internally, postLoginUrl makes full sens. My bad. :)

kate-shine · 2024-01-21T18:39:08Z

src/agent/mod.rs

@@ -434,6 +444,16 @@ where
        let client = self.client.as_ref().ok_or(OAuth2Error::NotInitialized)?;
        let config = self.config.as_ref().ok_or(OAuth2Error::NotInitialized)?;

+        //If specified in the login options will store the current url in the session for retrieval
+        // when coming back from IDP
+        if let Some(options) = config.options.clone() {


I wonder if we actually need this to be configurable. It might be enough to just set this up in case the redirect_url differs from current_url

That's an interesting idea. There might be a drawback (which could be handled as well of course). What would happen if one time that URL differs and the next time it doesn't? In that case, the URL would still be set and evaluated when coming back.

The question is: does it hurt to always store the URL when starting, and then to handle this when it comes back? If (after the code exchange) it detects a different URL than the current, it could do the post-login redirect. Unless a flag is present which could be used to prevent that (if people don't like it).

My approach and idea when I started working on it was to set it only if it differs from redirect URL and then redirect to it if it's present when evaluating callback.

So if I summarize:

The user set a fixed redirectUrl (LoginOptions)

If the user has set a fixed redirectUrl then we store the currentUrl as postLoginUrl in the session

Once the process is finished, if there is a postProcessUrl in the session , we redirect to it instead of the returnURL

Sounds like a good scenario.

I'd reframe the second point'ts condition as

If the redirectUrl differs from the currentUrl
as that both means that redirect is needed and makes it obvious that fixed redirect url is set up

Isn't this handled by the check at the third point? "If there is a postProcessUrl, we redirect to it instead of the returnUrl"?

@kate-shine Nevermind :) Make full sens. I'm adapting my pull request.

ctron · 2024-01-22T07:25:51Z

I'm sorry for intruding with review despite not being a member of project myself, but I also started working on this feature, so just adding some ideas I collected during that :)

Never be sorry because of that. Every constructive feedback is more than welcome! I really appreciate both of your input and help on this!

ctron · 2024-01-22T07:27:57Z

src/agent/mod.rs

+    /// Keep the current url in the session
+    ///
+    ///Set this flag to true to store the current url for later retrieval in the session
+    pub keep_current_url_in_session: Option<bool>,


I am not sure this needs to be Option, it looks like a default (false) would just work the same way.

ctron · 2024-01-22T07:41:31Z

I think it goes in the right direction. I also think that it might make sense to have yew-oauth2 to handle this automatically during the post-login processing. With the option to opt-out of it. Exposing the post login redirect URL on an API feels a weird. I think it's ok doing this, but there should be a reasonable default (with some way to customize this).

I could imagine one or more of the following things:

An optional post login callback (something like Callback<…> which gets called after the login is complete)
A setting (boolean, enum) which allows one to customize the post-login action (Default, Nothing, …) where the default would be to check if a URL was recorded, and use the History API to navigate to that URL, in case it's different to the current one. Having the alternative to skip this (Nothing).

Having those two options should allow one to have a reasonable default, but also enough freedom to customize the behavior. And feel free to choose different names (am bad with naming things).

… for post login url

AlexandreRoba · 2024-01-22T11:31:13Z

So to summerize:

The user set a fixed redirectUrl (LoginOptions)
If the redirectUrl differs from the currentUrl we store the currentUrl as postLoginUrl in the session
Once the auth process is finished, if there is a postProcessUrl in the session , we redirect to it instead of the returnURL

The third part is missing. @kate-shine Can you do this part?

kate-shine · 2024-01-23T10:06:55Z

Yep, I will happily try to tackle that one :)

kate-shine · 2024-01-23T10:20:20Z

src/agent/mod.rs

@@ -434,6 +434,8 @@ where
        let client = self.client.as_ref().ok_or(OAuth2Error::NotInitialized)?;
        let config = self.config.as_ref().ok_or(OAuth2Error::NotInitialized)?;

+        let post_login_url = Self::current_url().map_err(OAuth2Error::StartLogin)?;
+
        // take the parameter value first, then the agent configured value, then fall back to the default
        let redirect_url = match options.redirect_url.or_else(|| {


You can now do a bit of a deduplication here, because match expression made sense mostly for lazy evaluation of current_url getter. (I'd personally also rename post_login_url to current_url to keep naming consistent with current context and for deduplication to make more sense if you go for it, but that's more a matter of taste :) )

let redirect_url = options.redirect_url.or_else(|| { config .options .as_ref() .and_then(|opts| opts.redirect_url.clone()) }).unwrap_or(post_login_url);

ctron · 2024-01-24T07:13:07Z

I'll really appreciate you both taking care of this. I won't have much time to review this until Friday. But I'll promise on Friday I will take a close look and hope to get it merged and released!

kate-shine · 2024-01-24T08:30:09Z

I'll really appreciate you both taking care of this. I won't have much time to review this until Friday. But I'll promise on Friday I will take a close look and hope to get it merged and released!

Sent you few question on Matrix, I hope you don't mind :)

ctron · 2024-01-25T08:12:22Z

@AlexandreRoba I think it's good to be merged, it just needs a rebase, sorry.

BREAKING-CHANGE: In order to allow adding new options in the future, the structs LoginOptions and LogoutOptions have been made non-exhaustive. Adding some documentation on how to work with them.

…ring-to-login-options

AlexandreRoba · 2024-01-25T10:34:03Z

@ctron Sorry I spend the last hour trying to "rebase" and can't seem to do it properly with a fork... Simply drop this merge request. Not sure it brings anything to the project. :(

kate-shine · 2024-01-25T14:21:16Z

@ctron Sorry I spend the last hour trying to "rebase" and can't seem to do it properly with a fork... Simply drop this merge request. Not sure it brings anything to the project. :(

It was super helpful! Don't worry, I will finish the rebase and finish it in my branch, your commits are already there :)

ctron · 2024-01-26T07:51:02Z

This PR was pulled in via #29 … many thanks to all of you!

Allow user to store the current url in the session before going to th…

e39290e

…e IDP

simplified the login options configuration

4ecb091

kate-shine reviewed Jan 21, 2024

View reviewed changes

ctron reviewed Jan 22, 2024

View reviewed changes

Removed unecessary parameters in LoginOptions and renamed storage key…

deda90b

… for post login url

kate-shine reviewed Jan 23, 2024

View reviewed changes

This was referenced Jan 24, 2024

feat: expose History to the public interface ctron/yew-nested-router#12

Merged

Handle post login redirect #28

Closed

ctron previously approved these changes Jan 25, 2024

View reviewed changes

ctron and others added 4 commits January 25, 2024 11:15

refactor: make some options non-exhaustive

9a23a1e

BREAKING-CHANGE: In order to allow adding new options in the future, the structs LoginOptions and LogoutOptions have been made non-exhaustive. Adding some documentation on how to work with them.

Resolved merged conflict

db518ce

Resolved conflict

f6c9899

Resolved conflict

8caabd8

AlexandreRoba dismissed ctron’s stale review via 8caabd8 January 25, 2024 10:18

AlexandreRoba added 4 commits January 25, 2024 11:25

Resolve conflict

b702993

Resolved conflict

90a31e6

Resolve conflict

429287e

Merge branch 'ctron:main' into dynamically-adding-self-url-as-queryst…

8830f20

…ring-to-login-options

kate-shine mentioned this pull request Jan 25, 2024

Save and handle post login redirect #29

Merged

ctron closed this Jan 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow user to store the current url in the session through options #25

Allow user to store the current url in the session through options #25

AlexandreRoba commented Jan 20, 2024

AlexandreRoba commented Jan 20, 2024

AlexandreRoba commented Jan 21, 2024

kate-shine left a comment

kate-shine Jan 21, 2024

AlexandreRoba Jan 22, 2024

AlexandreRoba Jan 22, 2024

ctron Jan 22, 2024

AlexandreRoba Jan 22, 2024

AlexandreRoba Jan 22, 2024

AlexandreRoba Jan 22, 2024

kate-shine Jan 21, 2024

ctron Jan 22, 2024

kate-shine Jan 22, 2024

AlexandreRoba Jan 22, 2024

kate-shine Jan 22, 2024

AlexandreRoba Jan 22, 2024 •

edited

Loading

AlexandreRoba Jan 22, 2024

ctron commented Jan 22, 2024

ctron Jan 22, 2024

ctron commented Jan 22, 2024

AlexandreRoba commented Jan 22, 2024

kate-shine commented Jan 23, 2024

kate-shine Jan 23, 2024

ctron commented Jan 24, 2024

kate-shine commented Jan 24, 2024

ctron commented Jan 25, 2024

AlexandreRoba commented Jan 25, 2024

kate-shine commented Jan 25, 2024

ctron commented Jan 26, 2024

Allow user to store the current url in the session through options #25

Allow user to store the current url in the session through options #25

Conversation

AlexandreRoba commented Jan 20, 2024

AlexandreRoba commented Jan 20, 2024

AlexandreRoba commented Jan 21, 2024

kate-shine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexandreRoba Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctron commented Jan 22, 2024

Choose a reason for hiding this comment

ctron commented Jan 22, 2024

AlexandreRoba commented Jan 22, 2024

kate-shine commented Jan 23, 2024

Choose a reason for hiding this comment

ctron commented Jan 24, 2024

kate-shine commented Jan 24, 2024

ctron commented Jan 25, 2024

AlexandreRoba commented Jan 25, 2024

kate-shine commented Jan 25, 2024

ctron commented Jan 26, 2024

AlexandreRoba Jan 22, 2024 •

edited

Loading