New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#OF-1285 Create a simple option of resetting admin password. #1249

Merged
merged 4 commits into from Jan 18, 2019

Conversation

Projects
None yet
5 participants
@ma1uta
Copy link
Contributor

ma1uta commented Jan 11, 2019

https://issues.igniterealtime.org/browse/OF-1285

There is a new option in the openfire.xml:

<oneTimeAccessToken>
  <username>admin</username>
  <token>123</token>
</oneTimeAccessToken>

to provide temporary access to admin console with privileges of the specified in the username tag user.
After successful login this option automatically will be removed from the openfire.xml.
It needs enter only the token to login instead of the username and password.

@akrherz

This comment has been minimized.

Copy link
Member

akrherz commented Jan 11, 2019

Woohoo! Awesome work, do you think this could get documented within the documentation found in our git repo?

@guusdk

This comment has been minimized.

Copy link
Member

guusdk commented Jan 11, 2019

@GregDThomas

This comment has been minimized.

Copy link
Contributor

GregDThomas commented Jan 11, 2019

A couple of questions, I would test but I’m offline ATM.

  1. Is an audit entry created when someone logs in using this mechanism? It should.
  2. If you create an entry with a token for a user that doesn’t exist, does it work?
  3. If it does, do subsequent audit actions get correctly logged? Not sure if that requires a user, or just something that looks like a user id.

One suggestion; Pre-populate the initial openfire.xml config file with empty values (and a comment as to their purpose), and rather than delete the XML values, set them to empty strings. That way it’s far easier to set this up.

@guusdk

This comment has been minimized.

Copy link
Member

guusdk commented Jan 11, 2019

@ma1uta

This comment has been minimized.

Copy link
Contributor Author

ma1uta commented Jan 12, 2019

Woohoo! Awesome work, do you think this could get documented within the documentation found in our git repo?

I can write the documentation of course but I don't know where :) If you point me I will do it.

@ma1uta

This comment has been minimized.

Copy link
Contributor Author

ma1uta commented Jan 12, 2019

A couple of questions, I would test but I’m offline ATM.

1. Is an audit entry created when someone logs in using this mechanism? It should.

I see. I will do it. Is this enough to log about this or there is an audit subsystem?

2. If you create an entry with a token for a user that doesn’t exist, does it work?

No :(. There are some checks that specified user is available or is admin. I will look how to fix this (for example make OneTimeToken inherits from the AuthToken and check this special class).

3. If it does, do subsequent audit actions get correctly logged? Not sure if that requires a user, or just something that looks like a user id.

Unfortunately, this user is need to pass the org.jivesoftware.openfire.admin.AdminManager#isUserAdmin(java.lang.String, boolean) and other similar checks.

One suggestion; Pre-populate the initial openfire.xml config file with empty values (and a comment as to their purpose), and rather than delete the XML values, set them to empty strings. That way it’s far easier to set this up.

Ok, I will fix it.

@ma1uta

This comment has been minimized.

Copy link
Contributor Author

ma1uta commented Jan 12, 2019

Yeah, I will look how I can get rid of the specifying user and use a stub.

@GregDThomas

This comment has been minimized.

Copy link
Contributor

GregDThomas commented Jan 12, 2019

Re auditing, Have a look at the JSP that edits system properties - that creates an audit entry when something is changed using an existing framework.

Re. not specifying a user, I worry that this will break other things (e.g. who gets audited when a sys prop is modified?), or will a random user be picked instead?

@ma1uta

This comment has been minimized.

Copy link
Contributor Author

ma1uta commented Jan 12, 2019

I excluded the specifying user and use the random username when passing with the one time token with clearing the token after first successful login.

@@ -163,6 +165,9 @@ public User getUser() {
Log.debug( "Unable to get user: no auth token on session." );
return null;
}
if (authToken instanceof OneTimeAuthToken) {
return new User(authToken.getUsername(), "one time user", null, new Date(), new Date());

This comment has been minimized.

@guusdk

guusdk Jan 14, 2019

Member

Instead of 'one time user', I'd use "Recovery via One Time Auth Token"

@guusdk

This comment has been minimized.

Copy link
Member

guusdk commented Jan 14, 2019

@SimonRWaters, would you mind reviewing this change, given that you have a strong background in security? This change attempts to introduce a feature where one can gain access to the Openfire admin console, by making a modification to the configuration file (that's on disc). The idea is to use this for password recovery purposes.

@GregDThomas

This comment has been minimized.

Copy link
Contributor

GregDThomas commented Jan 14, 2019

OK, had some more time to look at this - overall, this is really good, thank you.

Two small things;

  1. The prompt when logging in seems a little brief (i.e. login.token, set to just "token")- how about "Please supply the time login token".
  2. The user in a "regular" Security Audit Message is different from that from when the OTP is first used;
    image
    How about using something like OTP- for all, so it's clear it's a OTP thing on both login and subsequent actions?

One trivial thing, more a style thing, but feel free to ignore; I'd be tempted to make OneTimeAuthToken a static inner class of AuthToken, to better how closely the two are related.

@guusdk

This comment has been minimized.

Copy link
Member

guusdk commented Jan 17, 2019

I played with this just now. It is great, thanks again! I have a couple of small suggestions for improvement:

  • "Please supply the time login token" needs a 'dot' in the end: "Please supply the time login token."
  • the logintoken.jsp should have a very clear warning that Openfire is running in password recovery mode. Maybe put exactly that text on top of the input field, in bright orange letters: "Openfire is running in password recover mode!"
  • Although it is good to have an example in openfire.xml, I prefer that this example is put inside XML comment tags. This prevents the element from being up completely, which seems safer.
OF-1285 Create a simple option of resetting admin password. Fix typo.…
… Add warning to the login page. Move oneTimeAccessToken tag to the comment with example. Remove entire property after successful login.

@guusdk guusdk merged commit 5d8c2f4 into igniterealtime:master Jan 18, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@IgniteRealtime-Bot

This comment has been minimized.

Copy link

IgniteRealtime-Bot commented Feb 12, 2019

This pull request has been mentioned on Ignite Realtime Community Forums. There might be relevant details there:

https://discourse.igniterealtime.org/t/debian-jessie-openfire-4-2-3-to-4-3-2-upgrade/84163/4

@IgniteRealtime-Bot

This comment has been minimized.

Copy link

IgniteRealtime-Bot commented Feb 12, 2019

This pull request has been mentioned on Ignite Realtime Community Forums. There might be relevant details there:

https://discourse.igniterealtime.org/t/can-not-update-from-4-2-3-to-4-3-2-on-debian/84196/3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment