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

Remote auth not working #123

Closed
viktorkrivak opened this issue Apr 21, 2020 · 17 comments
Closed

Remote auth not working #123

viktorkrivak opened this issue Apr 21, 2020 · 17 comments
Labels
Milestone

Comments

@viktorkrivak
Copy link

viktorkrivak commented Apr 21, 2020

What component is this about ?

ara-server, mainly rest-api

What is your ARA installation like ?

Found in version 1.3.2 (from debian package) but same code is in master

What is happening ?

When EXTERNAL_AUTH is enabled, only admin and healthcheck working properly. /api and index return 401

What should be happening ?

Everything should be working same as it internal auth is used.

Main problem is in server.settings because rest_framework use BasicAuth even if EXTERNAL_AUTH is set.
This patch should fix issue

diff --git a/ara/server/settings.py b/ara/server/settings.py
index 252fa98..b4691d1 100644
--- a/ara/server/settings.py
+++ b/ara/server/settings.py
@@ -214,6 +214,12 @@ ROOT_URLCONF = "ara.server.urls"
 APPEND_SLASH = False
 
 PAGE_SIZE = settings.get("PAGE_SIZE", 100)
+
+if EXTERNAL_AUTH:
+    REST_FRAMEWORK_AUTH = ('rest_framework.authentication.RemoteUserAuthentication',)
+else:
+    REST_FRAMEWORK_AUTH = ('rest_framework.authentication.BasixAuthentication',)
+
 REST_FRAMEWORK = {
     "DEFAULT_PAGINATION_CLASS": "rest_framework.pagination.LimitOffsetPagination",
     "PAGE_SIZE": PAGE_SIZE,
@@ -227,7 +233,7 @@ REST_FRAMEWORK = {
         "rest_framework.parsers.FormParser",
         "rest_framework.parsers.MultiPartParser",
     ),
-    "DEFAULT_AUTHENTICATION_CLASSES": ("rest_framework.authentication.BasicAuthentication",),
+    "DEFAULT_AUTHENTICATION_CLASSES": REST_FRAMEWORK_AUTH ,
     "DEFAULT_PERMISSION_CLASSES": ("ara.api.auth.APIAccessPermission",),
     "TEST_REQUEST_DEFAULT_FORMAT": "json",
     "UNICODE_JSON": False,

NOTE: sorry If this is not a proper way to post patch. There are no pull request so I have no idea where to put this.

@dmsimard dmsimard added the bug label Apr 21, 2020
@dmsimard
Copy link
Contributor

Hey @viktorkrivak and thanks for the issue.
Your assessment of the problem and the patch makes sense at first glance.

If you'd like to submit the patch, there are unfortunately a few hoops to jump through for your first patch (see docs) but your contribution would be appreciated and I would be more than happy to help you get started on Slack or IRC.

Back to the issue, how are you using EXTERNAL_AUTH right now ? I'd like to be able to reproduce the issue and test the patch.

Somewhat related: I've noticed just now that it's missing from the docs and so I created an issue so we don't forget about it: #124

@viktorkrivak
Copy link
Author

Hi sorry, I didn't notice that part of documentation about gerrit. I'll send patch the correct way.
Also I'll write some notes about EXTERNAL_AUTH to #124 . But it'll take some time because I use apache connected to LDAP so it is unnecessary complex setup for documentation, so I need to simplify it.

@dmsimard
Copy link
Contributor

Hi sorry, I didn't notice that part of documentation about gerrit. I'll send patch the correct way.

🎉

Also I'll write some notes about EXTERNAL_AUTH to #124 . But it'll take some time because I use apache connected to LDAP so it is unnecessary complex setup for documentation, so I need to simplify it.

That's great! Let me know if I can help!

@keuko
Copy link
Contributor

keuko commented Aug 13, 2020

Hi, I'm debian maintainer for ara packages. Would you please let me know when this will be merged ? I would like to upload new version and want to be sure that this will go to upstream.

@dmsimard
Copy link
Contributor

Hey @viktorkrivak, were you still interested in sending the patch ?

Thanks !

@dmsimard dmsimard added this to the 1.5 milestone Aug 13, 2020
@keuko
Copy link
Contributor

keuko commented Aug 13, 2020

Hey @viktorkrivak, were you still interested in sending the patch ?

Thanks !

Well, Viktor is my colleguage :) , I think he already sent a patch above ... But you probably meant Viktor should send pull request , right ?

@dmsimard
Copy link
Contributor

@keuko OH! Yes, it would be great if he could send it through gerrit. We aren't set up for pull requests yet :(
I mentioned the docs in a comment above.

@keuko
Copy link
Contributor

keuko commented Aug 13, 2020

@dmsimard , ok I'm going to process it...

@dmsimard
Copy link
Contributor

Much appreciated ! Thank you and let me know if you need a hand.

@keuko
Copy link
Contributor

keuko commented Aug 13, 2020

Much appreciated ! Thank you and let me know if you need a hand.

Here it is :) ! https://review.opendev.org/746140 , let me know if I need to change something.

@dmsimard
Copy link
Contributor

Much appreciated ! Thank you and let me know if you need a hand.

Here it is :) ! https://review.opendev.org/746140 , let me know if I need to change something.

Looks good! I had to rebase your patch for an unrelated failure and left a comment -- should be good to merge afterwards. Thanks.

arecordsansible pushed a commit that referenced this issue Aug 13, 2020
Change-Id: I0ab7f62bfcd2ac3d274aa4016c5debfae7f704b0
Related: #123
Depends-On: https://review.opendev.org/#/c/746145
@dmsimard
Copy link
Contributor

@keuko there you go 🎉

1.4.3 was released earlier this week and the next release is likely going to be 1.5 with the new CLI work so this is where it would land.

From a packaging standpoint, there is a minor change in yaml library (see https://src.fedoraproject.org/rpms/ara/c/2527ca0fbd214dc6269ef8c15e4e97eb20636b8f?branch=master) and the addition of cliff (same framework as openstackclient) for 1.5 which should already be in debian.

I'll close this issue since the change has landed but note that I'd also like to land the documentation from #124 for EXTERNAL_AUTH in time for 1.5.

Let me know if you have any questions !

@keuko
Copy link
Contributor

keuko commented Aug 14, 2020 via email

@dmsimard
Copy link
Contributor

Hey @keuko,

Do you have a link for the debian packaging files ? It would be nice to write it down somewhere along with the packaging files for Fedora and SUSE.

I empathize with your feelings for docs, I think we need to do a good docs refresh in general but I won't bother you with that :)

For now if you'd like to add the external auth docs, the best fit would be in the "ARA API Server authentication and security" section (or here in the git repo). Don't be scared to take a first stab at it and we can iterate together during code review if need be.

The support for it in the Ansible roles aren't required. They're nice to have, though, because we use the roles during integration tests so we not only test the roles themselves but also the different ways to deploy and configure ara. I can take care of this eventually.

Note that the roles haven't been removed from this repository just yet but we're in the process of moving those to a collection: https://github.com/ansible-community/ara-collection

@dmsimard
Copy link
Contributor

Ah, I found the debian packaging work through https://tracker.debian.org/pkg/python-ara. Thank you very much for your work on that !

@keuko
Copy link
Contributor

keuko commented Aug 17, 2020 via email

@dmsimard
Copy link
Contributor

Hey @keuko and @viktorkrivak, I just wanted to let you know that external auth was released in 1.5 today.
You can see the full release notes here and the highlights on the blog: https://ara.recordsansible.org/blog/2020/09/23/announcing-the-release-of-ara-1.5.0/

Thanks for your patience and your help !

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

No branches or pull requests

3 participants