Smart Configuration #186

VaishSiddharth · 2020-05-27T21:12:06Z

Description of what I changed

Issue I worked on

see https://issues.openmrs.org/browse/FM2-

Checklist: I completed these to help reviewers :)

My IDE is configured to follow the code style of this project.

No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend
I have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)

No? -> write tests and add them to this commit git add . && git commit --amend
I ran mvn clean package right before creating this pull request and
added all formatting changes to my commit.

No? -> execute above command
All new and existing tests passed.

No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.

No? Unsure? -> execute command git pull --rebase upstream master

codecov · 2020-05-27T21:20:46Z

Codecov Report

Merging #186 into master will decrease coverage by 1.78%.
The diff coverage is 27.92%.

@@             Coverage Diff              @@
##             master     #186      +/-   ##
============================================
- Coverage     80.82%   79.04%   -1.78%     
- Complexity     1482     1497      +15     
============================================
  Files           161      167       +6     
  Lines          3812     3937     +125     
  Branches        497      516      +19     
============================================
+ Hits           3081     3112      +31     
- Misses          508      600      +92     
- Partials        223      225       +2

Impacted Files	Coverage Δ	Complexity Δ
.../module/fhir2/web/filter/AuthenticationFilter.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
.../openmrs/module/fhir2/web/SmartWebInitializer.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...le/fhir2/web/filter/SmartAuthenticationFilter.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...ule/fhir2/web/smart/SmartAuthenticationScheme.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
.../module/fhir2/web/smart/SmartTokenCredentials.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...nmrs/module/fhir2/web/filter/ForwardingFilter.java	`68.97% <70.59%> (-10.20%)`	`5.00 <0.00> (ø)`
...org/openmrs/module/fhir2/web/SmartConformance.java	`90.91% <90.91%> (ø)`	`10.00 <10.00> (?)`
...mrs/module/fhir2/api/dao/impl/FhirUserDaoImpl.java	`100.00% <100.00%> (ø)`	`4.00 <2.00> (+1.00)`
...dule/fhir2/web/servlet/FhirSmartConfigServlet.java	`100.00% <100.00%> (ø)`	`3.00 <3.00> (?)`
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d30e7b0...e49ffe9. Read the comment docs.

ibacher · 2020-06-02T14:37:46Z

omod/src/main/resources/config.xml

+	<filter-mapping>
+		<filter-name>clientAuthenticationFilter</filter-name>
+		<url-pattern>/keycloak/*</url-pattern>
+		<url-pattern>/protected/*</url-pattern>
+	</filter-mapping>


So what URLs do we actually want to protect?

@ibacher I am not very sure about it that's why I left it unchanged

The url-pattern determines which URLs this filter is run against. It should probably look very similar to the AuthenticationFilter, since they're ultimately protecting the same thing.

Don't you think that it'll be different than that in AuthenticationFilter because earlier we did not have any client (SMART App), we only had users

No, because we're still protecting the same resources either way. That's what's important here.

VaishSiddharth · 2020-06-02T15:15:06Z

omod/src/main/java/org/openmrs/module/fhir2/web/filter/ClientAuthenticationFilter.java

+	@Override
+	public void doFilter(ServletRequest req, ServletResponse res, FilterChain chain) throws IOException, ServletException {
+		super.doFilter(req, res, chain);
+	}


@ibacher Could you give some idea on what all should go in here as most of the code that we might need is already present in the super function

So what we need to do is to figure out the user name. From here, it looks like we can do that by getting the appropriate request attribute.

Then, we need to login the user. This can be done by running Context.becomeUser() with the user id (which we can hopefully obtain from the account.

ibacher · 2020-06-02T20:12:19Z

omod/src/main/resources/config.xml

@@ -67,7 +67,11 @@
 	<filter-mapping>
 		<filter-name>clientAuthenticationFilter</filter-name>
 		<url-pattern>/keycloak/*</url-pattern>


You definitely don't need this URL pattern. A URL pattern is just matched against the incoming URL without the host (http://localhost) or context (/openmrs), and OpenMRS doesn't have anything that responds to /keycloak.

VaishSiddharth · 2020-06-03T17:43:24Z

omod/src/main/java/org/openmrs/module/fhir2/web/filter/ClientAuthenticationFilter.java

+			if (!httpRequest.getAuthType().contains("BASIC")) {
+
+				super.doFilter(req, res, chain);
+				if (httpRequest.getRequestedSessionId() != null && !httpRequest.isRequestedSessionIdValid()) {
+					Context.logout();
+				}
+				if (httpRequest.getAttribute(KeycloakAccount.class.getName()) != null) {
+					OidcKeycloakAccount account = (OidcKeycloakAccount) httpRequest
+					        .getAttribute(KeycloakAccount.class.getName());
+					Context.becomeUser(account.getPrincipal().getName());
+				} else {
+					if (!res.isCommitted()) {
+						HttpServletResponse httpResponse = (HttpServletResponse) res;
+						httpResponse.sendError(HttpServletResponse.SC_UNAUTHORIZED, "Not authenticated");
+						return;
+					}
+				}
 			}
 		}
+


@ibacher This gives an NPE but don't know why it makes sense to me. The super function will always commit to response thats why I thought to write something like this.

Error log

SEVERE: Servlet.service() for servlet [openmrs] in context with path [/openmrs] threw exception java.lang.NullPointerException at org.openmrs.module.fhir2.web.filter.ClientAuthenticationFilter.doFilter(ClientAuthenticationFilter.java:47) at org.openmrs.module.web.filter.ModuleFilterChain.doFilter(ModuleFilterChain.java:71) at org.openmrs.module.referenceapplication.filter.RequireLoginLocationFilter.doFilter(RequireLoginLocationFilter.java:93) at org.openmrs.module.web.filter.ModuleFilterChain.doFilter(ModuleFilterChain.java:71) at org.openmrs.module.web.filter.ModuleFilter.doFilter(ModuleFilter.java:57) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:241) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:208)

The basic auth also doesn't works, neither for the last commit nor this one

I'm not sure what part of the code this is referring to exactly, because I think the line numbers have shifted?

ibacher

@VaishSiddharth Thanks for your work on this! These changes are looking really goood! I've made a few comments, but this feels like it's starting to come together.

ibacher · 2020-06-03T18:08:37Z

omod/src/main/java/org/openmrs/module/fhir2/web/SmartConformance.java

+@Data
+public class SmartConformance {
+
+	private String authorization_endpoint;


This doesn't have to be dealt with straight-away, but these should be camelCase, i.e., authorizationEndpoint rather than authorization_endpoint. We can then translate them to the appropriate name using an annotation, i.e.,

@JsonProperty("authorization_endpoint") private String authorizationEndpoint;

ibacher · 2020-06-03T18:09:36Z

omod/src/main/java/org/openmrs/module/fhir2/web/filter/ClientAuthenticationFilter.java

+	@Override
+	public void destroy() {
+		super.destroy();
+	}


We can probably get rid of this method.

ibacher · 2020-06-03T18:10:11Z

omod/src/main/java/org/openmrs/module/fhir2/web/filter/ForwardingFilter.java

-
+			if (requestURI.contains("/.well-known")) {
+				replacement = "/ms/fhir2SmartConfig";
+			}
 			String newURI = requestURI.replace(prefix, replacement);


There should be a new line before here.

ibacher · 2020-06-03T18:15:55Z

omod/src/main/java/org/openmrs/module/fhir2/web/filter/ForwardingFilter.java

 			} else {
 				((HttpServletResponse) res).sendError(HttpServletResponse.SC_NOT_FOUND);
 				return;
 			}
-


I actually think we should have a new line here

ibacher · 2020-06-03T18:16:08Z

omod/src/main/java/org/openmrs/module/fhir2/web/filter/ForwardingFilter.java

@@ -53,13 +53,17 @@ public void doFilter(ServletRequest req, ServletResponse res, FilterChain chain)
 			} else if (R4.equals(fhirVersionCase)) {
 				prefix += "/" + fhirVersion;
 				replacement = "/ms/fhir2Servlet";
+


This new line should not exist

ibacher · 2020-06-03T18:17:56Z

omod/src/main/java/org/openmrs/module/fhir2/web/filter/ClientAuthenticationFilter.java

+	public void doFilter(ServletRequest req, ServletResponse res, FilterChain chain) throws IOException, ServletException {
+		if (req instanceof HttpServletRequest) {
+			HttpServletRequest httpRequest = (HttpServletRequest) req;
+			if (!httpRequest.getAuthType().contains("BASIC")) {


This should be:

if (httpRequest.getAuthType() != null && !httpRequest.getAuthType().equals(HttpServletRequest.BASIC_AUTH))

ibacher · 2020-06-03T18:19:17Z

omod/src/main/java/org/openmrs/module/fhir2/web/servlet/Fhir2SmartConfig.java

+		ObjectMapper objectMapper = new ObjectMapper();
+
+		SmartConformance smartConformance = new SmartConformance();


Any reason these can't be class objects? I.e., so we only have one objectMapper and one smartConformance per servlet, rather than creating new ones for every request.

@ibacher Not sure about what should be done here.

Oh... I just meant turn them into class properties:

class Fhir2SmartConfig extends HttpServlet { private static final ObjectMapper objectMapper = new ObjectManager(); private final SmartConformance smartConformance; public Fhir2SmartConfig() { this.smartConformance = new SmartConformance(); // set properties here } public void doGet(HttpServletRequest req, HttpServletResponse res) throws ServletException, IOException { PrintWriter out = res.getWriter(); res.setContentType("application/json"); res.setCharacterEncoding("UTF-8"); res.setStatus(200); objectMapper.writeValue(out, smartConformance); // why do we need these next two lines? out.print(objectMapper); out.flush(); } }

VaishSiddharth · 2020-06-09T07:45:35Z

omod/src/main/java/org/openmrs/module/fhir2/web/filter/SmartAuthenticationFilter.java

+					OidcKeycloakAccount account = (OidcKeycloakAccount) httpRequest
+					        .getAttribute(KeycloakAccount.class.getName());
+
+					String userName = account.getKeycloakSecurityContext().getToken().getSubject();


@bacher This still give the internal keycloak user id

This solves the error.

String userName = account.getKeycloakSecurityContext().getToken().getPreferredUsername();

Ok, use that.

VaishSiddharth · 2020-06-09T11:45:42Z

omod/src/main/java/org/openmrs/module/fhir2/web/filter/SmartAuthenticationFilter.java

+						if (httpRequest.getContextPath() != null) {
+							newRequestUri.append(httpRequest.getContextPath());
+						}
+
+						if (httpRequest.getServletPath() != null) {
+							newRequestUri.append(httpRequest.getServletPath());
+						}
+
+						if (httpRequest.getPathInfo() != null) {
+							newRequestUri.append(httpRequest.getPathInfo());
+						}
+
+						newRequestUri.append('?').append(newQueryString);
+
+						req.getRequestDispatcher(newRequestUri.toString()).forward(req, res);
+					}


@ibacher Don't you think that we still need the ForwardingFilter to execute?

Also there is a mistake in the url formation it adds /openmrs/openmrs two times

This isn't intended to override the ForwardingFilter, only to remove the access_token from the query before it hits HAPI. Note that this "forwarding" is really just the way to internally change the URL without having to worry about too many details.

Also there is a mistake in the url formation it adds /openmrs/openmrs two times

That clearly needs to be fixed.

ibacher

So, we can't support config.xml in a non-OMOD submodule. Which does mean that we're going to need another means of loading the SMART filter. I'll have to give that some thought.

ibacher · 2020-06-12T14:20:53Z

omod/pom.xml

@@ -38,7 +38,7 @@
 		</dependency>
 		<dependency>
 			<groupId>${project.parent.groupId}</groupId>
-			<artifactId>smart</artifactId>
+			<artifactId>smartSupport</artifactId>


We should probably call this smart-support

ibacher · 2020-06-12T14:22:30Z

...rces/org/openmrs/module/fhir2/api/dao/impl/FhirObservationDaoImplTest_initial_data_suppl.xml

@@ -9,8 +9,8 @@
    graphic logo is a trademark of OpenMRS Inc.
 -->
 <dataset>


Not sure why this is part of this commit?

@ibacher Earlier I had changed this so that the tests pass for openmrsPlaformVersion 2.3.0. So I reverted it back to this after creating a new sub module

ibacher · 2020-06-12T14:22:41Z

api/src/test/java/org/openmrs/module/fhir2/api/search/PatientSearchQueryImplTest.java

@@ -310,7 +310,7 @@ public void searchForPatients_shouldSearchForPatientsByBirthDateWithLowerBound()

 		assertThat(results, notNullValue());
 		assertThat(get(results), not(empty()));
-		assertThat(get(results), hasSize(equalTo(1)));
+		assertThat(get(results), hasSize(greaterThan(1)));


Not sure why this is part of this commit?

@ibacher Earlier I had changed this so that the tests pass for openmrsPlaformVersion 2.3.0. So I reverted it back to this after creating a new sub module

ibacher · 2020-06-12T14:23:13Z

pom.xml

@@ -657,7 +657,7 @@
 		<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
 		<project.build.outputEncoding>UTF-8</project.build.outputEncoding>
 		<lombokVersion>1.18.10</lombokVersion>
-        <openmrsPlatformVersion>2.3.0</openmrsPlatformVersion>
+        <openmrsPlatformVersion>2.0.2</openmrsPlatformVersion>


This should be 2.0.5. Also, please ensure your IDE is configured to indent XML files with tabs.

Ok will make this change

VaishSiddharth · 2020-06-17T13:11:02Z

omod/src/main/java/org/openmrs/module/fhir2/web/filter/AuthenticationFilter.java

 						catch (Exception ignored) {
-							HttpServletResponse httpResponse = (HttpServletResponse) response;
-							httpResponse.sendError(HttpServletResponse.SC_UNAUTHORIZED, "Not authenticated");
+							if (!response.isCommitted()) {
+								HttpServletResponse httpResponse = (HttpServletResponse) response;
+								httpResponse.sendError(HttpServletResponse.SC_UNAUTHORIZED, "Not authenticated");
+							}
+							return;
 						}


@ibacher The only error that is left now is this, even on giving the correct password and username for basic auth the catch block is executed with invalid credentials error

I guess that there is some problem with the authentication scheme I mean either its not set or it is changed because of the keycloak filter. I can say this because I copy pasted the same AuthenticationFilter code to the master branch and tried it with the same request and it worked perfectly fine.

@VaishSiddharth This change should be made outside of this PR, since it's outside of the scope of the SMART stuff. I'm aware that's not really the question, but to see why this is failing, this really needs to be stepped through with a debugger.

VaishSiddharth · 2020-06-17T18:11:31Z

...t-support/src/main/java/org/openmrs/module/fhir2/web/smart/UsernameAuthenticationScheme.java

+
+		if (!(credentials instanceof SmartTokenCredentials)) {
+			throw new ContextAuthenticationException("Invalid credentials");
+		}
+


@ibacher I figured out that the error is because of this part. I tried changing it to

if (!(credentials instanceof SmartTokenCredentials)) { return new BasicAuthenticated(dao.getUserByUserName(credentials.getClientName()), UsernamePasswordCredentials.SCHEME); }

But it didn't work.

Could you help me with this?

ibacher · 2020-06-18T19:47:24Z

Closing this as we are moving this support to a new module.

ibacher reviewed Jun 2, 2020

View reviewed changes

VaishSiddharth commented Jun 2, 2020

View reviewed changes

VaishSiddharth force-pushed the SmartSupport branch from 5de2332 to fb9911f Compare June 2, 2020 17:00

ibacher reviewed Jun 2, 2020

View reviewed changes

VaishSiddharth force-pushed the SmartSupport branch from 4968b9d to 29d7ebe Compare June 3, 2020 17:41

VaishSiddharth commented Jun 3, 2020

View reviewed changes

ibacher reviewed Jun 3, 2020

View reviewed changes

VaishSiddharth commented Jun 9, 2020

View reviewed changes

ibacher reviewed Jun 12, 2020

View reviewed changes

VaishSiddharth force-pushed the SmartSupport branch from cd88de1 to ab63c34 Compare June 12, 2020 16:48

VaishSiddharth commented Jun 17, 2020

View reviewed changes

VaishSiddharth and others added 16 commits June 17, 2020 18:15

controller for well known config add in omod

306a906

improvements

9aa717e

wellKnown servlet

ef001c7

wellKnown servlet fixes

ad2e36f

keycloak filter add

ad22a10

client filter config

4de4810

client filter improve

b924598

keycloak json add

54666a9

client filter improvement

8c793ec

fixes

2cff059

dependencies fix

2f624a5

Load Keycloak config from classpath

c884300

classpath fix keycloak.json

a6221ee

client auth dependency add

f7ab201

oauth login add

257b133

Some more ideas

73b036c

ibacher and others added 12 commits June 17, 2020 18:15

Some more renaming and a few other things

9a0e774

fixes

53b8e85

removed access_token support code

8047053

smart module add

80271e4

different openmrs version for submodule

82bfda8

fixes

9e74861

fixes

f3c5a50

rebase

2660e6e

smart servlet and filter work

8e37591

tests for smart config

877070c

response commit error fix

a5a5edd

Fixes for my environment

074ff69

ibacher force-pushed the SmartSupport branch from c469b04 to 074ff69 Compare June 18, 2020 16:02

smartfilter config add

e49ffe9

ibacher closed this Jun 18, 2020

		ObjectMapper objectMapper = new ObjectMapper();

		SmartConformance smartConformance = new SmartConformance();

Smart Configuration #186

Smart Configuration #186

Conversation

VaishSiddharth commented May 27, 2020

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

codecov bot commented May 27, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VaishSiddharth Jun 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VaishSiddharth Jun 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VaishSiddharth Jun 3, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibacher 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

VaishSiddharth Jun 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VaishSiddharth Jun 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibacher 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

VaishSiddharth Jun 17, 2020 • edited

Choose a reason for hiding this comment

ibacher Jun 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibacher commented Jun 18, 2020

codecov bot commented May 27, 2020 •

edited

VaishSiddharth Jun 2, 2020 •

edited

VaishSiddharth Jun 2, 2020 •

edited

VaishSiddharth Jun 3, 2020 •

edited

VaishSiddharth Jun 9, 2020 •

edited

VaishSiddharth Jun 9, 2020 •

edited

VaishSiddharth Jun 17, 2020 •

edited

ibacher Jun 17, 2020 •

edited