Skip to content
This repository has been archived by the owner on Apr 24, 2021. It is now read-only.

some improvements during usage in a project #2

Closed
wants to merge 2 commits into from

Conversation

bkiselka
Copy link

@bkiselka bkiselka commented Feb 21, 2021

Purpose

Does this introduce a breaking change?

[ ] Yes
[x ] No

Pull Request Type

What kind of change does this Pull Request introduce?

[ x] Bugfix
[ x] Feature
[ x] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ ] Other... Please describe:

How to Test

  • Get the code

  • Test the code
    Change app.prompt to empty ("") and compile a new causing the authorize-call to set no prompt-parameter.
    Change app.prompt to login and compile a new causing the authorize-call to use prompt=login.

What to Check

Verify that the following are valid

1) pom.xml use msal4j > 1.8.0 because IAccount is serializable then, see https://github.com/AzureAD/microsoft-authentication-library-for-java/blob/dev/changelog.txt and AzureAD/microsoft-authentication-library-for-java#292

2) AuthHelper.java and authentication.properties: add a configuration property app.prompt: the value might be login, select_account (was/is the default), consent, admin_consent or not set/empty (none), see https://docs.microsoft.com/en-us/azure/active-directory/develop/msal-js-prompt-behavior

3) AuthHelper.java: remove one unnecessary variable initialization (line 35/36) and check (line 228/238) causing a warning

4) AuthHelper.java: improve exception, if state is null or doesn't match or TTL expired
(this will happen, if different session e.g. caused by different domains used as URL (used in the browser), app.redirectUri and app.homePage do not match!)

5) index.jsp: enable call / (e.g. http://localhost:8080/ms-identity-b2c-java-servlet-webapp-authentication/ - no need to call /index )
@ghost
Copy link

ghost commented Feb 21, 2021

CLA assistant check
All CLA requirements met.

Copy link
Contributor

@idg-sam idg-sam left a comment

Choose a reason for hiding this comment

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

Great stuff. I think the sample should demonstrate the prompt options.

I made some comments and suggestions. If you want to update and commit based on the suggestions I'll merge them.

pom.xml Outdated Show resolved Hide resolved
Comment on lines +21 to +26
<%
// enable call / (no need to call /index )
if (request.getAttribute("bodyContent") == null){
request.setAttribute("bodyContent", "auth/status.jsp");
}
%>
Copy link
Contributor

Choose a reason for hiding this comment

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

Already resolved by adding extra route matchers to the servlet that match empty ("/"), /sign_in_status, and /index

Copy link
Author

Choose a reason for hiding this comment

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

Hm, could be but I did not succeed in finding/adding these extra route matchers in the servlet.
What I did: I tried to change line 10 of HomePageServlet.java from
@WebServlet(name = "HomePageServlet", urlPatterns = "/index")
to
@WebServlet(name = "HomePageServlet", urlPatterns = {"/index", "/"})
but still calling http://localhost:8080/ms-identity-b2c-java-servlet-webapp-authentication/ on a tomcat brought a NullPointerException because the bodyContent is null and thus cannot be included in index.jsp
It works, if I call it with http://localhost:8080/ms-identity-b2c-java-servlet-webapp-authentication/index tought!

Comment on lines +20 to +22
# values for prompt, might be login, select_account, consent, admin_consent or empty (none), see https://docs.microsoft.com/en-us/azure/active-directory/develop/msal-js-prompt-behavior
app.prompt=select_account
Copy link
Contributor

Choose a reason for hiding this comment

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

Great stuff. Nice demonstration of the available options.
Is it more correct for us to call this aad.prompt, since we are changing the behaviour of the AAD sign in page?

Comment on lines 151 to 153
} else {
parameters = AuthorizationRequestUrlParameters.builder(REDIRECT_URI, Collections.singleton(SCOPES))
.responseMode(ResponseMode.QUERY).prompt(Prompt.SELECT_ACCOUNT).state(state).nonce(nonce).build();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
} else {
parameters = AuthorizationRequestUrlParameters.builder(REDIRECT_URI, Collections.singleton(SCOPES))
.responseMode(ResponseMode.QUERY).prompt(Prompt.SELECT_ACCOUNT).state(state).nonce(nonce).build();

Maybe these lines are no longer necessary if we make the app require aad.prompt config.

Comment on lines +142 to +143
} else if (Config.getProperty("app.prompt") != null) {
if (!PROMPT.trim().equals("")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
} else if (Config.getProperty("app.prompt") != null) {
if (!PROMPT.trim().equals("")) {
} else if (!PROMPT.trim().equals("")) {

Maybe we can just delete the null check and require the aad.prompt config. the config loader causes the program to exit if there is a null param anyhow.

@idg-sam idg-sam requested a review from sangonzal March 6, 2021 09:48
@idg-sam
Copy link
Contributor

idg-sam commented Apr 24, 2021

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants