PR#36: Implement proxyuser feature (RhBug: 1381574) - copr/copr

		`@@ -0,0 +1,26 @@`
		`+ """Add proxy column to user table`
		`+`
		`+ Revision ID: 38ea34def9a`
		`+ Revises: 4af9d157c4ea`
		`+ Create Date: 2017-02-06 17:56:10.187111`
		`+`
		`+ """`
		`+`
		`+ # revision identifiers, used by Alembic.`
		`+ revision = '38ea34def9a'`
		`+ down_revision = '4af9d157c4ea'`
		`+`
		`+ from alembic import op`
		`+ import sqlalchemy as sa`
		`+`
		`+`
		`+ def upgrade():`
		`+ ### commands auto generated by Alembic - please adjust! ###`
msuchy commented 8 years ago I am always trying to remove this comment. When removed this is sign I actually reviewed it and it is not "generated and I did not checked it, hopefully it will be ok".
		`+ op.add_column('user', sa.Column('proxy', sa.Boolean(), nullable=True))`
		`+ ### end Alembic commands ###`
		`+`
		`+`
		`+ def downgrade():`
		`+ ### commands auto generated by Alembic - please adjust! ###`
		`+ op.drop_column('user', 'proxy')`
		`+ ### end Alembic commands ###`

frontend/coprs_frontend/coprs/models.py

file modified

		`@@ -51,6 +51,9 @@`
		`# is this user admin of the system?`
		`admin = db.Column(db.Boolean, default=False)`

		`+ # can this user behave as someone else?`
		`+ proxy = db.Column(db.Boolean, default=False)`
		`+`
		`# stuff for the cli interface`
		`api_login = db.Column(db.String(40), nullable=False, default="abc")`
		`api_token = db.Column(db.String(40), nullable=False, default="abc")`

frontend/coprs_frontend/coprs/views/misc.py

file modified

		`@@ -268,6 +268,9 @@`
		`if (user and user.api_token == token and`
		`user.api_token_expiration >= datetime.date.today()):`

		`+ if user.proxy and "username" in flask.request.form:`
		`+ user = UsersLogic.get(flask.request.form["username"]).first()`
		`+`
		`token_auth = True`
		`flask.g.user = user`
		`if not token_auth:`

frontend/coprs_frontend/manage.py

file modified

+26 -36

		`@@ -285,33 +285,26 @@`
		`)`


		`- class AddDebugUserCommand(Command):`
		`+ class AddUserCommand(Command):`

		`"""`
		`- Adds user for debug/testing purpose.`
		`- You shouldn't use this on production instance`
		`+ You should not use regularly as that user will not be related to FAS account.`
		`+ This should be used only for testing or adding special accounts e.g. proxy user.`
		`"""`

		`def run(self, name, mail, **kwargs):`
		`- user = User(username=name, mail=mail)`
		`+ user = models.User.query.filter(models.User.username == name).first()`
		`+ if user:`
		`+ print("User named {0} already exists.".format(name))`
		`+ return`

		`- if kwargs["admin"]:`
		`- user.admin = True`
		`- if kwargs["no_admin"]:`
		`- user.admin = False`
		`- if kwargs["proven"]:`
		`- user.proven = True`
		`- if kwargs["no_proven"]:`
		`- user.proven = False`
		`- #`
		`- # if kwargs["api_token"]:`
		`- # user.api_token = kwargs["api_token"]`
		`- # user.api_token_expiration = datetime.date(2030, 1, 1)`
		`- # if kwargs["api_login"]:`
		`- # user.api_token = kwargs["api_login"]`
		`- #`
		`-`
		`- db.session.add(create_user_wrapper(user, mail))`
		`+ user = create_user_wrapper(name, mail)`
		`+ if kwargs["api_token"]:`
		`+ user.api_token = kwargs["api_token"]`
		`+ if kwargs["api_login"]:`
		`+ user.api_token = kwargs["api_login"]`
		`+`
		`+ db.session.add(user)`
		`db.session.commit()`

		`option_list = (`
		`@@ -319,20 +312,6 @@`
		`Option("mail"),`
		`Option("--api_token", default=None, required=False),`
		`Option("--api_login", default=None, required=False),`
		`- Group(`
		`- Option("--admin",`
		`- action="store_true"),`
		`- Option("--no-admin",`
		`- action="store_true"),`
		`- exclusive=True`
		`- ),`
		`- Group(`
		`- Option("--proven",`
		`- action="store_true"),`
		`- Option("--no-proven",`
		`- action="store_true"),`
		`- exclusive=True`
		`- ),`
		`)`


		`@@ -353,6 +332,10 @@`
		`user.proven = True`
		`if kwargs["no_proven"]:`
		`user.proven = False`
		`+ if kwargs["proxy"]:`
		`+ user.proxy = True`
		`+ if kwargs["no_proxy"]:`
		`+ user.proxy = False`

		`db.session.add(user)`
		`db.session.commit()`
		`@@ -372,6 +355,13 @@`
		`Option("--no-proven",`
		`action="store_true"),`
		`exclusive=True`
		`+ ),`
		`+ Group(`
		`+ Option("--proxy",`
		`+ action="store_true"),`
		`+ Option("--no-proxy",`
		`+ action="store_true"),`
		`+ exclusive=True`
		`)`
		`)`

		`@@ -455,7 +445,7 @@`
		`manager.add_command("display_chroots", DisplayChrootsCommand())`
		`manager.add_command("drop_chroot", DropChrootCommand())`
		`manager.add_command("alter_user", AlterUserCommand())`
		`- manager.add_command("add_debug_user", AddDebugUserCommand())`
		`+ manager.add_command("add_user", AddUserCommand())`
		`manager.add_command("fail_build", FailBuildCommand())`
		`manager.add_command("update_indexes", UpdateIndexesCommand())`
		`manager.add_command("update_indexes_quick", UpdateIndexesQuickCommand())`

python/copr/client/client.py

file modified

		`@@ -189,6 +189,8 @@`
		`if not skip_auth:`
		`kwargs["auth"] = (self.login, self.token)`
		`if data is not None:`
		`+ if type(data) != MultipartEncoderMonitor:`
		`+ data["username"] = username`
		`kwargs["data"] = data`
		`if headers is not None:`
		`kwargs["headers"] = headers`
		`@@ -546,6 +548,7 @@`
		`background=False, progress_callback=None, multipart=False):`
		`if not username:`
		`username = self.username`
		`+ data["username"] = username`

		`url = "{0}/coprs/{1}/{2}/{3}/".format(`
		`self.api_url, username, projectname, api_endpoint`

frostyx commented 8 years ago

The feature is described in BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1381574

We need this feature in order to user can submit build into our MBS and MBS Builder can do things in Copr in name of the user.

With this PR it is possible to create such proxyuser as:

python manage.py add_debug_user mbs foo@bar.baz
python manage.py alter_user mbs --proxy

Then on MBS instance we can have config:

username =
login = <mbs-user-login>
token = <mbs-user-token>
copr_url = <some-copr-instance>

And username can be dynamically set to whatever user. Copr will verify token and login, finds out that it is proxyuser and it will act as username

praiskup commented 8 years ago

Cool, add_debug_user is new for me, thanks! But manage.py says:

add_debug_user ... You shouldn't use this on production instance

Based on your report, is not truth probably anymore, right? Can we fix that comment then? Possibly we could rather warn admin to choose a "proper" username (to not cause clash with users from OpenID or kerberos.

praiskup commented 8 years ago

It would be nice to have at least "model change" + "migration" in one commit, so every git hash is properly usable.

msuchy commented on line 19 of frontend/coprs_frontend/alembic/schema/versions/38ea34def9a_add_proxy_column_to_user_table.py 8 years ago

I am always trying to remove this comment. When removed this is sign I actually reviewed it and it is not "generated and I did not checked it, hopefully it will be ok".

msuchy commented on line 18 of frontend/coprs_frontend/manage.py 8 years ago

I understand removal of "proven". But why you removed "admin"?

msuchy commented 8 years ago

I agree that "add_debug_user" should be probably renamed to "add_user" with note in --help:
"""" You should not use regularly as that user will not be related to FAS account. This should be used only for testing or adding special accounts e.g. proxy user. """

frostyx commented 8 years ago

Thank you for review guys,

It would be nice to have at least "model change" + "migration" in one commit, so every git hash is properly usable.

Both are in d6cd091 or am I missing something?

Cool, add_debug_user is new for me, thanks!

I didn't know about it either. I suppose that nobody uses it, since run method failed on it's first line. I will rename it and add the help as @msuchy said above.

frostyx commented 8 years ago

I am always trying to remove this comment. When removed this is sign I actually reviewed it and it is not "generated and I did not checked it, hopefully it will be ok".

Sure, I usually let the comments so that it is obvious that --autogenerate was used. But you are right, it looks like it was generated and committed without reviewing. I will remove those comments next time.

I understand removal of "proven". But why you removed "admin"?

I dislike the duplicity (lines of code, command usage). IMO it is pointless to have all the possibilities like proven, admin or proxy, when creating new user, because we have alter_user command for all of that.

praiskup commented 8 years ago