Skip to content

auth: allow external accounts to login and restrict upload - #2091

Merged
zzacharo merged 2 commits into
CERNDocumentServer:additional-filesfrom
zubeydecivelek:login-allow-external
Sep 9, 2025
Merged

auth: allow external accounts to login and restrict upload#2091
zzacharo merged 2 commits into
CERNDocumentServer:additional-filesfrom
zubeydecivelek:login-allow-external

Conversation

@zubeydecivelek

@zubeydecivelek zubeydecivelek commented Aug 18, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

In SSO application:

  • Role cern-user should change to not required
  • Add new role with Minimum Level of Assurance and identifier authenticated-user.
    • Enable: This role is required to access my application
    • Enable: This role applies to all authenticated users

Permissions:

  • Users should have cern-user to upload/create.
  • This role is connected with action: upload_access_action
cds access allow videos-upload-access role cern-user

TODO

  • Write unit test
  • Test restriction for all endpoints
  • Better naming for new SSO role
  • Hide Upload button from the top navbar
  • Restrict edit even if external users in the access.update

@zubeydecivelek zubeydecivelek linked an issue Aug 18, 2025 that may be closed by this pull request
Allow sharing of published records with non-CERN users #1935
Closed
jrcastro2
jrcastro2 reviewed Aug 19, 2025
View reviewed changes

@jrcastro2 jrcastro2 left a comment

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.

Good job! Looks neat!

I would add some comments explaining that some values are only present in the CERN users and others on external accounts. I would maybe also check with the Auth team that the sub value is always present in all non cern accounts, and if not ask them which one we could use.

Also did you manage to test it locally with an external account?

Comment thread cds/modules/oauthclient/cern_openid.py Outdated
Comment thread cds/modules/oauthclient/cern_openid.py
@zubeydecivelek

zubeydecivelek commented Aug 19, 2025

Copy link
Copy Markdown
Contributor Author

@jrcastro2

Also did you manage to test it locally with an external account?

Yes, I tested with Google, Github and External email and all of them worked well

jrcastro2
jrcastro2 approved these changes Aug 19, 2025
View reviewed changes

@jrcastro2 jrcastro2 left a comment

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.

LGTM!

zubeydecivelek
zubeydecivelek commented Aug 19, 2025
View reviewed changes
Comment thread cds/modules/oauthclient/cern_openid.py Outdated
Comment on lines +189 to +193
raw_username = resource.get("preferred_username") or email
if "@" in raw_username:
raw_username = raw_username.split("@")[0]

@zubeydecivelek zubeydecivelek Aug 19, 2025

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.

We should find a better way for username, it should be unique

@zubeydecivelek zubeydecivelek Sep 8, 2025

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.

@zzacharo do you have any idea for username?

@zzacharo zzacharo Sep 8, 2025

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.

what about if you just replace @ with - or _? The rule about the valid usernames is defined via this config. Eventually, we could change the pattern to allow the email as username. @ntarocco wdyt?

@zubeydecivelek
zubeydecivelek force-pushed the login-allow-external branch 3 times, most recently from 4f35af1 to 53ab9ed Compare September 8, 2025 11:46
zzacharo
zzacharo reviewed Sep 8, 2025
View reviewed changes
Comment thread cds/modules/records/permissions.py Outdated
Comment thread tests/unit/test_external_user.py
@zubeydecivelek
zubeydecivelek force-pushed the login-allow-external branch 3 times, most recently from 742efb1 to 76914a1 Compare September 8, 2025 15:53
zzacharo
zzacharo reviewed Sep 9, 2025
View reviewed changes
Comment thread cds/modules/oauthclient/cern_openid.py

with db.session.begin_nested():
external_id = resource.get("cern_uid")
external_id = resource.get("cern_uid") or resource.get("sub")

@zzacharo zzacharo Sep 9, 2025

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.

What is the sub key here? Is it unique?

@zubeydecivelek zubeydecivelek Sep 9, 2025

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.

Yes it's unique, See here

zzacharo
zzacharo reviewed Sep 9, 2025
View reviewed changes
Comment thread cds/modules/oauthclient/cern_openid.py Outdated
Comment thread cds/config.py Outdated
Comment thread tests/unit/test_external_user.py Outdated
Comment thread tests/unit/test_external_user.py Outdated
zzacharo
zzacharo reviewed Sep 9, 2025
View reviewed changes
Comment thread cds/modules/invenio_deposit/views/ui.py Outdated
@zubeydecivelek
zubeydecivelek force-pushed the login-allow-external branch 2 times, most recently from 6101ab8 to d043bae Compare September 9, 2025 13:07
zzacharo
zzacharo reviewed Sep 9, 2025
View reviewed changes
Comment thread tests/unit/test_external_user.py
zzacharo
zzacharo reviewed Sep 9, 2025
View reviewed changes
Comment thread cds/modules/oauthclient/cern_openid.py
external_id = str(external_id)
raw_username = resource.get("preferred_username") or email
if "@" in raw_username:
raw_username = raw_username.replace("@", "_").replace(".", "_")

@zzacharo zzacharo Sep 9, 2025

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.

in which cases we have a . and you replace it?

@zubeydecivelek zubeydecivelek Sep 9, 2025

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.

If we have @ in preferred_username which is email/gmail account, you have ......@gmail.com and as username . also invalid. that's why I'm replacing but it can change

@ntarocco ntarocco Sep 10, 2025

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.

Interesting, we did not do that in CDS-RDM. We are using the sub for the username, see here.

@zzacharo
zzacharo merged commit 76961d1 into CERNDocumentServer:additional-files Sep 9, 2025
1 check passed
@zubeydecivelek zubeydecivelek mentioned this pull request Sep 9, 2025
Closed
ntarocco
ntarocco reviewed Sep 10, 2025
View reviewed changes
Comment thread cds/modules/oauthclient/cern_openid.py
external_id = str(external_id)
raw_username = resource.get("preferred_username") or email
if "@" in raw_username:
raw_username = raw_username.replace("@", "_").replace(".", "_")

@ntarocco ntarocco Sep 10, 2025

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.

Interesting, we did not do that in CDS-RDM. We are using the sub for the username, see here.

Comment thread scripts/setup
cds roles add admin@test.ch admin
cds access allow deposit-admin-access role admin
cds access allow superuser-access role admin
cds access allow videos-upload-access role cern-user

@ntarocco ntarocco Sep 10, 2025

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.

remember to apply this when deploying. You might want to already do it in all instances to be sure that you don't forget.

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

Reviewers

@ntarocco ntarocco ntarocco left review comments
@jrcastro2 jrcastro2 jrcastro2 approved these changes
@zzacharo zzacharo zzacharo approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Allow sharing of published records with non-CERN users

4 participants

@zubeydecivelek @ntarocco @jrcastro2 @zzacharo