Skip to content

Add user provided auth mechanism#21790

Merged
PVince81 merged 10 commits into
masterfrom
ext-auth-user-provided
Jan 29, 2016
Merged

Add user provided auth mechanism#21790
PVince81 merged 10 commits into
masterfrom
ext-auth-user-provided

Conversation

@icewind1991

Copy link
Copy Markdown
Contributor

This adds an authentication option for admin defined mounts that require the user to enter the credentials on first use.

Currently the user can set his credentials in the personal settings, a nice dialog for entering the credentials if they are missing/wrong is to be added separately (iirc @jmaciasportela was going to work on that)

cc @PVince81 @Xenopathic @jmaciasportela

@icewind1991 icewind1991 added this to the 9.0-current milestone Jan 19, 2016
@mention-bot

Copy link
Copy Markdown

By analyzing the blame information on this pull request, we identified @MorrisJobke, @Xenopathic and @nickvergessen to be potential reviewers

@icewind1991

Copy link
Copy Markdown
Contributor Author

Usage

  1. Admin creates mount and selects user provided auth admin
  2. Users sees mount with missing/invalid credentials invalid
  3. User enters his credentials in the personal settings settings
  4. Mount is happy and works works

@RobinMcCorkell

Copy link
Copy Markdown
Contributor

I feel as though this is a really hacky way to implement this, with the separate controller, custom logic in the StoragesServices, and custom JS. I think a better way would be to leverage standard storage saving procedures, but have a way to define certain parameters as being user-modifyable, so they don't get greyed out in the settings area.

The whole point of the super-generic backend code was to avoid needing custom logic like this 🙊

@icewind1991 icewind1991 force-pushed the ext-auth-user-provided branch from 7a72208 to 855f9f1 Compare January 19, 2016 15:59
@icewind1991

Copy link
Copy Markdown
Contributor Author

Removed the custom controller and auth mechanism specific logic from the js.

the UserGlobalStoragesController now allows editing for storages that have an auth mechanism that implement IUserProvided, the auth mechanism can control what fields are visible for the user to edit (and hidden for the admin) by setting FLAG_USER_PROVIDED on the parameters

Comment thread apps/files_external/appinfo/routes.php Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some leftovers need to be removed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

@RobinMcCorkell

Copy link
Copy Markdown
Contributor

I like this a lot! Once the other PR is merged and this rebased, I'll review it

@icewind1991 icewind1991 force-pushed the ext-auth-user-provided branch from 855f9f1 to 519f82e Compare January 19, 2016 16:26
@PVince81

Copy link
Copy Markdown
Contributor

@icewind1991 please rebase to include the changes from the other PR

@icewind1991 icewind1991 force-pushed the ext-auth-user-provided branch from 519f82e to a68bbe3 Compare January 22, 2016 12:42
@icewind1991

Copy link
Copy Markdown
Contributor Author

rebased

@PVince81

Copy link
Copy Markdown
Contributor

@icewind1991 please fix the failing JS unit tests

@PVince81

Copy link
Copy Markdown
Contributor
  • BUG: status indicator must be yellow, not red (just like for "login credentials")

@PVince81

Copy link
Copy Markdown
Contributor
  • BUG: edit the configuration several times, let it save, refresh the page: config duplicated

Steps:

  1. Setup a SFTP mount
  2. Select "User provided"
  3. Type in host and path
  4. Modify path once, exit field, wait for save
  5. Modify path agin, exit field, wait for save
  6. Refresh page

In all cases, the indicator was red and after refreshing the page, I had 3 SFTP configs.

@icewind1991 icewind1991 force-pushed the ext-auth-user-provided branch from a68bbe3 to 3c80c8f Compare January 25, 2016 15:24
@PVince81

Copy link
Copy Markdown
Contributor

@icewind1991 please fix the bugs I mentionned above.
The status indicator is still red the first time instead of yellow.
After refreshing the page it becomes yellow, but the "config multiplication" issue is still there.

@PVince81

Copy link
Copy Markdown
Contributor
  • BUG: the mount point doesn't appear neither for SMB nor SFTP. Expected: the mount point must appear in the file list and be shown in red with the message "credentials required" (I know the dialog doesn't exist yet)

@PVince81

Copy link
Copy Markdown
Contributor

Basically on my env it doesn't work like in your screenshots here #21790 (comment).
Even for SMB the status is red.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IUserProvided instead of UserProvided ?

@icewind1991

Copy link
Copy Markdown
Contributor Author

Also broken on master

@PVince81

Copy link
Copy Markdown
Contributor

@icewind1991 please rebase, the master bug was merged

@icewind1991 icewind1991 force-pushed the ext-auth-user-provided branch 2 times, most recently from 37acfbf to 1426ceb Compare January 28, 2016 14:26
@PVince81

Copy link
Copy Markdown
Contributor

@DeepDiver1975 all tests pass, re-review ?

@DeepDiver1975

Copy link
Copy Markdown
Member

@icewind1991 you force pushed and killed my commit which cleaned up some use statments 👿

@PVince81

Copy link
Copy Markdown
Contributor

@DeepDiver1975 you might be able to cherry-pick them from your local git branch

@DeepDiver1975

Copy link
Copy Markdown
Member

@DeepDiver1975 you might be able to cherry-pick them from your local git branch

the changeset is not worth the effort - but the 'disrespect' in contribution is worth a discussion

@icewind1991

Copy link
Copy Markdown
Contributor Author

Sorry, didn't see the commit while doing the rebasing

@PVince81

Copy link
Copy Markdown
Contributor

@DeepDiver1975 I also tend to rebase+force push often as it seldom happens that people push on my branches, easy to be overlooked. When pushing it's best to also post a comment to say so to make sure the PR owner gets a notification. I also didn't see your commit 😦

Let's be more careful in the future.

@PVince81

Copy link
Copy Markdown
Contributor

@DeepDiver1975 any remaining objections to merging this PR ?

@DeepDiver1975

Copy link
Copy Markdown
Member

@DeepDiver1975 any remaining objections to merging this PR ?

no

👍

@DeepDiver1975

Copy link
Copy Markdown
Member

needs rebase - conflict in setting.js

@PVince81

Copy link
Copy Markdown
Contributor

Noooooooooooooooooooooh. @icewind1991 ?

@icewind1991 icewind1991 force-pushed the ext-auth-user-provided branch from 1426ceb to 6b18134 Compare January 29, 2016 13:51
PVince81 pushed a commit that referenced this pull request Jan 29, 2016
@PVince81 PVince81 merged commit 7d27af5 into master Jan 29, 2016
@PVince81 PVince81 deleted the ext-auth-user-provided branch January 29, 2016 15:56
@lock lock Bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants