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

Hotfix authapi #1244

Merged
merged 6 commits into from
Apr 18, 2019
Merged

Conversation

joshuaboniface
Copy link
Member

Changes
Add a MethodNotAllowedException to the HTTP handler. Checks the /Users/AuthenticateByName API call to see if a plaintext password has been provided; if not, return the MethodNotAllowedException to the client.

Issues
Addresses #1222

Don't accept pre-hashed (not-plaintext) passwords as the auth
provider no longer supports this due to sha1+salting the passwords
in the database.
@joshuaboniface joshuaboniface changed the base branch from master to release-10.3.z April 18, 2019 02:38
@joshuaboniface
Copy link
Member Author

CC @Tolriq I think this is what you're hoping for with the API return!

Copy link
Member

@anthonylavado anthonylavado left a comment

Choose a reason for hiding this comment

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

Just check with the other C# folk to see if my comment is valid or not. I haven’t tested this, but it looks okay to me.

@@ -379,6 +379,11 @@ public object Post(AuthenticateUser request)
throw new ResourceNotFoundException("User not found");
}

if (request.Pw == "")
Copy link
Member

Choose a reason for hiding this comment

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

I’m no genius, but I think you want a string.IsNullOrEmpty(request.Pw) here.

Copy link
Member

Choose a reason for hiding this comment

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

Yes

Copy link
Member

Choose a reason for hiding this comment

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

So here’s the suggestion:

Suggested change
if (request.Pw == "")
if (string.IsNullOrEmpty(request.Pw))

@Tolriq
Copy link

Tolriq commented Apr 18, 2019

The 405 error yes but I'm not sure the check is OK as Jellyfin allows to have users without password. (The default created account is the server name without one for example)

And since it seems pre hashed password have priority later in code, proper check is probablyif (!string.IsNullOrEmpty(request.Password)) (Should be enough since it seems you do not use the PasswordMd5 in the later call).

@joshuaboniface
Copy link
Member Author

@Tolriq @cvium @anthonylavado So I guess this construct would be the best of both?

if (!string.IsNullOrEmpty(request.Password) || string.IsNullOrEmpty(request.Pw))

@Tolriq
Copy link

Tolriq commented Apr 18, 2019

Well no you still return the error for empty password.

Proper would be if (!string.IsNullOrEmpty(request.Password) && string.IsNullOrEmpty(request.Pw)) but then you'd need to update line 390
Password = request.Password, by Password = null, to properly ignore when both are provided.

@joshuaboniface joshuaboniface requested review from anthonylavado and cvium and removed request for anthonylavado April 18, 2019 14:24
Copy link
Member

@anthonylavado anthonylavado left a comment

Choose a reason for hiding this comment

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

LGTM

@joshuaboniface joshuaboniface added this to To Do in Release 10.3.0 via automation Apr 18, 2019
@joshuaboniface joshuaboniface moved this from To Do to To review in Release 10.3.0 Apr 18, 2019
Copy link
Contributor

@sparky8251 sparky8251 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@joshuaboniface joshuaboniface merged commit 06a1e1f into jellyfin:release-10.3.z Apr 18, 2019
Release 10.3.0 automation moved this from To review to Done Apr 18, 2019
@joshuaboniface joshuaboniface deleted the hotfix-authapi branch April 18, 2019 22:01
@Tolriq
Copy link

Tolriq commented Apr 19, 2019

Thanks for all the changes. Do you plan to make an RC3? Do you have an approximative date for official 10.3 release?

@joshuaboniface
Copy link
Member Author

@Tolriq Happy to help keep Yatse compatible ;-) I'm hoping for final release in the next day or so, no RC3 unless a showstopper comes up.

@Tolriq
Copy link

Tolriq commented Apr 19, 2019

Hum ok :( Bad timing so as I've just released without support for auto fixing connexion.

Will see if there's too much support to push a quick fix release.

@joshuaboniface
Copy link
Member Author

Ah darn, yea, will probably release tonight. Keep me updated either way, and I can adjust release notes accordingly.

@joshuaboniface
Copy link
Member Author

joshuaboniface commented Apr 19, 2019

@Tolriq What version would you target for the updated changes? Just writing a note in the release notes for Yatse users to avoid upgrading until your release is out.

@Tolriq
Copy link

Tolriq commented Apr 19, 2019

My release is out and support Jellyfin 10.3 if they add it manually.

The issue is about users that already have Jellyfin configured, it's currently seen as an Emby device and so won't be automatically migrated until I handle the error 405 case.

You can just add a note that they may need to remove / add again the host as Jellyfin to ensure.

@joshuaboniface
Copy link
Member Author

Sounds good to me, thanks @Tolriq!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants