copr / copr

Clone

#377 Don't hardcode OpenID provider (see #374)
Merged 5 years ago by clime. Opened 5 years ago by frostyx.
copr/ frostyx/copr openid-provider  into  master

file modified
+2 
@@ -119,3 +119,5 @@ 
 

  

  NEWS_URL = "https://fedora-copr.github.io/"
 

  NEWS_FEED_URL = "https://fedora-copr.github.io/feed.xml"
 

+ 

+ OPENID_PROVIDER_URL = "https://id.fedoraproject.org"

file modified
+2 
@@ -14,6 +14,8 @@ 
 

      KRB5_LOGIN_BASEURI = "/krb5_login/"
 

      KRB5_LOGIN = {}
 

  

+     OPENID_PROVIDER_URL = "https://id.fedoraproject.org"

Can we rather have this:
OPENID_PROVIDER_URL = "https://{0}.id.fedoraproject.org"
Then we do not need to parse it and we can just format() this string.

Isn't there a different way to get fasusername, e.g. from the session, instead of the URL? It smells like the url is fedora specific.

Isn't there a different way to get fasusername, e.g. from the session, instead of the URL?

Unfortunately when fed_raw_name function is called, flask.g.user is None.

Can we rather have this:
OPENID_PROVIDER_URL = "https://{0}.id.fedoraproject.org"
Then we do not need to parse it and we can just format() this string.

I think that this would not help. You see, constructing URL with FAS name in it is a one thing (which we actually don't even do, as I said in the PR description), but we also need to parse FAS name from a given URL and in login function we need to use the provider URL without FAS name in it. Considering all these use cases, I find the current implementation better. What do you think?

Isn't there a different way to get fasusername, e.g. from the session, instead of the URL? It smells like the url is fedora specific.

Hmm, actually, there is a solution. We can change the login function and ask also for a nickname

ask_for=["email", "timezone", "nickname"],

Then, instead of using fed_raw_name, we could just do resp.nickname.
There are two things, that I would like to note. First, users will have to approve the permissions form again, before logging in. Second, I am not 100% sure, that in all cases that nickname equals to the currently used username from the URL. I will have to check that.

I looked it just now, and I agree that nickname migth bring some risk. So I agree that parsing the identity_url is the best we can do now, and we can expect that re.sub("https?://([^.]+).*", "\\1", ..) works...

I guess I know where we are making mistake ...the concept of identity url is meant to be unique across all the OpenID providers I guess. And we make a bad assumption that copr user id needs to match the FAS user id...

In theory, users could be allowed to login using any OpenID provider. The thing is that we would have to pair identity_provider url(s) with arbitrarily chosen copr user id (that might be related to #344 where we discuss that groups could be merged with users). And even more, the mapping between OID and COPRID could be M:N.
Anyways, this is not what is @mizdebsk requesting..

That said, as long as we have that constraint (single OID provider per copr instance), I think it is completely fine to parse the userid from identity_url.

clime commented 5 years ago

Strictly speaking, the param should be called 'FAS_OPENID_PROVIDER_URLbecause our code is quite specific to fedoraproject.org openID provider (assuming the <username>.<domainname> structure, theFAS_ALL_GROUPS` param...

 
+ 

      # restrict access to a set of users
 

      USE_ALLOWED_USERS = False
 

      ALLOWED_USERS = []

file modified
+6 -16 
@@ -9,6 +9,7 @@ 
 

  import flask
 

  from flask import send_file
 

  

+ from urllib.parse import urlparse
 

  from openid_teams.teams import TeamsRequest
 

  

  from coprs import app
 
@@ -21,17 +22,6 @@ 
 

  from coprs.logic.coprs_logic import CoprsLogic
 

  

  

- def fed_openidize_name(name):
 

-     """
 

-     Create proper Fedora OpenID name from short name.
 

- 

-     >>> fedoraoid == fed_openidize_name(user.name)
 

-     True
 

-     """
 

- 

-     return "http://{0}.id.fedoraproject.org/".format(name)
 

- 

- 

  def create_user_wrapper(username, email, timezone=None):
 

      expiration_date_token = datetime.date.today() + \
 

          datetime.timedelta(
 
@@ -49,8 +39,9 @@ 
 

  

  

  def fed_raw_name(oidname):
 

-     return oidname.replace(".id.fedoraproject.org/", "") \
 

-                   .replace("http://", "")
 

+     oidname_parse = urlparse(oidname)
 

+     config_parse = urlparse(app.config["OPENID_PROVIDER_URL"])
 

+     return oidname_parse.netloc.replace(".{0}".format(config_parse.netloc), "")
 

  

  

  def krb_straighten_username(krb_remote_user):
 
@@ -201,7 +192,7 @@ 
 

      else:
 

          # a bit of magic
 

          team_req = TeamsRequest(["_FAS_ALL_GROUPS_"])
 

-         return oid.try_login("https://id.fedoraproject.org/",
 

+         return oid.try_login(app.config["OPENID_PROVIDER_URL"],
 

                               ask_for=["email", "timezone"],
 

                               extensions=[team_req])
 

  
@@ -209,8 +200,7 @@ 
 

  @oid.after_login
 

  def create_or_login(resp):
 

      flask.session["openid"] = resp.identity_url
 

-     fasusername = resp.identity_url.replace(
 

-         ".id.fedoraproject.org/", "").replace("http://", "")
 

+     fasusername = fed_raw_name(resp.identity_url)
 

  

      # kidding me.. or not
 

      if fasusername and (

file added
+18 
@@ -0,0 +1,18 @@ 
 

+ from coprs import app
 

+ from coprs.views.misc import fed_raw_name
 

+ from tests.coprs_test_case import CoprsTestCase
 

+ 

+ 

+ class TestMisc(CoprsTestCase):
 

+     def test_fed_raw_name(self):
 

+         providers = [
 

+             "https://id.fedoraproject.org/",
 

+             "https://id.fedoraproject.org",
 

+         ]
 

+         for provider in providers:
 

+             app.config["OPENID_PROVIDER_URL"] = provider
 

+             assert fed_raw_name("https://someuser.id.fedoraproject.org/") == "someuser"
 

+ 

+     def test_fed_raw_name_scheme(self):
 

+         app.config["OPENID_PROVIDER_URL"] = "foo://id.fedoraproject.org/"
 

+         assert fed_raw_name("bar://someuser.id.fedoraproject.org/") == "someuser"

This PR is dealing with issue #374 and moves a hardcoded OpenID provider out to the configuration.
I've also added few tests, so we can be sure that it works as expected.

Please note, that function fed_openidize_name is not used anywhere and can be safely removed, but I decided to keep it,
as it may come in handy in the future. If you prefer to remove it, we can do so.

Can we rather have this:
OPENID_PROVIDER_URL = "https://{0}.id.fedoraproject.org"
Then we do not need to parse it and we can just format() this string.

Isn't there a different way to get fasusername, e.g. from the session, instead of the URL? It smells like the url is fedora specific.

Isn't there a different way to get fasusername, e.g. from the session, instead of the URL?

Unfortunately when fed_raw_name function is called, flask.g.user is None.

Can we rather have this:
OPENID_PROVIDER_URL = "https://{0}.id.fedoraproject.org"
Then we do not need to parse it and we can just format() this string.

I think that this would not help. You see, constructing URL with FAS name in it is a one thing (which we actually don't even do, as I said in the PR description), but we also need to parse FAS name from a given URL and in login function we need to use the provider URL without FAS name in it. Considering all these use cases, I find the current implementation better. What do you think?

Isn't there a different way to get fasusername, e.g. from the session, instead of the URL? It smells like the url is fedora specific.

Hmm, actually, there is a solution. We can change the login function and ask also for a nickname

ask_for=["email", "timezone", "nickname"],

Then, instead of using fed_raw_name, we could just do resp.nickname.
There are two things, that I would like to note. First, users will have to approve the permissions form again, before logging in. Second, I am not 100% sure, that in all cases that nickname equals to the currently used username from the URL. I will have to check that.

I looked it just now, and I agree that nickname migth bring some risk. So I agree that parsing the identity_url is the best we can do now, and we can expect that re.sub("https?://([^.]+).*", "\\1", ..) works...

I guess I know where we are making mistake ...the concept of identity url is meant to be unique across all the OpenID providers I guess. And we make a bad assumption that copr user id needs to match the FAS user id...

In theory, users could be allowed to login using any OpenID provider. The thing is that we would have to pair identity_provider url(s) with arbitrarily chosen copr user id (that might be related to #344 where we discuss that groups could be merged with users). And even more, the mapping between OID and COPRID could be M:N.
Anyways, this is not what is @mizdebsk requesting..

That said, as long as we have that constraint (single OID provider per copr instance), I think it is completely fine to parse the userid from identity_url.

iow +1

+1

Why are you using six when frontend is already only python3? I understand we have this on multiple other places as well. Can you open a ticket about it (removing the six references from the code)?

Strictly speaking, the param should be called 'FAS_OPENID_PROVIDER_URLbecause our code is quite specific to fedoraproject.org openID provider (assuming the <username>.<domainname> structure, theFAS_ALL_GROUPS` param...

I am not sure why you are editing and even testing dead code. This function was actually never used according to our git history.

I think it would be good to at least fix the unneeded use of six. Otherwise +1.

Why are you using six when frontend is already only python3? I understand we have this on multiple other places as well.

Exactly because of this reason. I didn't want to deviate from the rest of the code

To be honest, I thought that I will need this function. As it turned out, I read the code wrong and realized that I don't actually need it. I've stated it in the PR description, we can throw the function (and test) away, or say, that it could be helpful in the future and keep it. I don't really mind any of this options.

Well, keeping around methods, which are not used and even develop on them seems like a bad idea to me.

Well, the rest of code should be updated. It would be good to continuously clean the code from the outdated/unneeded things so that we can e.g. drop uneeded requires/build-requires in then end. It would be good to fix it here and file a ticket about needed cleanup or comment at https://pagure.io/copr/copr/issue/346 that this should be part of it.

On Friday, August 31, 2018 9:11:37 AM CEST clime wrote:

Well, the rest of code should be updated.

Not here, no.

It would be good to continuously clean the code from the outdated/unneeded
things

Please no, not continuously. It is bad habit for git maintenance, and makes
e.g. backporting almost impossible. The git-log becomes unreadable.

We should do the cleanups as a part of separate issues, and since it's not
burning - we should first setup an automatic guards against making the same
mistakes again. So, e.g. we should be able to install pull-request CI task that
checks that python-six is not used in frontend, backend, ..., code. And then
start with the cleanups.

so that we can e.g. drop uneeded requires/build-requires in then end.

We should have CI taks which will try to at least install the packages, so we
don't face issues like
https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2018-e206e83be9 again.

It would be good to fix it here and file a ticket about needed cleanup or
comment at
https://pagure.io/copr/copr/issue/346 that this should be part of it. ``

Agreed.

2 new commits added

  • [frontend] remove unused fed_openidize_name function
  • [frontend] use standard python3 urlparse
5 years ago

Well, keeping around methods, which are not used and even develop on them seems like a bad idea to me.

I've explained, that I misread the code and thought, that I will need that function. And since the changes were already done, I've stated, that the function is not used and whether you guys prefer it to stay or get removed. It's not like I had nothing better to do than pick a useless function and refactor it. I am removing it now.

I think it would be good to at least fix the unneeded use of six

Np, I've updated it. I have a different view on it, though. I think, that it is better to do things the same way as they are done in different places within the codebase. Then decide to and migrate all of it at once. Because otherwise, it can happen, that we can decide to migrate to something else, then standard urlparse (because of functionality, speed, or whatever, it doesn't matter) and then someone will have to go through all the things, that we used within the years and migrate them. Which is much more annoying, then knowing, that currently everywhere should be urlparse from six.

+1, thanks

Np, I've updated it. I have a different view on it, though. I think, that it is better to do things the same way as they are done in different places within the codebase.

I am ok with that if there is a note or a ticket somewhere so that we remember to do it.

Anyway, thanks.

Pull-Request has been merged by clime
5 years ago
Metadata
None
No Tags
Changes Summary 4
+2 -0
file changed
docker/frontend/files/etc/copr/copr.conf
+2 -0
file changed
frontend/coprs_frontend/coprs/config.py
+6 -16
file changed
frontend/coprs_frontend/coprs/views/misc.py
+18
file added
frontend/coprs_frontend/tests/test_views/test_misc.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.