TRUNK-381: Support for alternate authentication schemes #2833

ahammi · 2019-02-14T11:12:26Z

Issue I worked on

see https://issues.openmrs.org/browse/TRUNK-381

Checklist: I completed these to help reviewers :)

My pull request only contains ONE single commit
(the number above, next to the 'Commits' tab is 1).

No? -> read here on how to squash multiple commits into one
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

coveralls · 2019-02-14T11:28:34Z

Coverage decreased (-0.06%) to 59.458% when pulling 35290cf on ahammi:TRUNK-381 into c9996a6 on openmrs:master.

api/src/test/java/org/openmrs/api/context/ContextTest.java

api/src/main/java/org/openmrs/api/context/UserPassAuthenticationScheme.java

api/src/main/java/org/openmrs/api/context/Authenticated.java

api/src/main/java/org/openmrs/api/context/AuthenticationScheme.java

dkayiwa · 2019-02-14T16:08:57Z

api/src/main/java/org/openmrs/api/context/BasicAuthenticated.java

+
+/**
+ * A basic implementation of {@link Authenticated} that contains the necessary elements for an OpenMRS authentication.
+ */


Can you put a since annotation on this class?

api/src/main/java/org/openmrs/api/context/ContextAuthenticationException.java

api/src/main/java/org/openmrs/api/context/Credentials.java

dkayiwa · 2019-02-14T16:10:30Z

api/src/main/java/org/openmrs/api/context/DaoAuthenticationScheme.java

+ * Base class for authentication schemes that intend to leverage OpenMRS' {@link ContextDAO}.
+ *
+ * @see {@link Context#authenticate(AuthenticationScheme, Credentials)}
+ */


Can you put a since annotation on this?

api/src/main/java/org/openmrs/api/context/UserPassAuthenticationScheme.java

api/src/main/java/org/openmrs/api/context/UserPassCredentials.java

api/src/main/java/org/openmrs/api/context/Context.java

dkayiwa · 2019-02-14T17:24:50Z

api/src/main/java/org/openmrs/api/context/Credentials.java

+	 * Typically in the case of a user the client would simply be identified with its username. 
+	 */
+	public String getClientName();
+


How do you plan to deal with this? "Credentials will contains scheme-specific properties"

@dkayiwa this quote comes from @bmamlin's suggested JavaDoc on the ticket's description. I think the point is that devs/implementers can keep extending Credentials implementations (such as extending UserPassCredentials for example) or can provide their own implementations of Credentials. Either way they will add members and methods that are useful to their custom auth. schemes. We can't anticipate as of yet what will be done, we can just leave the design open enough.

api/src/main/java/org/openmrs/api/context/BasicAuthenticated.java

api/src/main/java/org/openmrs/api/context/UserPassAuthenticationScheme.java

dkayiwa · 2019-02-15T09:51:17Z

api/src/main/java/org/openmrs/api/context/UserPassAuthenticationScheme.java

+ */
+public class UserPassAuthenticationScheme extends DaoAuthenticationScheme {
+
+	protected static Log log = LogFactory.getLog(UserPassAuthenticationScheme.class);


Any reason why the above is not private static final?

api/src/main/java/org/openmrs/api/context/UserPassAuthenticationScheme.java

dkayiwa · 2019-02-15T10:15:20Z

api/src/main/java/org/openmrs/api/context/UserPassCredentials.java

+ */
+public class UserPassCredentials implements Credentials {
+
+	protected String username;


Any reason why these are not private?

api/src/test/java/org/openmrs/api/context/ContextTest.java

dkayiwa · 2019-02-15T10:42:00Z

api/src/test/java/org/openmrs/api/context/TestUuidAuthenticationScheme.java

+/*
+ * This test auth scheme logs a user simply based on asserting that its UUID is valid and points to an exisiting user.
+ */
+public class TestUuidAuthenticationScheme extends DaoAuthenticationScheme {


Wouldn't it be better to instead of use a test with a username and password? Some sort of admin/test combination. For this to me looks closer to the real basic authentication which we have that is based on a username and password combination.

Just to say this more explicitly, this test should also stay to ensure that we support more than just our basic username and password db based authentication.

@dkayiwa yes absolutely, this test should have existed already with the method that we suggest to deprecate. Anyway I added it in the commit #3.

api/src/main/java/org/openmrs/api/context/UserContext.java

dkayiwa · 2019-02-15T11:02:46Z

api/src/main/java/org/openmrs/api/context/UserContext.java

+	public Authenticated authenticate(AuthenticationScheme authenticationScheme, Credentials credentials)
+			throws ContextAuthenticationException {
+
+		if (log.isDebugEnabled()) {


Do we need the log.isDebugEnabled() check?

dkayiwa · 2019-02-15T11:02:58Z

api/src/main/java/org/openmrs/api/context/UserContext.java


-		return this.user;
+		if (log.isDebugEnabled()) {


Do we need the log.isDebugEnabled() check?

@dkayiwa those checks are all over the code base though, so I will let them in for now. I agree that it's weird, I would have assumed that a logger knows whether it supports debugging or not. Please advise.

Are you sure there is any such in the latest version of master?

@dkayiwa yes if you do a search, simply via GitHub like here, you will find dozens. One example on Concept: https://github.com/openmrs/openmrs-core/blob/master/api/src/main/java/org/openmrs/Concept.java#L451

Permalink here.

There was a pull request which removed those in a number of classes. I just noticed that it did not fully remove them from the entire codebase. 😊

dkayiwa · 2019-02-15T11:03:17Z

api/src/main/java/org/openmrs/api/context/UserPassAuthenticationScheme.java

+	public Authenticated authenticate(Credentials credentials)
+			throws ContextAuthenticationException {
+
+		if (log.isDebugEnabled()) {


Do we need the log.isDebugEnabled() check?

dkayiwa · 2019-02-15T11:25:38Z

Which API will a module use for the wiring of a new authentication scheme? Will that be in another pull request?

mks-d · 2019-02-15T16:29:21Z

Which API will a module use for the wiring of a new authentication scheme? Will that be in another pull request?

@dkayiwa no wiring per se, you just explicitly invoke Context.authenticate(AuthenticationScheme, Credentials) with your own pair scheme/credentials defined in your custom module.

We are working on a OAuth2 client module that authenticates with Keycloak, we will be able to provide an example shortly with a custom scheme. No Spring magic as of yet.

dkayiwa · 2019-02-16T17:42:20Z

@mks-d what i mean is when some one wants the existing unmodified openmrs reference application login page to use a custom authentication scheme provided by a module.

dkayiwa · 2019-02-17T11:11:30Z

api/src/main/java/org/openmrs/api/context/Context.java

@@ -267,6 +267,9 @@ public static void setContext(ServiceContext ctx) {
 	}

 	/**
+	 * @deprecated


Did you take a look at this? https://wiki.openmrs.org/display/docs/Java+Conventions#JavaConventions-Deprecation

@dkayiwa I did this in my last commit:

/** * @deprecated as of 2.3.0, replaced by #authenticate(AuthenticationScheme, Credentials) ... */

Is that what you expect? If not please point me to the exact JavaDoc that you would like to have in place.

dkayiwa · 2019-02-17T11:13:25Z

api/src/main/java/org/openmrs/api/context/UsernamePasswordAuthenticationScheme.java

+	public Authenticated authenticate(Credentials credentials)
+			throws ContextAuthenticationException {
+
+		if (log.isDebugEnabled()) {


Do we need the log.isDebugEnabled() check?

mks-d · 2019-02-18T09:55:10Z

@mks-d what i mean is when some one wants the existing unmodified openmrs reference application login page to use a custom authentication scheme provided by a module.

@dkayiwa this PR does not yet cover such use case. It rather provides devs with an API to authenticate with other schemes if they need to.

Btw I would suggest that we follow up with a PR on the Ref App module so that it uses the new API rather than the one that will be deprecated.

dkayiwa · 2019-02-18T10:04:26Z

I was looking at this text in the ticket description: " Ideally, substitution (or addition) of the authentication scheme would be protected (e.g., limited to the configuration file setting or not easily done through the API) so that random code could not easily substitute a bogus authentication scheme to subvert proper authentication."

My understanding of the above was not to necessarily hard code the UsernamePasswordAuthenticationScheme but provide it as the default with ability to be overridden by the same authenticate(String username, String password) method call.

dkayiwa · 2019-02-18T10:07:32Z

Infact the public authenticate(AuthenticationScheme authenticationScheme, Credentials credentials) seems to do the opposite of the above. That is, makes it so easy for some random code to substitute a bogus authentication scheme to subvert proper authentication. 😊

mks-d · 2019-02-18T10:39:56Z

Infact the public authenticate(AuthenticationScheme authenticationScheme, Credentials credentials) seems to do the opposite of the above. That is, makes it so easy for some random code to substitute a bogus authentication scheme to subvert proper authentication. 😊

@dkayiwa I see, so authentication schemes should be Spring-wired, and the Spring config would use the add and remove protected methods. So that's how UserContext knows which scheme(s) to use.
But when using an alternate scheme, how does one pass to UserContext the credentials to use with the scheme, since the only method that could ever be called is Context.authenticate(String username, String password)?

Does that imply that the only user identification that could ever be passed downstream is the username?

mks-d · 2019-02-18T12:33:50Z

@dkayiwa more precisely, deep down one will rely on ContextDAO to fetch the authenticated user, and it's only possible to fetch users by UUID (see here). What will be needed by alternate schemes is a getUserByUsername(String) so this will be added in the next commit.

mks-d requested review from bmamlin and dkayiwa February 14, 2019 13:09

mks-d requested changes Feb 14, 2019

View reviewed changes

api/src/test/java/org/openmrs/api/context/ContextTest.java Outdated Show resolved Hide resolved

api/src/main/java/org/openmrs/api/context/UserPassAuthenticationScheme.java Outdated Show resolved Hide resolved

TRUNK-381: Support for alternate authentication scheme

4956e5a

mks-d force-pushed the TRUNK-381 branch from 28bf4a0 to 4956e5a Compare February 14, 2019 14:33

mks-d approved these changes Feb 14, 2019

View reviewed changes