Skip to content
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-2101: JWTAuthProvider for authenticating with Json Web Token #1703

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

mightymop
Copy link
Contributor

  • login with user & token
  • token will be checked for

    "subject" (same as user and will be used for user search)
    expiration date
    issuer must be same as configured in OF SystemProperties

  • set JWT Secret String in OF SystemProperties (used vor validating jwt signature)

@lgtm-com
Copy link

lgtm-com bot commented Sep 15, 2020

This pull request introduces 1 alert when merging 0d98251 into 6229c5d - view on LGTM.com

new alerts:

  • 1 for Dereferenced variable may be null

@guusdk
Copy link
Member

guusdk commented Oct 12, 2020

This is an exciting new feature to add! However, especially since it's security related, I'd prefer to see both:

  • more (any!) code comments and javadoc
  • unit tests
  • end-user / admin oriented documentation on how to use this new feature (similar to what we have for the JDBC-providers.

Can you add those?

@guusdk
Copy link
Member

guusdk commented Oct 12, 2020

I've created this issue in JIRA to track this new feature: https://issues.igniterealtime.org/browse/OF-2101

There are a lot of commits in this PR. If you would not mind squashing them, can you add "OF-2101" to a commit message? That way, automation will link the JIRA issue to these commits.

@mightymop
Copy link
Contributor Author

This is an exciting new feature to add! However, especially since it's security related, I'd prefer to see both:

* more (any!) code comments and javadoc

* unit tests

* end-user / admin oriented documentation on how to use this new feature (similar to what we have for the JDBC-providers.

Can you add those?

I have added comments and javadoc and cleaned up the code...
But i have never used unit tests...

xmppserver/pom.xml Outdated Show resolved Hide resolved
- login with user & token
- token will be checked for
  > "subject" (same as user and will be used for user search)
  > expiration date
  > issuer must be same as configured in OF SystemProperties
- set JWT Secret String in OF SystemProperties (used vor validating jwt signature)
- or set an certificate for validating via public key

- add javadoc and comments
- clean up code
- removed unused dependency
@mightymop mightymop changed the title JWTAuthProvider for authenticating with Json Web Token OF-2101: JWTAuthProvider for authenticating with Json Web Token Oct 14, 2020
@guusdk
Copy link
Member

guusdk commented Oct 16, 2020

I'm very interested in having this in Openfire, but I'd like to go over this in more detail that what I am comfortable with prior to the upcoming 4.6.0 release. Lets aim to include this in 4.7.

@akrherz akrherz added this to the 4.7.0 milestone Oct 16, 2020
@gjaekel
Copy link
Contributor

gjaekel commented Jun 9, 2021

@deleolajide You know about this PR? I wonder if this Auth-Provider may be also used for OFmeetings/Pàdé?
Then, in the next sprint, it might be married with it and enhanced to optionally "freeze" some other properties within the users JWT like the name of the room it's valid for.

I got this idea from a German school, which is using (at the moment) plain Jitsi and a self-made, web-based central class schedule application. After login to this, each pupil get an individual time table with lessons, task and tests with identification based on JWTs. For a common virtual classroom session, it has just to follow the link and will be dropped into the right room. And the JWT in this link isn't usable for other times, other rooms and the teacher can be comparable sure that the user of the link is "this" pupil.

If OpenFire/OpenMeetings is used in a business context, this might be also used to provide "foreign tickets" to any session with different restrictions as need (timeslot-based, room-based).

@lgtm-com
Copy link

lgtm-com bot commented Dec 20, 2021

This pull request introduces 3 alerts when merging 7fea70c into 81096dc - view on LGTM.com

new alerts:

  • 3 for Missing JWT signature check

@akrherz akrherz removed this from the 4.7.0 milestone Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants