Skip to content

Authentication#44

Merged
sinnec merged 7 commits intomainfrom
app-authentication
Nov 23, 2022
Merged

Authentication#44
sinnec merged 7 commits intomainfrom
app-authentication

Conversation

@sinnec
Copy link
Copy Markdown
Contributor

@sinnec sinnec commented Nov 17, 2022

No description provided.

@sinnec sinnec requested a review from momegas November 17, 2022 15:07
sinnec
sinnec commented Nov 17, 2022
View reviewed changes
Comment thread src/core/db.py Outdated
momegas
momegas reviewed Nov 17, 2022
View reviewed changes
Comment thread src/api/v1/models.py Outdated
@gcharis
Copy link
Copy Markdown
Contributor

gcharis commented Nov 18, 2022

The tests are failing 🙅‍♂️

@sinnec
Copy link
Copy Markdown
Contributor Author

sinnec commented Nov 21, 2022

@gcharis @momegas Do you think the auth stuff that use the token should be removed for now?

@momegas
Copy link
Copy Markdown
Member

momegas commented Nov 21, 2022

@gcharis @momegas Do you think the auth stuff that use the token should be removed for now?

What do you mean "the token"? Which calls are you refering to?

@sinnec sinnec marked this pull request as draft November 21, 2022 09:26
@sinnec sinnec changed the title [Draft] Authentrication Authentication Nov 21, 2022
@sinnec
Copy link
Copy Markdown
Contributor Author

sinnec commented Nov 21, 2022

@gcharis @momegas Do you think the auth stuff that use the token should be removed for now?

What do you mean "the token"? Which calls are you refering to?

I mean the auth endpoint and functions that use a token to authorize the user.

@momegas
Copy link
Copy Markdown
Member

momegas commented Nov 21, 2022

Yeah, any code not in use should be removed

@sinnec sinnec marked this pull request as ready for review November 21, 2022 15:25
momegas
momegas reviewed Nov 21, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@momegas momegas left a comment

Choose a reason for hiding this comment

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

if not authenticated:
        return errors.unauthorized()

I think this needs to be in a middleware

momegas
momegas reviewed Nov 21, 2022
View reviewed changes
Comment thread src/tests/v1/test_drifting_metrics.py Outdated
@sinnec sinnec requested a review from momegas November 22, 2022 15:41
momegas
momegas approved these changes Nov 23, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@momegas momegas left a comment

Choose a reason for hiding this comment

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

Looks great.

Comment thread src/api/v1/dataset_rows.py Outdated
@sinnec sinnec merged commit 3d9fbda into main Nov 23, 2022
@gcharis gcharis deleted the app-authentication branch December 20, 2022 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gcharis gcharis gcharis left review comments

@momegas momegas momegas approved these changes

Assignees

@gcharis gcharis

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sinnec @gcharis @momegas