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.

Sign up for GitHub

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

Jump to bottom

feat: User awaiting approval #680

Merged
merged 12 commits into from
Jul 18, 2023
Merged

feat: User awaiting approval #680

merged 12 commits into from
Jul 18, 2023

Conversation

IC-1101asterisk
Copy link
Contributor

@IC-1101asterisk IC-1101asterisk commented Jun 29, 2023 •
edited by oleobal
Loading

Description

This features adds a way to deploy Substra with users being created by the SSO as "unactivated users". They need the approval from an admin.

Fixes FL-989

How has this been tested?

Some tests needed to be changed as this is exploiting the fact that a user with no channel is an "unactivated user"

Checklist

  • changelog was updated with notable changes
  • documentation was updated

@github-actions github-actions bot added the api label Jun 29, 2023
@IC-1101asterisk IC-1101asterisk changed the title Feat unactivated user feat: User awaiting approval Jun 29, 2023
@IC-1101asterisk IC-1101asterisk force-pushed the feat-unactivated-user branch 3 times, most recently from 712bd38 to 43d3821 Compare June 30, 2023 13:19
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Jun 30, 2023
@IC-1101asterisk IC-1101asterisk marked this pull request as ready for review June 30, 2023 13:36
@IC-1101asterisk IC-1101asterisk requested a review from a team as a code owner June 30, 2023 13:36
@IC-1101asterisk
Copy link
Contributor Author

/e2e

@Owlfred
Copy link

Owlfred commented Jul 3, 2023 •
edited
Loading

End to end tests: ❌ FAILURE

Jobs status:

  • Tests Benchmark: ⏭️
  • Tests Distributed: ❌
  • Tests Standalone: ❌

“Rien ne sert de courir ; il faut partir à point.” ― Jean de La Fontaine (Le Lièvre et la Tortue)

guilhem-barthes
guilhem-barthes previously requested changes Jul 5, 2023
View reviewed changes
Copy link
Contributor

@guilhem-barthes guilhem-barthes left a comment

Choose a reason for hiding this comment

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

Thanks for your work!

I think there are still some small changes to be done, especially to not leave dead code behind. I am not sure what is the best practice for HTTP_ACCEPT, but if we go to remove it, we should remove it completely.

backend/api/tests/common.py Show resolved Hide resolved
backend/api/tests/views/conftest.py Show resolved Hide resolved
backend/api/tests/views/test_utils.py Show resolved Hide resolved
backend/api/tests/views/test_views_datamanager.py Outdated Show resolved Hide resolved
backend/api/tests/views/test_views_datamanager.py Outdated Show resolved Hide resolved
backend/users/views/user.py Outdated Show resolved Hide resolved
backend/api/tests/views/test_views_compute_plan_graph.py Show resolved Hide resolved
backend/users/views/user.py Show resolved Hide resolved
backend/users/views/user.py Outdated Show resolved Hide resolved
backend/backend/urls.py Outdated Show resolved Hide resolved
@IC-1101asterisk IC-1101asterisk mentioned this pull request Jul 10, 2023
Merged
guilhem-barthes
guilhem-barthes reviewed Jul 12, 2023
View reviewed changes
backend/api/tests/views/test_views_datasample.py Outdated Show resolved Hide resolved
backend/users/views/user.py Show resolved Hide resolved
backend/backend/urls.py Outdated Show resolved Hide resolved
@IC-1101asterisk IC-1101asterisk force-pushed the feat-unactivated-user branch 3 times, most recently from 4b451e3 to 609915b Compare July 13, 2023 08:19
@IC-1101asterisk IC-1101asterisk dismissed guilhem-barthes’s stale review July 13, 2023 08:25

This has been taken into account (don't now why github is bugging me with this)

@IC-1101asterisk IC-1101asterisk force-pushed the feat-unactivated-user branch 2 times, most recently from 1873caa to c6a18bb Compare July 13, 2023 13:00
@IC-1101asterisk
Copy link
Contributor Author

/e2e

@Owlfred
Copy link

Owlfred commented Jul 13, 2023 •
edited
Loading

End to end tests: ✔️ SUCCESS

@IC-1101asterisk
feat: unactivated users exists and have no channel
3856e3b 
Signed-off-by: Léo-Paul HAUET <[email protected]>
@IC-1101asterisk
feat: unactivated user tests working (not sure why)
1c2b690 
Signed-off-by: Léo-Paul HAUET <[email protected]>
@IC-1101asterisk
feat: unactivated users endpoint working
a3f4a92 
Signed-off-by: Léo-Paul HAUET <[email protected]>
@IC-1101asterisk
feat: users awaiting approval serializers
185d01b 
Signed-off-by: Léo-Paul HAUET <[email protected]>
@IC-1101asterisk
feat: adding email and delete methods
9717d26 
Signed-off-by: Léo-Paul HAUET <[email protected]>
@IC-1101asterisk
feat: user waiting approval working PUT
acc922d 
Signed-off-by: Léo-Paul HAUET <[email protected]>
@IC-1101asterisk
feat: change IsAuthenticated to IsAuthorized
df92256 
Signed-off-by: Léo-Paul HAUET <[email protected]>
@IC-1101asterisk
feat: WIP wrong channel tests failing
36ebf0d 
Signed-off-by: Léo-Paul HAUET <[email protected]>
@IC-1101asterisk
feat: tests working with new permission
316f797 
Signed-off-by: Léo-Paul HAUET <[email protected]>
@IC-1101asterisk
feat: changed the path
2bd4fc2 
Signed-off-by: Léo-Paul HAUET <[email protected]>
@IC-1101asterisk
Copy link
Contributor Author

/e2e --tests sdk,frontend --refs substra-frontend=feat/unactivated/users

@Owlfred
Copy link

Owlfred commented Jul 18, 2023 •
edited
Loading

End to end tests: ❌ FAILURE

Jobs status:

  • Camelyon: ⏭️
  • Distributed / distributed-sdk,frontend: ❌
  • MNIST: ⏭️
  • Standalone / standalone-sdk,frontend: ❌

“Rien ne sert de courir ; il faut partir à point.” ― Jean de La Fontaine (Le Lièvre et la Tortue)

@IC-1101asterisk
Copy link
Contributor Author

/e2e --tests sdk,frontend --refs substra-frontend=feat/unactivated/users

@Owlfred
Copy link

Owlfred commented Jul 18, 2023 •
edited
Loading

End to end tests: ❌ FAILURE

Jobs status:

  • Camelyon: ⏭️
  • Distributed / distributed-sdk,frontend: ❌
  • MNIST: ⏭️
  • Standalone / standalone-sdk,frontend: ❌

“I’m sorry, Dave. I’m afraid I can’t do that.” ― Hal, 2001: A Space Odyssey

guilhem-barthes
guilhem-barthes approved these changes Jul 18, 2023
View reviewed changes
Copy link
Contributor

@guilhem-barthes guilhem-barthes left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your work! 🙏 Just a typo and a proposed change

charts/substra-backend/CHANGELOG.md Outdated Show resolved Hide resolved
charts/substra-backend/README.md Outdated Show resolved Hide resolved
@IC-1101asterisk
Copy link
Contributor Author

/e2e --tests sdk --refs substra-frontend=feat/unactivated/users

@Owlfred
Copy link

Owlfred commented Jul 18, 2023 •
edited
Loading

End to end tests: ❌ FAILURE

“You shall not pass!” ― Gandalf, The Lord of the Rings, The Fellowship of the Ring

@IC-1101asterisk
feat: using router for the ViewSet
a9e54e2 
Signed-off-by: Léo-Paul HAUET <[email protected]>
@IC-1101asterisk
Copy link
Contributor Author

/e2e --tests sdk --refs substra-frontend=feat/unactivated/users

@Owlfred
Copy link

Owlfred commented Jul 18, 2023 •
edited
Loading

End to end tests: ✔️ SUCCESS

@IC-1101asterisk IC-1101asterisk merged commit 90b4e69 into main Jul 18, 2023
9 checks passed
@IC-1101asterisk IC-1101asterisk deleted the feat-unactivated-user branch July 18, 2023 15:07
@linear
Copy link

linear bot commented Jul 20, 2023

FL-989 SSO v3: Admin should validate user

See Notion document: https://www.notion.so/owkin-fdn/SSO-v3-99a6bacb57cd4407a5e4537ae018e469

To try and better specify this feature :

  • there should be a new type of users "shadow users"
  • shadow users can connect but will be met with a page telling them to wait for approval
  • they have to contact admin on their own
  • admin will need a new page with shadow users to approve them (or on the already existing page user but what if tons of shadow users ?)

- this should be a new model

- new page "unregistered" users

- new "blank" page => go contact your admin user by slack or email

- SSO system handles differently depending on registered or un registered user

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oleobal oleobal oleobal left review comments

@guilhem-barthes guilhem-barthes guilhem-barthes approved these changes

@thbcmlowk thbcmlowk Awaiting requested review from thbcmlowk

@SdgJlbl SdgJlbl Awaiting requested review from SdgJlbl

Assignees
No one assigned
Labels
api documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@IC-1101asterisk @Owlfred @oleobal @guilhem-barthes