Skip to content

Feature/pro 33#23

Merged
ctucker3 merged 6 commits intodevelopfrom
feature/PRO-33
Jun 16, 2020
Merged

Feature/pro 33#23
ctucker3 merged 6 commits intodevelopfrom
feature/PRO-33

Conversation

@ctucker3
Copy link
Contributor

@ctucker3 ctucker3 commented Jun 15, 2020

Implements changes to check the cookies on successful login and looks for a cookie named redirect-login. If the cookie is present, and has a valid url as its value, the user is redirected to that url on successful login. If the cookie is not present or is not a valid url, the user is redirected to the default url set in the config.

Should be reviewed with PRO-33 on bi-web.

@ctucker3 ctucker3 marked this pull request as ready for review June 15, 2020 19:29
@nickpalladino
Copy link
Member

PRO-33 - Reauth User

@eawoods
Copy link

eawoods commented Jun 15, 2020

I got both bi-api and bi-web running. Works like a charm! One question: I can't seem to get the redirect-login cookie to set regardless of the page I've visited. When should I be expecting to see that cookie?
I get only phylo-token on any page (Chrome v. 83.0.4, MacOS 10.14.6).

However, when I delete that token and try to navigate to another page, it does the right thing beautifully: takes me to the home page, gives the needs-login dialog, and redirects to login.

@ctucker3
Copy link
Contributor Author

I got both bi-api and bi-web running. Works like a charm! One question: I can't seem to get the redirect-login cookie to set regardless of the page I've visited. When should I be expecting to see that cookie?
I get only phylo-token on any page (Chrome v. 83.0.4, MacOS 10.14.6).

The redirect-login cookie will be set when you are not logged in and try to visit a page. You should be redirected to the home page immediately. There you will see the redirect-login cookie. The redirect-login cookie is removed each time you navigate to a page so that it doesn't hang about. So, you will only see it on the home page after a redirect.

Is the redirect flow for the navigating to a protected page from an external link working for you?

@ctucker3 ctucker3 requested a review from timparsons June 16, 2020 12:09
Copy link
Member

@nickpalladino nickpalladino left a comment

Choose a reason for hiding this comment

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

Test cases look to provide good coverage, a few minor comments


@Inject
private UserService userService;
private String loginSuccessUrlCookieName = "redirect-login";
Copy link
Member

Choose a reason for hiding this comment

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

Do we want the cookie name in the config file? You could then inject it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed and pushed

MutableHttpResponse mutableHttpResponse = HttpResponse.seeOther(location);

Cookie cookie;
for(Iterator var4 = cookies.iterator(); var4.hasNext(); mutableHttpResponse = mutableHttpResponse.cookie(cookie)) {
Copy link
Member

Choose a reason for hiding this comment

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

Curious about these var4 and var6 variable names, maybe could be something a little more informative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just copied and pasted from the micronaut loginSuccessCookies method. Not a problem to change them for our use though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed and pushed

if (isValidURL(returnUrl)){
locationUrl = returnUrl;
}
} catch (UnsupportedEncodingException e){}
Copy link
Member

Choose a reason for hiding this comment

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

May be useful to have a log entry instead of silent exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed and pushed

returnUrl = URLDecoder.decode(returnUrl, StandardCharsets.UTF_8.name());
if (isValidURL(returnUrl)){
locationUrl = returnUrl;
}
Copy link
Member

Choose a reason for hiding this comment

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

May be useful to have a log entry for the invalid url case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed and pushed

Copy link

@eawoods eawoods left a comment

Choose a reason for hiding this comment

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

All good, working well! Again I defer to Nick's judgement on various improvements.

@ctucker3 ctucker3 merged commit 509228e into develop Jun 16, 2020
@ctucker3 ctucker3 deleted the feature/PRO-33 branch June 16, 2020 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants